drizzle-team / drizzle-team/drizzle-orm

[BUG]: Failure to cache query results (despite successful query) leads to exception.

Open
#4,757 1 comment 0 reactions 0 assignees View on GitHub
bug
Dominant language
TypeScript
Stars
35.8k
Forks
1.6k
Avg merge
2d 7h
Merged PRs (30d)
4

Description

### Report hasn't been filed before.

- [x] I have verified that the bug I'm about to report hasn't been filed before.

### What version of `drizzle-orm` are you using?

0.44.3

### What version of `drizzle-kit` are you using?

N/A

### Other packages

_No response_

### Describe the Bug

I ran into an issue in production where my application was querying Postgres with very large result sets. It turns out these result sets when json-encoded exceed the maximum string value size in Upstash redis. This in turn caused the UpstashCache.put method to fail and subsequently failed the query because of the logic here:

https://github.com/drizzle-team/drizzle-orm/blob/main/drizzle-orm/src/pg-core/session.ts#L129

UpstashError: Command 1 [ hset ] failed: ERR max single record size exceeded. Key: '__CT__AccelData', Limit: 104857600 bytes, Usage: 105206918 bytes. See https://upstash.com/docs/redis/troubleshooting/max_record_size_exceeded for details
File "web/../../../../node_modules/@upstash/redis/chunk-AIBLSL5D.mjs", line 2408, col 15, in
throw new UpstashError(
File "", in Array.map
File "web/../../../../node_modules/@upstash/redis/chunk-AIBLSL5D.mjs", line 2406, col 14, in this.exec
}) : res.map(({ error, result }, i) => {
File "node:internal/process/task_queues", line 105, col 5, in process.processTicksAndRejections
File "web/../../../../node_modules/drizzle-orm/cache/upstash/cache.js", line 145, col 47, in fq.put
pipeline.sadd(this.addTablePrefix(table), compositeKey);
File "web/../../../node_modules/drizzle-orm/pg-core/session.js", line 84, col 9, in x.queryWithCache
await this.cache.put(
File "web/../../../../node_modules/drizzle-orm/neon-serverless/session.js", line 109, col 20, in b2.execute
const result = await this.queryWithCache(query.text, params, async () => {
File "web/./src/data/accel/getAccelData.action.ts", line 84, col 16, in getAccelData

So I guess I have a few questions:

1. Is this the intended behavior of the caching functionality? I could imagine instances where caching the value is critical, but I think for most use cases caching (at least on the get/put) is a "best effort" optimization where if it's there already, great! but if it's not that's okay too. It's pretty much impossible to ensure cache consistency generally, so __ideally__ people have systems in place to handle this already.

2. If this is not the intended behavior, if we change it to just log the error, would we consider this a breaking change?
3. If we do want to allow a way to continue without throwing what would the API look like? I'm imagining something like:
```
.$withCache({
tag: "xyz",
onError(action: "get" | "put" | "invalidate", query: PreparedQuery, cause: Error) { /* error handle here */ }
}
```

I'm available to try and take a stab at the postgres implementation depending on what the decision is here.

Thanks!

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.