sanjeet006py opened a new pull request, #2574:
URL: https://github.com/apache/phoenix/pull/2574
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines:
https://phoenix.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]
PHOENIX-XXXX Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes, and
prefix it with the JIRA id, e.g. 'PHOENIX-XXXX Your PR title ...'.
6. If possible, provide a concise example to reproduce the issue for a
faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section
is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster
reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class
hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other
DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
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:
1. Client side — ScanUtil.annotateMutationWithLiteralTTL (called from
MutationState) threads onto each mutation exactly what the client
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.
2. Server side — new ServerScanUtil (package
org.apache.phoenix.coprocessor, module phoenix-core-server) reproduces the
client read path
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.
3. Anchor the masking clock at batchTimestamp — batchTimestamp is computed
before the current-row read, and the internal scan sets
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.
4. Companion correctness fix — moving getBatchTimestamp earlier means a
registered set could be structurally mutated outside its
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?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
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?
<!--
Note that it means *any* user-facing change including all aspects such as
new features, bug fixes, or other behavior changes. Documentation-only updates
are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes
- provide the console output, description and/or an example to show the
behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to
the released Phoenix versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Yes — a bug fix (behavior change) relative to master and released versions,
for tables/views with a literal TTL and a secondary index.
- Before: on a partial-touch upsert near/after the TTL boundary, the
secondary index could return a value that the data table no longer
returned (index resurrected a logically-expired value); data and index
disagreed.
- After: the internal current-row read is TTL-masked identically to a
client read and anchored at batchTimestamp, so the index is
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?
<!--
If tests were added, say they were added here. Please make sure to add some
test cases that check the changes thoroughly including negative and positive
cases if possible.
If it was tested in a way different from regular unit tests, please clarify
how you tested step by step, ideally copy and paste-able, so that other
reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why
it was difficult to add.
-->
New integration test IndexDataTableSyncIT (parameterized over column-encoded
on/off), covering:
- base-table covered-column re-sync;
- uncovered-index indexed-column re-sync;
- view covered-column re-sync;
- non-strict table is not masked;
- no-index atomic (ON DUPLICATE KEY) upsert masks an expired row;
- UPSERT ... SELECT and UPSERT ... VALUES touches near the TTL boundary
keep data and index consistent;
- an immutable index with a differing storage scheme;
- a concurrency regression
(testConcurrentMajorCompactionDuringIndexWriteKeepsDataAndIndexConsistent)
where a data-table major
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?
<!--
If generative AI tooling has been used in the process of authoring this
patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling
Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
Generated-by: Claude Code (Anthropic Claude Opus 4.8)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
