Skip to content

fix: allow omnisharded reads with a shard directive inside transactions - #1510

Merged
levkk merged 2 commits into
pgdogdev:mainfrom
rlittlefield:omni-read-with-directive-in-tx
Sep 10, 2026
Merged

fix: allow omnisharded reads with a shard directive inside transactions#1510
levkk merged 2 commits into
pgdogdev:mainfrom
rlittlefield:omni-read-with-directive-in-tx

Conversation

@rlittlefield

@rlittlefield rlittlefield commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

A plain SELECT from an omnisharded table inside a read/write transaction failed with cannot write to an omnisharded table with a shard directive whenever a directive was present, whether /* pgdog_shard */, /* pgdog_sharding_key */, or SET pgdog.shard. The guard used Route::is_write() to decide whether a statement mutates, but under the conservative read/write strategy every statement in a read/write transaction is marked a write so it runs on the primary. Any application that keys its transactions and reads a replicated table inside one hit this on a multi-shard cluster.

Route now carries a statement-intrinsic mutates flag: DML, DDL, data-modifying CTEs, locking clauses, write functions and advisory locks. The omnisharded coverage check uses it; read is unchanged and still drives primary/replica placement. The guard is per statement, so a later omni write in the same pinned transaction is still rejected.

This finishes the fix for #1374. #1384 and #1398 diagnosed the same false positive and relaxed the check when schema sharding is used exclusively, leaving the mixed sharded_schemas + sharded_tables case for later ("the error will still throw ... I also added a test for that for later"). Scoping the exemption by statement kind instead of by config covers that case: a plain read of a pinned shard is correct whether the table is omnisharded or schema-sharded. The "invalid cases" test in integration/rust/tests/integration/cross_shard_omni_check.rs, which asserted the pinned read fails, now checks the read succeeds and that an INSERT into an omnisharded table in the same pinned transaction is still rejected.

Covered by parser and route unit tests plus engine tests against Postgres for reads inside pinned transactions.

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.40000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...frontend/router/parser/query/test/test_sharding.rs 97.22% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@rlittlefield
rlittlefield force-pushed the omni-read-with-directive-in-tx branch from ece8532 to be05e4d Compare September 9, 2026 16:41
@rlittlefield rlittlefield changed the title fix: omnisharded reads with a shard directive in transactions, broadcast omnisharded writes inside CTEs fix: allow omnisharded reads with a shard directive inside transactions Sep 9, 2026
@rlittlefield
rlittlefield force-pushed the omni-read-with-directive-in-tx branch from cd57575 to 3858dc2 Compare September 9, 2026 21:30
@rlittlefield
rlittlefield force-pushed the omni-read-with-directive-in-tx branch from 3858dc2 to 0f66203 Compare September 10, 2026 14:31
@rlittlefield
rlittlefield marked this pull request as ready for review September 10, 2026 14:34

@levkk levkk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Got to love PRs where number of test lines exceeds the actual change by a factor of 10. Thank you sir!

@levkk
levkk merged commit 52b9ac8 into pgdogdev:main Sep 10, 2026
29 checks passed
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.

2 participants