PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts#2574
Open
sanjeet006py wants to merge 9 commits into
Open
PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts#2574sanjeet006py wants to merge 9 commits into
sanjeet006py wants to merge 9 commits into
Conversation
added 9 commits
July 5, 2026 02:52
…nc in IndexRegionObserver Make server-side secondary-index maintenance honor Phoenix TTL exactly like a client read, and re-persist index-referenced data columns so a data table and its global index are retained or expired together under compaction. Two distinct defects caused a global index to keep a value the data table no longer had after TTL expiry: - The internal current-row scan in IndexRegionObserver.preBatchMutate opened via region.getScanner bypassed postScannerOpen, so it was never wrapped in TTLRegionScanner and read TTL-expired-but-present cells, rebuilding the index from logically-expired data. - A partial "touch" upsert that omitted an index-referenced column left the data-side cell at its original timestamp while the index cell was rebuilt at batchTimestamp; a later major compaction opened a >ttl gap on only the data side, dropping the value there while the index kept it. Changes: - New ServerScanUtil (setInternalScanAttributes, openRegionScanner) that sets the empty-column / TTL / strictness scan attributes and opens a TTLRegionScanner-wrapped scanner mirroring postScannerOpen. - New ScanUtil.annotateMutationWithLiteralTTL, called from MutationState.sendMutations, threading a view's literal TTL and non-strict flag to the server per mutation. - IndexRegionObserver: extract/remove a literal _TTL mutation attribute before identifyMutationTypes so it is never misread as conditional TTL; wire the current-row scan through setInternalScanAttributes + openRegionScanner; and re-inject index-referenced non-PK columns into the touch's data Put at LATEST_TIMESTAMP so setTimestamps re-stamps them to batchTimestamp. - New phoenix.index.ttl.column.resync.enabled flag (default true) gating the column re-sync as an operator kill switch. - New IndexDataTableTTLSyncIT and IndexDataTableTTLSyncFlagOffIT covering base-table, view, non-strict, and flag-off cases.
…ow reads Set the empty-column CF/CQ mutation attributes unconditionally in ScanUtil.annotateMutationWithLiteralTTL for any mutable literal-TTL table/view, mirroring the client read path (setScanAttributesForClient), which sets the empty column on every non-analyze scan. They only identify the table's empty column and enable masking; TTLRegionScanner still independently requires an effective, non-FOREVER, strict TTL to actually mask, so setting them whenever a current-row read may happen makes the internal scan mask identically to a client read rather than diverging. Server side, getCurrentRowStates now falls back to these client-threaded CF/CQ bytes when no IndexMaintainer is available, so the no-index current-row read (atomic / ON DUPLICATE KEY / returnResult / row-delete on a TTL table) is masked and an expired row is treated as absent instead of resurrected. extractLiteralTTLForInternalScan captures the CF/CQ off the representative mutation regardless of the _TTL attribute and leaves them on the mutations (inert on the write path). The internal scan is also given the client read path's server-paging setup (SERVER_PAGE_SIZE_MS plus a PagingFilter wrap) via ServerScanUtil.setInternalScanAttributesForPaging, and readDataTableRows skips the dummy results paging can emit. Extend the index-referenced column re-sync to uncovered global indexes: an uncovered index encodes its indexed column positionally into the index key rebuilt at batchTimestamp, so the indexed column must be re-persisted on the data side too, else a live row silently drops out of an indexed-column predicate after compaction trims the stale data-side cell. Add ITs: testNoIndexAtomicUpsertMasksExpiredRow (no-index masking), testUncoveredIndexIndexedColumnResync, and a flag-off uncovered counterpart.
The client now threads the empty-column CF/CQ onto every mutation unconditionally (ScanUtil.annotateMutationWithLiteralTTL, matching setScanAttributesForClient), and the batch context captures them. That is the single source for every path reaching getCurrentRowStates -- the secondary-index case and the no-index atomic / ON DUPLICATE KEY / returnResult / row-delete case alike -- so the maintainer branch is redundant. Remove getDataTableMaintainerForInternalScan and the IndexMaintainer parameter from getCurrentRowStates; resolve emptyCF/CQ solely from the client-threaded batch-context bytes. This ties internal-scan masking to the same client signal that governs client read masking, keeping the two consistent.
…ath; drop config gate The plain-upsert re-sync (rewriteIndexReferencedColumns) deliberately excludes atomic / ON DUPLICATE KEY / returnResult Puts because generateOnDupMutations reconstructs them from conditional expressions rather than writing them verbatim. That left the atomic path re-creating the covered-column timestamp-skew divergence: a touch that omits an index-referenced column leaves the data-side cell at its original timestamp while the index side is rebuilt at batchTimestamp, so a later major compaction drops the value only on the data table. Add the equivalent re-sync inline in generateOnDupMutations: - Snapshot the pre-upsert row into a local oldRowColumnCellExprMap (only exposed on the context under the existing OLD_ROW condition, unchanged semantics). - rewriteIndexReferencedColumnsForAtomicPut injects every index-referenced non-PK column the upsert omitted, from that snapshot, at LATEST_TIMESTAMP so setTimestamps re-stamps it to batchTimestamp alongside the rest of the Put. - Inject at the two upsert-reconstruction points: the opBytes == null (plain upsert with returnResult) branch into atomicPut, and after the checkCellNeedUpdate loop into the reconstructed put, guarded by !put.isEmpty() so a genuine no-op ON DUPLICATE KEY UPDATE is not turned into a spurious write. - Share the index-referenced column-set computation via getIndexReferencedColumns. Remove the phoenix.index.ttl.column.resync.enabled config gate entirely: the re-sync now always applies (constants, field, start() init, and both guard sites deleted). Delete IndexDataTableTTLSyncFlagOffIT (it only asserted the kill switch) and fix its now-dangling javadoc references in IndexDataTableTTLSyncIT.
…ncedColumns The previous commit added a separate rewriteIndexReferencedColumnsForAtomicPut on the ON DUPLICATE KEY path because the general re-sync excluded atomic / returnResult Puts. Relocating and re-sourcing the general re-sync makes that separate path redundant, so remove it and let a single method cover every Put. Relocate and re-source rewriteIndexReferencedColumns: - Move the call to after prepareDataRowStates (was before setTimestamps) and source the re-persisted value from the merged next row state dataRowStates.getSecond() instead of the raw current row getFirst(). - This fixes a NULL-set resurrection bug: a column the touch sets to NULL is emitted as a separate Delete, not a Put cell, so getFirst() still held the old value and the previous logic (guarded only on put.has) resurrected it. After prepareDataRowStates the merged next state has the Delete applied, so a NULLed column is absent from getSecond() and correctly not re-persisted. The index is generated from the same getSecond(), so data and index always agree. - Inject injected cells directly at batchTimestamp (setTimestamps has already run), matching the index cell rebuilt at the same timestamp. Extend the same method to the atomic / ON DUPLICATE KEY / returnResult path: - addOnDupMutationsToBatch runs before prepareDataRowStates and merges each reconstructed atomic Put (and its coproc Delete) into the in-batch operation, so by the time rewriteIndexReferencedColumns runs the atomic row's getSecond() is its fully reconstructed next state. Replace the atomic/returnResult exclusion with an isAtomicOperationComplete guard so genuine no-op ON DUPLICATE KEY / IGNORE ops are skipped (nothing written) while non-no-op atomic Puts are re-synced like any Put. - Sourcing from getSecond() rather than the pre-upsert snapshot also fixes the same NULL-set resurrection for ON DUPLICATE KEY UPDATE ... = NULL. - Delete rewriteIndexReferencedColumnsForAtomicPut, its two call sites, the now-dead indexReferencedCols local, and the now-unused PhoenixIndexMetaData parameter threaded through generateOnDupMutations / addOnDupMutationsToBatch. getIndex ReferencedColumns now has a single caller. Thread empty-column CF/CQ unconditionally on the internal current-row scan: - Drop the maskInternalScan gate in getCurrentRowStates and always call ServerScanUtil.setInternalScanAttributes at both scan branches. TTLRegionScanner no-ops masking when the empty-column attributes are absent, so passing null args is safe and keeps the internal scan masking identically to a client read.
… comments Follow-up to the atomic / ON DUPLICATE KEY re-sync consolidation. - Correct the rewriteIndexReferencedColumns Javadoc: it now processes every index-enabled Put (plain upsert or reconstructed atomic / ON DUPLICATE KEY upsert alike), not only non-atomic Puts. The stale "non-atomic" wording contradicted both the method body and its own closing paragraph. - Trim the getIndexReferencedColumns Javadoc: drop the "shared by ... generateOnDup Mutations (atomic path)" note now that the atomic path no longer calls it; the method has a single caller. - Simplify the OLD_ROW snapshot in generateOnDupMutations: build context.oldRowColumnCellExprMap directly under the returnOldRow condition instead of via a throwaway local, now that the local is no longer needed for the removed atomic re-sync. - Remove a stray whitespace-only line before the addEmptyKVCellToPut block.
…undary data/index consistency Adds an integration test that reproduces the RCA divergence against pre-fix code and verifies the fix keeps a data table and its global covered index consistent when a partial touch upsert lands across the TTL expiry boundary. The test asserts data/index agreement on the covered and indexed columns (GREEN when consistent, RED on the pre-fix divergence), major-compacting both tables so gap-analysis trimming is applied physically. Uses unique per-method physical table names so future-dated cells from the injected clock cannot bridge the next method's TTL gap on the shared mini-cluster.
…teIndexReferencedColumns Replace the re-persist fix for the data/index TTL-boundary divergence with a TTL-anchored masked read. On a partial "touch" upsert that does not re-write an index-referenced column, the index side is rebuilt in full at batchTimestamp while the data-side cell keeps its original timestamp; near the TTL boundary the internal current-row read was masked as of the scan-open wall clock, which is slightly earlier than batchTimestamp (getBatchTimestamp bumps it forward), so a cell expiring in that sub-millisecond window was visible to the index build yet let expire on the data side. Fix: compute batchTimestamp right after lockRows (before the current-row read) and set scan.setTimeRange(0, batchTimestamp) on the existing TTLRegionScanner- wrapped internal scan, so TTLRegionScanner anchors its masking clock (currentTime = scan.getTimeRange().getMax()) at exactly the timestamp the index is built at. CompactionScanner converges the data side to the identical keep/drop decision via the same empty-column gap rule, so both sides always agree. This avoids the read-all-versions GC pressure and the per-touch write amplification of the previous approach. - IndexRegionObserver: hoist getBatchTimestamp above the current-row read; thread batchTimestamp into getCurrentRowStates and setTimeRange both scan branches; register a defensive TreeSet copy in batchesWithLastTimestamp (the reorder puts registration before releaseLocksForOnDupIgnoreMutations's unsynchronized remove); delete rewriteIndexReferencedColumns and getIndexReferencedColumns. - ScanUtil.annotateMutationWithLiteralTTL: no longer early-return for immutable tables (the server-side literal-TTL current-row read is driven by table/index structure and mutation type, not the annotated attribute). - IndexDataTableTTLSyncIT: invert the four re-stamp assertions to assert the data cell retains its original timestamp; rewrite Javadocs for the anchored trim. - IndexDataTableTTLBoundaryConsistencyIT: set max-lookback non-zero (below TTL); assert data/index agreement at the query layer.
…-sync ITs Rename IndexDataTableTTLSyncIT to IndexDataTableSyncIT and add testConcurrentMajorCompactionDuringIndexWriteKeepsDataAndIndexConsistent, which parks the index write via a blocking region observer on the index table so a data-table major compaction runs after the current-row read but before the index write completes, then asserts data/index agreement. Remove the redundant IndexDataTableTTLBoundaryConsistencyIT (its boundary coverage is subsumed by IndexDataTableSyncIT). Remaining changes are doc/comment reflow and an unused-import removal in ServerScanUtil; no production logic changes (the fix landed in 339fe94).
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
1 participant
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.
What changes were proposed in this pull request?
This PR makes Phoenix's internal current-row read during secondary-index maintenance TTL-mask exactly like an ordinary client read, so a
data table and its secondary index stay consistent after TTL expiry.
Index maintenance in IndexRegionObserver.preBatchMutateWithExceptions reads the current on-disk row via getCurrentRowStates to rebuild
the correct index entry. That read was opened directly through region.getScanner(scan), which bypasses the postScannerOpen coprocessor
hook — the only place a scan is normally wrapped in TTLRegionScanner. As a result the internal read was not TTL-masked, so on a "partial
touch" upsert (an UPSERT that does not re-write any index-referenced column) the index was rebuilt from
logically-expired-but-not-yet-compacted cells, while the data table correctly expired them on read.
Changes:
read path sets as scan attributes: the empty-column CF/CQ (unconditionally for any literal-TTL table/view), a view's compiled literal
TTL as the _TTL attribute (base tables rely on the CF-descriptor TTL), and IS_STRICT_TTL=false only when the table/view is non-strict.
server-side: setInternalScanAttributes / setInternalScanAttributesForPaging set the same empty-column/TTL/strict/paging attributes, and
openRegionScanner wraps the scan in TTLRegionScanner + PagingRegionScanner exactly as postScannerOpen does. IndexRegionObserver
captures the client-threaded attributes into the batch context (extractLiteralTTLForInternalScan) and passes them into
getCurrentRowStates.
scan.setTimeRange(0, batchTimestamp) so TTLRegionScanner's "current time" equals the exact timestamp at which the index is rebuilt,
closing a sub-millisecond boundary window between the scan-open wall clock and batchTimestamp. The half-open range drops nothing
current, since every pre-existing locked-row cell was written at ts < batchTimestamp.
synchronized block; batchesWithLastTimestamp now registers a defensive TreeSet snapshot to avoid a ConcurrentModificationException,
staying conservative for the sleep/timestamp invariant (at most an extra sleep, never a missed one).
Production files touched: IndexRegionObserver, ScanUtil, new ServerScanUtil, MutationState, and a doc-comment update in MetaDataClient.
Why are the changes needed?
This is a silent data-integrity/correctness bug. On a table or view with a literal TTL and a secondary index, after TTL expiry the
secondary index can return a column value the data table no longer returns — the two diverge with no error raised. The divergence
surfaces after logical TTL expiry and before major compaction physically purges the expired cells.
The root cause is that the internal current-row read bypassed TTLRegionScanner, so index rebuild saw expired cells the data-side read
masks. The affected paths are: secondary indexes (global covered, global uncovered, and immutable indexes whose data/index storage
schemes differ) and the no-index current-row reads on a literal-TTL table (atomic / ON DUPLICATE KEY upserts, returnResult upserts, and
row deletes). Conditional-TTL, non-TTL, and non-strict-TTL tables are unaffected — masking is a no-op there and the read is
byte-identical to before.
Does this PR introduce any user-facing change?
Yes — a bug fix (behavior change) relative to master and released versions, for tables/views with a literal TTL and a secondary index.
returned (index resurrected a logically-expired value); data and index disagreed.
rebuilt from the same masked state the data table exposes, and the two stay consistent.
No API, syntax, or configuration change. No new config flag is introduced.
How was this patch tested?
New integration test IndexDataTableSyncIT (parameterized over column-encoded on/off), covering:
compaction runs during a slow index write — after the current-row read and after the data locks are released, but before the index write
completes — and data/index must remain consistent.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Anthropic Claude Opus 4.8)