Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts - #2574
Merged
sanjeet006py merged 20 commits intoAug 18, 2026
Conversation
added 12 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).
Remove verbose inline/Javadoc comments across the internal-scan masking path (ScanUtil.annotateMutationWithLiteralTTL, ServerScanUtil, IndexRegionObserver.getCurrentRowStates and extractLiteralTTLForInternalScan) that duplicated logic now evident from the code, keeping only the concise batchTimestamp-anchor notes at the two setTimeRange call sites. In IndexDataTableSyncIT, fix the phase-2 comment in the immutable differing-storage-scheme test: with autoCommit off the UPSERT VALUES only buffers on the client, so the masked current-row read and the mutation timestamp are both established at commit. Collapse the misleading two-step clock advance into a single increment that lands the commit cleanly past covcol's TTL boundary. Also drop the now-redundant Javadoc on the concurrent-compaction test and align its injected timings.
The client previously piggy-backed a view's literal TTL for the server-side internal current-row scan on the _TTL mutation attribute, which the server otherwise reads as conditional TTL. Disambiguating one overloaded attribute by deserialized type forced the server to strip _TTL from every mutation before identifyMutationTypes, and left a rolling-upgrade hazard: an old RegionServer that predates the disambiguation code would cast a literal-TTL view's _TTL bytes to CompiledConditionalTTLExpression and fail the upsert on the write path. Introduce a dedicated _LITERAL_TTL attribute so _TTL on a mutation is unambiguously conditional TTL again: - ScanUtil.annotateMutationWithLiteralTTL threads the view's literal TTL on _LITERAL_TTL instead of _TTL. - IndexRegionObserver.extractLiteralTTLForInternalScan reads _LITERAL_TTL directly; the type-disambiguation check and the _TTL-strip loop are gone, and updateMutationsForConditionalTTL's cast reverts to the direct cast (no overloaded attribute to guard against). - An old RegionServer ignores the unknown _LITERAL_TTL attribute rather than mis-parsing it, turning the crash into a benign no-op and relaxing the RS-before-client upgrade requirement to a soft recommendation. Also register HashSet (not TreeSet) snapshots of rowsToLock in getBatchTimestamp's batchesWithLastTimestamp: shouldSleep only calls contains(), so no ordering is needed and the snapshot is marginally cheaper.
sanjeet006py
commented
Jul 27, 2026
ContributorAuthor
The internal current-row read in getCurrentRowStates opened its scan with setTimeRange(0, batchTimestamp). HBase scan time ranges are half-open [min, max), so this excluded any committed cell sitting at exactly batchTimestamp. TTLRegionScanner then derived its masking clock from getTimeRange().getMax(), which is why the range max was pinned at batchTimestamp. In production a same-row batch cannot land on an already-committed cell at batchTimestamp: getBatchTimestamp forces a distinct timestamp via shouldSleep + Thread.sleep(1) while holding the row locks. Under a frozen test clock (ConcurrentMutationsIT.MyClock.setAdvance(false)) that separation is defeated, so an UPSERT and a DELETE on the same row share batchTimestamp; the DELETE's read then excluded the UPSERT cells, rebuilt a stale current-row state, and left a dangling index entry (testDeleteRowAndUpsertValueAtSameTS2). Use setTimeRange(0, batchTimestamp + 1) at both read sites (bloom-filter get path and skip-scan path) so the boundary cell is included, mirroring CompactionScanner's setTimeRange(0, compactionTime + 1). The masking clock becomes batchTimestamp + 1, a 1ms forward nudge toward the live client read clock that over-masks at most a single measure-zero boundary cell. Verified: ConcurrentMutationsIT#testDeleteRowAndUpsertValueAtSameTS2 passes and IndexDataTableSyncIT (strict-TTL-boundary suite) stays green 18/18.
tkhurana
reviewed
Aug 3, 2026
| // no-op unless table/view has a literal TTL; threads the empty-column CF/CQ (plus a view's | ||
| // literal TTL and any non-strict flag) so the internal current-row scan masks like a client | ||
| // read | ||
| ScanUtil.annotateMutationWithLiteralTTL(connection, tableInfo.getPTable(), mutationList); |
Contributor
There was a problem hiding this comment.
Did you verify that conditional ttl doesn't have this problem ? I didn't see any test cases with conditional ttl
ContributorAuthor
There was a problem hiding this comment.
I observed that conditional TTL case was already covered by IndexRegionObserver#updateMutationsForConditionalTTL. There we check if a row is already expired and add delete markers.
I didn't explicitly verify via IT. Do you think I should add one?
Contributor
There was a problem hiding this comment.
Its ok. There are already existing tests.
tkhurana
reviewed
Aug 3, 2026
| ) { | ||
| getCurrentRowStates(c, context); | ||
| getCurrentRowStates(c, context, context.literalTTLForInternalScan, | ||
| isStrictTTLEnabled(miniBatchOp), batchTimestamp); |
Contributor
There was a problem hiding this comment.
We can save is strict ttl in also in the context. You are already saving ttl and empty column family and qualifier. That will make the API cleaner.
tkhurana
reviewed
Aug 3, 2026
Comment on lines
+1301
to
+1307
| // With server paging wired in (ServerScanUtil.setInternalScanAttributes), | ||
| // PagingRegionScanner | ||
| // returns a dummy result when a page is paged out; skip it and let the loop resume rather | ||
| // than build a Put from the dummy cell. | ||
| if (ScanUtil.isDummy(cells)) { | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
I am not sure what are we gaining with paging here ?
ContributorAuthor
There was a problem hiding this comment.
There is no explicit functional gain rather this is scan path and my understanding is we want to cover all scan paths via paging to ensure fair utilization of server resources/handler threads. Thus, added paging explicitly. This way internal scan done by IndexRegionObserver by directly opening region scanner won't bypass paging.
ContributorAuthor
There was a problem hiding this comment.
Oh, I didn't realize that if its not going through RPC path then it won't even consume additional handler threads. Yeah, there is no benefit of paging in here. Thanks, will remove it.
tkhurana
reviewed
Aug 3, 2026
Comment on lines
+85
to
+120
| * Reproduces the client read path's server-paging setup for an internal scan. On the client the | ||
| * {@code SERVER_PAGE_SIZE_MS} attribute is set by | ||
| * {@code ScanUtil.setScanAttributeForPaging(Scan, PhoenixConnection)} and the scan filter is | ||
| * later wrapped in a {@link PagingFilter} by {@code BaseScannerRegionObserver.preScannerOpen}. | ||
| * Internal scans opened directly via {@code region.getScanner(scan)} bypass both, so this method | ||
| * performs both steps up-front. The region-server {@link Configuration} is the source of the | ||
| * paging props here, standing in for the client's {@code PhoenixConnection} props. | ||
| * <p> | ||
| * Ordering matters: {@code PagingRegionScanner}'s constructor reads the {@link PagingFilter} and | ||
| * the page size off the scan, so this must run before | ||
| * {@link #openRegionScanner(RegionCoprocessorEnvironment, Region, Scan)} builds the scanner. | ||
| */ | ||
| public static void setInternalScanAttributesForPaging(Configuration conf, Scan scan) { | ||
| if ( | ||
| !conf.getBoolean(QueryServices.PHOENIX_SERVER_PAGING_ENABLED_ATTRIB, | ||
| QueryServicesOptions.DEFAULT_PHOENIX_SERVER_PAGING_ENABLED) | ||
| ) { | ||
| return; | ||
| } | ||
| long pageSizeMs = conf.getInt(QueryServices.PHOENIX_SERVER_PAGE_SIZE_MS, -1); | ||
| if (pageSizeMs == -1) { | ||
| // Use half of the HBase RPC timeout value as the server page size, mirroring the client | ||
| // ScanUtil.setScanAttributeForPaging fallback. | ||
| pageSizeMs = | ||
| (long) (conf.getLong(HConstants.HBASE_RPC_TIMEOUT_KEY, HConstants.DEFAULT_HBASE_RPC_TIMEOUT) | ||
| * 0.5); | ||
| } | ||
| scan.setAttribute(BaseScannerRegionObserverConstants.SERVER_PAGE_SIZE_MS, | ||
| Bytes.toBytes(Long.valueOf(pageSizeMs))); | ||
| // Wrap the scan filter in a PagingFilter as the top-level filter, matching | ||
| // BaseScannerRegionObserver.preScannerOpen. PagingRegionScanner then detects when PagingFilter | ||
| // has paged the scan out and returns a dummy result; readDataTableRows skips those dummies. | ||
| if (!(scan.getFilter() instanceof PagingFilter)) { | ||
| scan.setFilter(new PagingFilter(scan.getFilter(), ScanUtil.getPageSizeMsForFilter(scan))); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This whole code has been duplicated from the client but there seems to be little benefit since we are doing a region local scan which doesn't go through the rpc path so what is paging benefit we are trying to achieve ?
ContributorAuthor
There was a problem hiding this comment.
Thanks for highlighting this explicitly that no handler threads will be involved here.
tkhurana
requested changes
Aug 3, 2026
tkhurana
reviewed
Aug 4, 2026
| Scan scan) throws IOException { | ||
| return new TTLRegionScanner(env, scan, | ||
| new PagingRegionScanner(region, region.getScanner(scan), scan)); | ||
| return new TTLRegionScanner(env, scan, region.getScanner(scan)); |
Contributor
There was a problem hiding this comment.
It might not be safe in all cases to directly pass the hbase regionscanner as the delegate to TTLRegionScanner. There are code paths where we cast to DelegateRegionScanner.
| public static final String SKIP_REGION_BOUNDARY_CHECK = "_SKIP_REGION_BOUNDARY_CHECK"; | ||
| public static final String TX_SCN = "_TxScn"; | ||
| public static final String TTL = "_TTL"; | ||
| // Literal TTL threaded per-mutation for the server-side internal current-row scan |
Contributor
There was a problem hiding this comment.
I think it is better to use the same TTL attribute for both conditional and literal ttl. We already do that on the scan path. There is no reason we can't do that on the mutation path also.
ContributorAuthor
There was a problem hiding this comment.
I intentionally used a new mutation attribute for literal TTL as currently in updateMutationsForConditionalTTL there is a blind cast of TTL expression to Conditional TTL expression and moreover, in IndexRegionObserver there is implicit assumption that if _TTL attribute is set on a mutation then its conditional TTL. If client changes in this PR gets deployed earlier then server changes, then that can break mutation path by throwing ClassCastException.
Contributor
There was a problem hiding this comment.
The current check was done because it was a cheap way to do it but the server can deserialize the ttl expression and can easily determine if the expression is a literal ttl or conditional ttl. If the server is upgraded first which is typically the case it will remain backward compatible to any client using conditional ttl. I am not sure if we really need to worry about the case of the client getting deployed before server. That can cause all sorts of other issues.
ContributorAuthor
There was a problem hiding this comment.
Had offline discussion, will unify the attributes. Thanks
ContributorAuthor
There was a problem hiding this comment.
@tkhurana I have addressed the suggestion. PTAL
| CompiledTTLExpression ttlExpr = TTLExpressionFactory.create(ttlBytes); | ||
| context.ttlExpressionForBatch = ttlExpr; | ||
| if (context.isLiteralTTL()) { | ||
| context.literalTTLForInternalScan = ttlBytes; |
Contributor
There was a problem hiding this comment.
Do we need a separate field for literal ttl ?
ContributorAuthor
There was a problem hiding this comment.
Ideally no. But instead of reserializiing TTL Expression (which we have already extracted) for literal TTL, I thought why not store that also as bytes. Literal TTL in bytes form doesn't add much overhead.
Contributor
There was a problem hiding this comment.
If you want to store raw bytes I would just store the raw ttl bytes all the time and rename the variable accordingly.
| BatchMutateContext context) throws IOException { | ||
| // If TTL is not strict, skip conditional TTL processing | ||
| if (!context.isStrictTTLForInternalScan) { | ||
| if (!context.isConditionalTTL()) { |
Contributor
There was a problem hiding this comment.
Are we mixing the two concepts here conditional ttl and literal ttl and strict vs non-strict ttl. These are independent properties and all 4 combinations are possible and permitted.
ContributorAuthor
There was a problem hiding this comment.
The invocation of method updateMutationsForConditionalTTL is gated by pre-existing hasConditionalTTL in preBatchMutateWithExceptions. hasConditionalTTL is true when mutation batch has conditionalTTL along with strict TTL being true. Given conditional TTL and strict TTL are two independent properties then gating by the caller of the method makes sense so, I removed the check from method itself.
But separately added a check for conditionalTTL explicitly as this method should never be called when its not conditional TTL. Its more of defense in depth check as whole method is relevant for conditional TTL only.
So, I am not mixing two concepts.
ContributorAuthor
There was a problem hiding this comment.
If you think this can cause confusion then I can add the check for strictTTL back. This was a nit change so, I am fine either way.
Contributor
There was a problem hiding this comment.
There is confusion. We have hasConditionalTTL which is true when when mutation batch has conditionalTTL along with strict TTL being true. That is confusing naming wise. We also have context.hasConditionalTTL and context.isConditionalTTL.
tkhurana
approved these changes
Aug 17, 2026
tkhurana
commented
Aug 17, 2026
Contributor
@sanjeet006py Thanks for making the changes. This looks neat and clean now. |
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 freeto 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.
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)