[#33444] DocDB: Expose the unique-index verification scan as a tserver admin RPC - #33602
Open
jmeehan16 wants to merge 1 commit into
Conversation
✅ Deploy Preview for infallible-bardeen-164bc9 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
jmeehan16
marked this pull request as ready for review
August 31, 2026 15:18
…r admin RPC Wires the verifier core to the world: VerifyUniqueIndexTablet on the tablet server admin service, read-only and paginated, with the preconditions the scan's correctness rests on enforced at the entry point. No production caller yet -- the verification coordinator drives it in a later part; end-to-end tests drive it directly. The RPC (leader-only, BackfillIndex-style pagination): - Discharges the applied-intents precondition in two steps: waits for the tablet's safe time to pass verify_upper_ht (leader lease held; no future write can land at or below the bound), then TransactionParticipant::ResolveIntents(verify_upper_ht) -- safe time alone does not imply applied, since transactional apply is asynchronous, and a committed-but-unapplied foreground write is invisible to a regular-DB-only scan (the record missed could be exactly the foreground half of a duplicate). - Validates the active ordering generation against the request's expectation (index table, and base_op_index when set -- the coordinator always sets it; omitting it relaxes to "any active generation for this table" for test tooling). A released or replaced generation fails the RPC without scanning: the window no longer describes the tablet's marked writes. - Refuses a window that begins below the *applied* history cutoff, read from the regular DB's flushed frontier -- the cutoff past compactions actually used. Deliberately not the retention policy's directive: GetRetentionDirective mutates committed cutoff state (unacceptable on a read path) and only bounds future compactions, which the retention-hold part of this feature prevents from advancing anyway. Until that hold lands the pre-scan check is advisory, so the applied cutoff is re-checked after the scan: a compaction advancing it mid-scan fails the call rather than returning a result computed over possibly-incomplete history. Deterministic failures (generation mismatch, non-unique-index tablet, invalid or inverted window) return typed error codes, so callers do not burn a retry budget on requests that cannot succeed. The scan holds the RocksDB shutdown guard for its duration (the not-blocking variant: tablet shutdown aborts the scan, never the reverse), and an unset backfill_read_ht is rejected at the boundary -- it decodes to the invalid sentinel, which compares greater than any real hybrid time and would make the history-cutoff checks pass vacuously. Tablet::VerifyUniqueIndex resolves the scan options from the tablet: the primary table's ybidxbasectid column, the tablet metadata as the schema-packing provider, the regular DB within the tablet's key bounds, and the new runtime flag unique_index_verify_max_buffered_versions_per_group. TEST_block_index_backfill_ordering_generation_release (master) holds the terminal funnel's release open so tests can verify an index built by the real backfill path while its generation is still active. The e2e suite closes the encoding-drift loop the verifier-core review required: every physical shape asserted by the unit tests is re-asserted against tablets written by the production SKIP_ALL backfill and live pg DML. Test Plan: ./yb_build.sh release --cxx-test pg_index_backfill-test --gtest_filter 'PgIndexBackfillVerifier.CleanBuildWithForegroundDml/0' ./yb_build.sh release --cxx-test pg_index_backfill-test --gtest_filter 'PgIndexBackfillVerifier.PkUpdateAcrossBackfillBoundaryIsClean/0' ./yb_build.sh release --cxx-test pg_index_backfill-test --gtest_filter 'PgIndexBackfillVerifier.DetectsPreexistingDuplicates/0' ./yb_build.sh release --cxx-test pg_index_backfill-test --gtest_filter 'PgIndexBackfillVerifierPackedUpdate.PkUpdateAcrossBackfillBoundaryIsClean/0' ./yb_build.sh release --cxx-test pg_index_backfill-test --gtest_filter 'PgIndexBackfillVerifierReleased.VerifyAfterReleaseFailsCleanly/0' The PK-update regression runs under both physical shapes (standalone ybidxbasectid column with default flags; packed V2 + kIsUpdateFlag with pack_full_row_update + mark) and must be Clean in both. Regressions: unique_index_verifier-test (18), OrderingGenerationActivatedAndReleased (release still fires with the TEST flag off), OrderingGenerationChangeMetadataOp tablet_peer test -- all green. Assisted-By: devx/08801cf6-2854-4c6f-a378-1d98d239b22d --- _automated · Claude Fable 5 (opencode)_
jmeehan16
force-pushed
the
feature-stack/Shopify/uniq-idx-4b-verifier-rpc
branch
from
August 31, 2026 18:36
15a71ee to
68db79a
Compare
This was referenced Aug 31, 2026
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
2 participants
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.
Summary
Wires the verifier core (#33444) to the world:
VerifyUniqueIndexTableton the tablet serveradmin service, read-only and paginated, with the preconditions the scan's correctness rests
on enforced at the entry point. No production caller yet -- the verification coordinator
drives it in the next part; end-to-end tests drive it directly.
The RPC (leader-only, BackfillIndex-style pagination):
to pass
verify_upper_ht(leader lease held; no future write can land at or below thebound), then
TransactionParticipant::ResolveIntents(verify_upper_ht)-- safe time alonedoes not imply applied, since transactional apply is asynchronous, and a
committed-but-unapplied foreground write is invisible to a regular-DB-only scan (the record
missed could be exactly the foreground half of a duplicate).
and
base_op_indexwhen set -- the coordinator always sets the table; omitting the baserelaxes to "any active generation for this index table", which is also what the coordinator
wants: bases are per-tablet activation-op Raft indexes). A released or replaced generation
fails the RPC without scanning: the window no longer describes the tablet's marked writes.
flushed frontier -- the cutoff past compactions actually used. Deliberately not the
retention policy's directive:
GetRetentionDirectivemutates committed cutoff state(unacceptable on a read path) and only bounds future compactions, which the retention-hold
part of this feature prevents from advancing anyway. Until that hold lands the pre-scan
check is advisory, so the applied cutoff is re-checked after the scan: a compaction
advancing it mid-scan fails the call rather than returning a result computed over
possibly-incomplete history.
looks: an unset
backfill_read_htdecodes to the invalid sentinel, which comparesgreater than any real hybrid time -- both history-cutoff checks would pass vacuously and
an invalid scan lower bound would reach the verifier.
tablet, invalid window, cutoff violations) so callers do not burn retry budgets on requests
that cannot succeed.
shutdown aborts the scan, never the reverse).
Tablet::VerifyUniqueIndexresolves the scan options from the tablet: the primary table'sybidxbasectid column, the tablet metadata as the schema-packing provider, the regular DB
within the tablet's key bounds, and the runtime flag
unique_index_verify_max_buffered_versions_per_group.Two TEST flags support the stack's end-to-end tests:
TEST_block_index_backfill_ordering_generation_release(master) holds the terminal funnel'srelease open so tests can verify an index built by the real backfill path while its
generation is still active, and
TEST_pause_verify_unique_index_tablet_rpc(tserver) rejectsverification RPCs with a retryable error to hold a phase open (used by the coordinator part's
failover test).
GenerationMismatchFailsCleanlyexercises the generation-reference validation directlythrough the RPC (wrong index table, wrong base), via a request-mutation hook on the test
helper.
The e2e suite closes the encoding-drift loop the verifier-core review required: every
physical shape asserted by the unit tests is re-asserted against tablets written by the
production SKIP_ALL backfill and live pg DML, including the design document's named
PK-update-across-the-backfill-boundary regression under both physical shapes.
Stacked on #33403, #33404, #33484, #33544, #33553, #33580, #33584, and #33601,
based on
feature-stack/Shopify/uniq-idx-4a-verifier-coreper the feature-stack workflow, sothe diff is exactly this PR's own commit. A failing pr-title check on stacked PRs is a known
gap in that check; the stack merges bottom-up and retargets as parents merge.
Upgrade/Rollback safety
One wire surface changes, additively:
tserver_admin.protogains theVerifyUniqueIndexTabletRPC and its request/response messages (fresh messages; no existingfield numbers touched). A new master calling an old tserver would get a clean unknown-RPC
error -- but no production caller exists until the coordinator part, and that caller is
flag-gated and fails open (records INCONCLUSIVE), so mixed-version clusters cannot be
affected. Rollback is trivial: the RPC is read-only, persists nothing, and nothing references
it. As with the rest of the stack: ships in the same release as #33553 through activation,
under the same one-release constraint.
Test plan
macOS arm64, release -- all green:
PgIndexBackfillVerifier.CleanBuildWithForegroundDml/0-- funnel build (4-tabletsource, INCLUDE column) plus post-build inserts, delete + re-insert of the same indexed
value from a different base row, and an INCLUDE-only update: CLEAN, groups scanned > 0,
no resume key.
PgIndexBackfillVerifier.PkUpdateAcrossBackfillBoundaryIsClean/0-- the designdocument's named regression, standalone-ybidxbasectid shape (default flags): CLEAN.
PgIndexBackfillVerifierPackedUpdate.PkUpdateAcrossBackfillBoundaryIsClean/0--the same regression in the packed shape (packed_row + pack_full_row_update + mark):
CLEAN.
PgIndexBackfillVerifier.DetectsPreexistingDuplicates/0-- duplicates thatpre-existed the SKIP_ALL build (which therefore succeeded): VIOLATION with a value-free
reason.
PgIndexBackfillVerifier.GenerationMismatchFailsCleanly/0-- wrong index table idand wrong generation base each fail without scanning.
PgIndexBackfillVerifierReleased.VerifyAfterReleaseFailsCleanly/0-- with thefunnel release NOT held open, a late verification RPC fails cleanly on the released
generation.
unique_index_verifier-test(18), the skip_all e2e suite through the realactivation flow,
OrderingGenerationChangeMetadataOptablet_peer test.AlmaLinux (x86_64, clang21) -- all green, re-validated after the stack-wide rework:
At this PR's own commit (
68db79ae25):PK-update-across-backfill-boundary cases in both physical shapes, the
pre-existing-duplicates positive control, and
GenerationMismatchFailsCleanly(whichthe rework moved into this layer).
unique_index_verifier-test,tablet_peer-test.Integrated at the stack tip (
c69686cd0d, which contains this commit):tablet_peer-test,unique_index_verifier-test,fixed_hybrid_time_write_id-itest.pg_index_backfill-test.The only debug-build
pg_index_backfill-testfailure in the re-validation isPgIndexBackfillBackendsManager.NoAbortTxn/1(GetSafeTime60s timeout) -- the pre-existing upstream flake that fails 3/3 at a base commit carrying none of this stack's
code (upstream hardening lineage [YSQL] test: harden YSQL backends manager tests #19351).