compute: fix shard misrouting for cross-stripe multi-block prefetch batches - #12931
Open
reatcat wants to merge 1 commit into
Open
compute: fix shard misrouting for cross-stripe multi-block prefetch batches#12931reatcat wants to merge 1 commit into
reatcat wants to merge 1 commit into
Conversation
…atches In prefetch_register_bufferv(), compute the shard number from the per-block tag (hashkey.buftag) instead of the batch's base tag, which is never advanced in the registration loop.
reatcat
requested review from
HaoyuHuang,
hlinnaka and
problame
and
a lite review from Copilot
August 11, 2026 05:02
There was a problem hiding this comment.
Pull request overview
Fixes shard misrouting for vectorized multi-block reads when a single batch crosses a shard stripe boundary by computing the shard number from the per-block BufferTag rather than the base tag. This prevents misrouted pagestream requests (and resulting reconnect/discard churn) during sequential scans on sharded tenants.
Changes:
- Compute
slot->shard_nofromhashkey.buftag(per-block tag) insideprefetch_register_bufferv()’s multi-block loop. - Add a PostgreSQL 17 regression test that reliably triggers cross-stripe multi-block batches and asserts no misrouted pagestream requests.
- Add a control test (
io_combine_limit=1) to demonstrate the failure mode is specific to multi-block batches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pgxn/neon/communicator.c | Fixes shard computation to use the per-block tag so multi-block batches crossing stripes route to the correct pageserver shard. |
| test_runner/regress/test_prefetch_register_bufferv_wrong_shard.py | Adds a targeted PG17 regression + control test validating the fix by asserting misroute metrics remain zero under a forced cross-stripe workload. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #12932
Problem
In
prefetch_register_bufferv(), when registering a multi-block batch, the shard number of every block is computed from the base block's tag (get_shard_number(&tag)) instead of the per-block tag (hashkey.buftag, whoseblockNumhas already been advanced byi). Sinceget_shard_number()hashesblockNum / stripe_size, whenever a multi-block request batch crosses a shard stripe boundary, the blocks beyond the boundary get the shard number of the base block and are sent to the wrong pageserver shard connection.The receiving pageserver counts it in
pageserver_misrouted_pagestream_requests_totaland drops the connection without a response (PageStreamError::Reconnect). The compute discards all in-flight prefetches (getpage_prefetch_discards_total) and retries one block at a time, which computes the shard correctly — so the query self-heals, but every crossing batch pays a connection teardown/reconnect and a wasted round trip.Summary of changes
pgxn/neon/communicator.c: compute the shard from&hashkey.buftag(the per-block tag) instead of&tag, with a comment explaining why.test_runner/regress/test_prefetch_register_bufferv_wrong_shard.py(PG 17 only, where the read-stream based multi-block path exists):ShardSelector::Page(key)would silently pick the correct local shard and hide the bug.shared_buffers=128MBso the read stream builds 16-block batches), and asserts the result is correct and the pageserver misroute counter stays at 0.io_combine_limit=1so every batch is a single block: misrouting is impossible even with buggy code, proving the main test exercises exactly the multi-block cross-stripe path.Expected behavior:
pageserver_misrouted_pagestream_requests_totalandgetpage_prefetch_discards_totalboth grow (roughly one event per crossed boundary), and the main test fails on themisroutes == 0assertion.