Skip to content

fix(universaldb): clear postgres key ranges one statement per range so the planner keeps index access - #5703

Open
MasterPtato wants to merge 1 commit into
stack/perf-universaldb-time-each-drain-batch-statement-to-localize-leader-apply-stalls-uzzxusqpfrom
stack/fix-universaldb-clear-postgres-key-ranges-one-statement-per-range-so-the-planner-keeps-index-access-opyzxvov
Open

fix(universaldb): clear postgres key ranges one statement per range so the planner keeps index access#5703
MasterPtato wants to merge 1 commit into
stack/perf-universaldb-time-each-drain-batch-statement-to-localize-leader-apply-stalls-uzzxusqpfrom
stack/fix-universaldb-clear-postgres-key-ranges-one-statement-per-range-so-the-planner-keeps-index-access-opyzxvov

Conversation

@MasterPtato

Copy link
Copy Markdown
Contributor

No description provided.

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found

Reviewed commit ca27c12.

@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Review

Clean, well-motivated fix. The root cause (unnest-array bounds becoming join columns the planner can't estimate) is documented clearly, and the new tests/unit/postgres_resolver.rs test actually validates the fix via auto_explain plan output rather than just checking row counts. Good test design, and it follows the repo's documented #[path]-shim convention for moving tests out of src/ (per CLAUDE.md).

Went through the diff in detail (schema hoist, clear_ranges, the new test) and didn't find correctness bugs:

  • Concurrency on Transaction: try_join_all firing concurrent execute() calls on the same &tokio_postgres::Transaction is legitimate pipelining (client-side multiplexing over one connection with server-side FIFO response ordering), not a race. Since the transaction's raw tokio_postgres::Transaction isn't wrapped by deadpool's statement cache, each call really is parsed fresh, matching the comment's claim.
  • Semantics preserved: clear_ranges deletes the same union of key ranges as the old single unnest-based statement. Empty ranges still short-circuits to zero queries.
  • Error path: if one range delete fails, try_join_all bails and the whole batch transaction aborts/rolls back. Same failure semantics as before (the old code also aborted the transaction on any error in the single statement).

Two minor, non-blocking observations:

  1. clear_ranges fans out one concurrent statement per range with no cap. range_deletes is built from up to DRAIN_BATCH_SIZE (256) jobs' operations, so a batch with many range-clearing transactions could produce a large number of concurrent in-flight statements on the single leader connection. Probably fine in practice since each is still cheap and pipelined, but worth a quick sanity check on whether there's an effective upper bound on ranges-per-batch, since this trades one big statement for many small ones plus a full parse each time.
  2. Dropped futures from the short-circuited try_join_all leave their already-sent requests in-flight on the wire. Harmless here since the transaction is doomed anyway, but might be worth a one-line comment noting that's intentional rather than an oversight, for the next reader.

No security concerns (all bound params, no string interpolation into SQL), and no issues with metrics/logging conventions.

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant