perf(postgres): batch doBulk + named prepared statements#997
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Review Summary by QodoPostgreSQL driver optimizations: batch doBulk and named prepared statements
WalkthroughsDescription• Batch doBulk operations into single multi-row INSERT with ON CONFLICT for native upsert • Add named prepared statements to three hottest queries (get, set, remove) • Refactor doBulk to use async.parallel with cleaner task composition • Preserve function-based fallback for old PostgreSQL/CockroachDB without native upsert Diagramflowchart LR
A["doBulk Operations"] -->|Native Upsert| B["Single Multi-row INSERT"]
A -->|Function-based| C["Per-row Upsert"]
B --> D["ON CONFLICT DO UPDATE"]
D --> E["Reduced Round-trips"]
F["Single-row Queries"] -->|Named Statements| G["get/set/remove"]
G --> H["Parse Once Per Connection"]
File Changes1. databases/postgres_db.ts
|
Code Review by Qodo
1. Write promise can hang
|
| const val = '' as any; | ||
| callback(Error('Your Key can only be 100 chars'), val); | ||
| } else if (this.upsertStatement != null) { | ||
| this.db.query(this.upsertStatement, [key, value], callback); | ||
| const name = this.upsertStatement.startsWith('INSERT INTO store(key, value) VALUES') | ||
| ? 'ueberdb_set_native' | ||
| : 'ueberdb_set_function'; | ||
| this.db.query({ name, text: this.upsertStatement, values: [key, value] }, callback); | ||
| } | ||
| } | ||
|
|
||
| remove(key:string, callback:()=>{}) { | ||
| this.db.query('DELETE FROM store WHERE key=$1', [key], callback); | ||
| remove(key: string, callback: () => {}) { | ||
| this.db.query( | ||
| { name: 'ueberdb_remove', text: 'DELETE FROM store WHERE key=$1', values: [key] }, | ||
| callback | ||
| ); | ||
| } | ||
|
|
||
| doBulk(bulk:BulkObject[], callback:()=>{}) { | ||
| const replaceVALs = []; | ||
| let removeSQL = 'DELETE FROM store WHERE key IN ('; | ||
| const removeVALs: string[] = []; | ||
| doBulk(bulk: BulkObject[], callback: () => {}) { | ||
| if (!this.upsertStatement) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
1. Write promise can hang 🐞 Bug ☼ Reliability
In databases/postgres_db.ts, set() and doBulk() can return without calling the provided callback when upsertStatement is null/undefined, which causes CacheAndBufferLayer’s util.promisify wrapper to create a Promise that never settles and can deadlock buffered writes/flush.
Agent Prompt
### Issue description
`databases/postgres_db.ts` has code paths where `set()` / `doBulk()` return early (or do nothing) if `this.upsertStatement` is not set, **without invoking the callback**. Because `CacheAndBufferLayer` wraps legacy callback APIs with `node:util.promisify()` and then `await`s them, a missing callback causes an **unresolved Promise** and can deadlock writes/flush.
### Issue Context
- `CacheAndBufferLayer` promisifies non-async DB drivers and `await`s `doBulk()` during flush.
- `CacheAndBufferLayer` also starts its periodic `flush()` timer in the constructor, so a caller that performs writes before awaiting/finishing DB `init()` can hit these paths.
### Fix Focus Areas
- Ensure **every** `set()`/`doBulk()` invocation either:
- calls the callback with an error (preferred: fail fast), or
- calls the callback successfully (only if it is safe to drop work, which it likely is not).
- Concretely: if `this.upsertStatement` is null/undefined, call `callback(new Error('...not initialized...'))` (and for `set()`, also pass an appropriate dummy result if needed), instead of returning/do-nothing.
#### Code locations
- databases/postgres_db.ts[188-210]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
82d3968 to
0aa9514
Compare
- doBulk collapses N round-trips into one on the native-upsert path via a single multi-row INSERT ... VALUES ($1,$2),($3,$4),... ON CONFLICT DO UPDATE. The function-based fallback (old PG / CockroachDB without native upsert) is preserved for single-row and multi-row cases. - Named prepared statements on the three hottest single-row queries (get, set, remove) so PG parses + plans them once per connection. Split out of #994 to keep one backend per PR (AGENTS.md). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0aa9514 to
39f6ecf
Compare
perf(postgres): PostgreSQL driver optimizations
Touches
databases/postgres_db.tsonly. Split out of the old combined MongoDB+PostgreSQL PR (#994) so each backend gets its own PR perAGENTS.md("one backend per PR"). The MongoDB half stays in #994. Thepostgres_db.tsdiff here is unchanged from the original combined PR.doBulkcollapses N round-trips into one on the native-upsert path via a single multi-rowINSERT … VALUES ($1,$2),($3,$4),… ON CONFLICT DO UPDATE. The function-based fallback (old PG / CockroachDB without native upsert) is preserved for single-row and multi-row cases.get,set,remove) so PG parses + plans them once per connection.🤖 Generated with Claude Code