From 6f62f6e4a0d48b5f0fee12b85dcd2c076192b819 Mon Sep 17 00:00:00 2001 From: Sanjeet Malhotra Date: Sun, 5 Jul 2026 02:52:11 +0530 Subject: [PATCH 1/9] TTL-aware internal current-row read and index-referenced column re-sync 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. --- .../apache/phoenix/execute/MutationState.java | 3 + .../org/apache/phoenix/util/ScanUtil.java | 71 ++++ .../phoenix/coprocessor/ServerScanUtil.java | 91 +++++ .../hbase/index/IndexRegionObserver.java | 198 +++++++++- .../index/IndexDataTableTTLSyncFlagOffIT.java | 112 ++++++ .../index/IndexDataTableTTLSyncIT.java | 363 ++++++++++++++++++ 6 files changed, 835 insertions(+), 3 deletions(-) create mode 100644 phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java create mode 100644 phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncFlagOffIT.java create mode 100644 phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java b/phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java index 52517a884df..3b06a664d62 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java @@ -1528,6 +1528,9 @@ private void sendMutations(Iterator>> mutationsI // no-op if table doesn't have Conditional TTL ScanUtil.annotateMutationWithConditionalTTL(connection, tableInfo.getPTable(), mutationList); + // no-op unless table/view has a literal TTL; threads the view's literal TTL and any + // non-strict flag so the server-side internal current-row scan masks like a client read + ScanUtil.annotateMutationWithLiteralTTL(connection, tableInfo.getPTable(), mutationList); // If we haven't retried yet, retry for this case only, as it's possible that // a split will occur after we send the index metadata cache to all known // region servers. diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java b/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java index 1dfc5b1e615..816bfb14971 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java @@ -92,6 +92,7 @@ import org.apache.phoenix.query.QueryServicesOptions; import org.apache.phoenix.schema.CompiledTTLExpression; import org.apache.phoenix.schema.IllegalDataException; +import org.apache.phoenix.schema.LiteralTTLExpression; import org.apache.phoenix.schema.PColumn; import org.apache.phoenix.schema.PName; import org.apache.phoenix.schema.PTable; @@ -1862,6 +1863,76 @@ public static void annotateMutationWithConditionalTTL(PhoenixConnection connecti } } + /** + * Annotates mutations for a table/view with a literal TTL so the server-side internal current-row + * scan (IndexRegionObserver.getCurrentRowStates) can mask expired rows exactly like a client read. + * This is the literal-TTL sibling of {@link #annotateMutationWithConditionalTTL}; the two are + * disjoint (one is guarded on a literal expression, the other on a conditional expression) so at + * most one fires for a given table. + *

+ * The guiding principle is to thread precisely what the read path + * ({@link #setScanAttributesForPhoenixTTL}) would set as the {@code _TTL} scan attribute: + *

+ */ + public static void annotateMutationWithLiteralTTL(PhoenixConnection connection, PTable table, + List mutations) throws SQLException { + + if (!(table.getTTLExpression() instanceof LiteralTTLExpression)) { + // Conditional TTL is handled by annotateMutationWithConditionalTTL; NONE has no literal to + // thread. Only a literal TTL (finite or FOREVER) reaches the internal-scan masking path. + return; + } + if (table.isImmutableRows()) { + // optimization for immutable tables since we don't need to read the current row + // before writing + return; + } + + // For a view, honor the view-TTL feature flag exactly as setScanAttributesForPhoenixTTL does. + boolean isView = table.getType() == PTableType.VIEW; + boolean viewTTLEnabled = !isView + || connection.getQueryServices().getConfiguration().getBoolean( + QueryServices.PHOENIX_VIEW_TTL_ENABLED, QueryServicesOptions.DEFAULT_PHOENIX_VIEW_TTL_ENABLED); + + byte[] ttlForScan = null; + if (isView && viewTTLEnabled) { + // A view's literal TTL is not on the shared CF descriptor, so thread it per-mutation. This + // is non-null for FOREVER and finite literals and null only for NONE, matching the read path + // at setScanAttributesForPhoenixTTL (ScanUtil non-null filter). FOREVER must be threaded, not + // skipped: the view CF-descriptor fallback carries the base table's (possibly finite) TTL, so + // an absent _TTL would wrongly mask a view whose rows should live forever. + ttlForScan = table.getCompiledTTLExpression(connection).serialize(); + } + + byte[] isStrictTTL = + table.isStrictTTL() ? null : PBoolean.INSTANCE.toBytes(table.isStrictTTL()); + if (ttlForScan == null && isStrictTTL == null) { + // Strict base literal TTL: the CF-descriptor fallback and default-strict masking already + // reproduce the client read, so no attribute is needed. + return; + } + for (Mutation mutation : mutations) { + if (ttlForScan != null) { + mutation.setAttribute(BaseScannerRegionObserverConstants.TTL, ttlForScan); + } + if (isStrictTTL != null) { + mutation.setAttribute(BaseScannerRegionObserverConstants.IS_STRICT_TTL, isStrictTTL); + } + } + } + public static PageFilter removePageFilterFromFilterList(FilterList filterList) { Iterator filterIterator = filterList.getFilters().iterator(); while (filterIterator.hasNext()) { diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java b/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java new file mode 100644 index 00000000000..01d1352bf90 --- /dev/null +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.coprocessor; + +import java.io.IOException; +import org.apache.hadoop.hbase.client.Scan; +import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment; +import org.apache.hadoop.hbase.regionserver.Region; +import org.apache.hadoop.hbase.regionserver.RegionScanner; +import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants; +import org.apache.phoenix.index.IndexMaintainer; +import org.apache.phoenix.schema.types.PBoolean; + +/** + * Utilities for internal server-side region scans that must honor Phoenix TTL exactly like a client + * read. The client normally sets the empty-column and TTL scan attributes + * ({@link org.apache.phoenix.util.ScanUtil#setScanAttributesForPhoenixTTL}) and the coprocessor + * hook {@code BaseScannerRegionObserver.postScannerOpen} wraps the scan in a + * {@link TTLRegionScanner}. Internal scans opened directly via {@code region.getScanner(scan)} + * bypass that hook, so they set no attributes and are never TTL-masked. These helpers reproduce + * both steps for server-side callers (e.g. {@code IndexRegionObserver} current-row reads) so an + * internal scan masks identically to a client scan. + *

+ * This class lives in the {@code org.apache.phoenix.coprocessor} package so it can reference the + * server-only {@link TTLRegionScanner} and {@link PagingRegionScanner}; the client-side + * {@code ScanUtil} cannot. + */ +public class ServerScanUtil { + + private ServerScanUtil() { + } + + /** + * Sets the Phoenix TTL scan attributes on an internal data-table scan so {@link TTLRegionScanner} + * masks exactly like a client read: + *

    + *
  • the empty-column CF/CQ, sourced from the data table's {@link IndexMaintainer} (identical + * across all maintainers of a data table);
    • + *
  • {@code IS_STRICT_TTL=false} when {@code isStrictTTL == false}, so a non-strict table is not + * masked (absence of the attribute defaults to strict, matching the read path);
    • + *
  • the view's literal TTL as the standard {@code _TTL} scan attribute when + * {@code literalTTLForScan != null}. A base table's literal TTL is left unset so + * {@link TTLRegionScanner}'s CF-descriptor fallback derives it.
    • + *
+ */ + public static void setInternalScanAttributes(Scan scan, IndexMaintainer dataTableMaintainer, + byte[] literalTTLForScan, boolean isStrictTTL) { + scan.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_FAMILY_NAME, + dataTableMaintainer.getDataEmptyKeyValueCF()); + scan.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_QUALIFIER_NAME, + dataTableMaintainer.getEmptyKeyValueQualifierForDataTable()); + if (!isStrictTTL) { + // Absence of the attribute defaults to strict-true (ScanUtil.isStrictTTL), so only set it + // when the table/view is non-strict, mirroring setScanAttributesForPhoenixTTL. + scan.setAttribute(BaseScannerRegionObserverConstants.IS_STRICT_TTL, + PBoolean.INSTANCE.toBytes(false)); + } + if (literalTTLForScan != null) { + // Only views carry a literal TTL here; a base table relies on the CF-descriptor fallback. + scan.setAttribute(BaseScannerRegionObserverConstants.TTL, literalTTLForScan); + } + } + + /** + * Opens a region scanner wrapped exactly as {@code BaseScannerRegionObserver.postScannerOpen} + * wraps a client scan, so TTL masking is applied. This is always safe: + * {@link TTLRegionScanner#isMaskingEnabled} no-ops the masking when Phoenix compaction is + * disabled, the empty-column attributes are absent, the TTL is FOREVER, or the scan is non-strict + * — so wrapping a non-TTL scan changes no behavior. + */ + public static RegionScanner openRegionScanner(RegionCoprocessorEnvironment env, Region region, + Scan scan) throws IOException { + return new TTLRegionScanner(env, scan, + new PagingRegionScanner(region, region.getScanner(scan), scan)); + } +} diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java index 9763388effb..2e6cde0549b 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java @@ -85,6 +85,7 @@ import org.apache.htrace.TraceScope; import org.apache.phoenix.compile.ScanRanges; import org.apache.phoenix.coprocessor.DelegateRegionCoprocessorEnvironment; +import org.apache.phoenix.coprocessor.ServerScanUtil; import org.apache.phoenix.coprocessor.generated.IndexMutationsProtos; import org.apache.phoenix.coprocessor.generated.PTableProtos; import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants; @@ -119,6 +120,8 @@ import org.apache.phoenix.query.QueryConstants; import org.apache.phoenix.query.QueryServicesOptions; import org.apache.phoenix.schema.CompiledConditionalTTLExpression; +import org.apache.phoenix.schema.CompiledTTLExpression; +import org.apache.phoenix.schema.LiteralTTLExpression; import org.apache.phoenix.schema.PColumn; import org.apache.phoenix.schema.PRow; import org.apache.phoenix.schema.PTable; @@ -172,6 +175,20 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver { private static final OperationStatus NOWRITE = new OperationStatus(SUCCESS); public static final String PHOENIX_APPEND_METADATA_TO_WAL = "phoenix.append.metadata.to.wal"; public static final boolean DEFAULT_PHOENIX_APPEND_METADATA_TO_WAL = false; + /** + * When true (default), a partial "touch" upsert re-persists into the data-table Put every non-PK + * column referenced by any index (indexed or covered) that the touch did not itself write, + * sourcing the value from the masked current-row read. This stamps the data-side covered cell at + * the same {@code batchTimestamp} as the index-side cell so any later compaction retains or + * expires the data and index rows together, closing the covered-column timestamp-skew divergence + * (RCA Point 2). It only fires when an effective Phoenix TTL applies (a current-row read is + * already happening for a non-immutable table with a global covered/indexed index), so non-TTL + * tables are unaffected regardless of this flag. It can be disabled to avoid the write + * amplification, SCN/time-travel, and CDC-fidelity costs it introduces. + */ + public static final String PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED = + "phoenix.index.ttl.column.resync.enabled"; + public static final boolean DEFAULT_PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED = true; public static final String PHOENIX_INDEX_CDC_CONSUMER_ENABLED = "phoenix.index.cdc.consumer.enabled"; public static final boolean DEFAULT_PHOENIX_INDEX_CDC_CONSUMER_ENABLED = true; @@ -380,6 +397,11 @@ public static class BatchMutateContext { private boolean returnOldRow; private boolean hasConditionalTTL; // table has Conditional TTL private boolean immutableRows; + // For a VIEW carrying a view-level literal TTL, the serialized compiled literal TTL threaded + // by the client via the _TTL mutation attribute. Captured (and the attribute removed) early so + // the internal current-row scan masks with the view's TTL. Null for base tables (their literal + // TTL is on the CF descriptor) and for conditional TTL (left on the mutation for its own path). + private byte[] literalTTLForInternalScan; public BatchMutateContext() { this.clientVersion = 0; @@ -486,6 +508,7 @@ public int getMaxPendingRowCount() { private boolean serializeCDCMutations = DEFAULT_PHOENIX_INDEX_CDC_MUTATION_SERIALIZE; private boolean isNamespaceEnabled = false; private boolean useBloomFilter = false; + private boolean indexTTLColumnResyncEnabled = DEFAULT_PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED; private long lastTimestamp = 0; private List> batchesWithLastTimestamp = new ArrayList<>(); private IndexCDCConsumer indexCDCConsumer; @@ -540,6 +563,9 @@ public void start(CoprocessorEnvironment e) throws IOException { this.dataTableName = env.getRegionInfo().getTable().getNameAsString(); this.shouldWALAppend = env.getConfiguration().getBoolean(PHOENIX_APPEND_METADATA_TO_WAL, DEFAULT_PHOENIX_APPEND_METADATA_TO_WAL); + this.indexTTLColumnResyncEnabled = + env.getConfiguration().getBoolean(PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED, + DEFAULT_PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED); this.indexCDCConsumerEnabled = env.getConfiguration() .getBoolean(PHOENIX_INDEX_CDC_CONSUMER_ENABLED, DEFAULT_PHOENIX_INDEX_CDC_CONSUMER_ENABLED); this.compressCDCMutations = @@ -950,6 +976,82 @@ private static void setTimestampOnMutation(Mutation m, long ts) throws IOExcepti } } + /** + * RCA Point 2. On a partial "touch" upsert that does not re-write an index-referenced column, the + * data-side cell keeps its original timestamp while the index-side cell is rebuilt at + * {@code batchTimestamp}. A later major compaction then opens a {@code >ttl} gap on the data side + * but not the index side, dropping the value only on the data table — the exact divergence in the + * RCA. To close it, for every index-enabled non-atomic Put we inject each non-PK column referenced + * by any index ({@link IndexMaintainer#getAllColumnsForDataTable()}, the union of indexed and + * covered columns) that the touch omitted, sourcing the value from the masked current-row state. + * The cells are added at {@link HConstants#LATEST_TIMESTAMP} so the subsequent + * {@link #setTimestamps} call re-stamps them to {@code batchTimestamp}, making the data-side and + * index-side cells share both the timestamp and the empty-column neighbor. Any compaction then + * feeds identical timestamps into the same gap analysis on both sides. + *

+ * This runs after {@code updateMutationsForConditionalTTL}, so a conditionally-expired row has + * {@code dataRowStates.put(key, null)} and the {@code current == null} guard correctly skips + * resurrecting it. Atomic / ON DUPLICATE KEY Puts are excluded — they are already reconstructed by + * {@code generateOnDupMutations} — and immutable tables no-op because no current-row read happened. + */ + private void rewriteIndexReferencedColumns(MiniBatchOperationInProgress miniBatchOp, + BatchMutateContext context, PhoenixIndexMetaData indexMetaData) throws IOException { + if (!indexTTLColumnResyncEnabled) { + return; + } + if (!context.hasGlobalIndex || context.dataRowStates == null) { + // Only covered/indexed global indexes can diverge this way; without a current-row read + // there is nothing to re-persist from. + return; + } + // Union of non-PK columns referenced by any index (indexed + covered). PK columns live in the + // row key, never as cells in a Put, so they are naturally excluded by the has()/get() checks. + Set indexReferencedCols = new HashSet<>(); + for (IndexMaintainer maintainer : indexMetaData.getIndexMaintainers()) { + if (maintainer.isLocalIndex() || maintainer.isUncovered()) { + continue; + } + indexReferencedCols.addAll(maintainer.getAllColumnsForDataTable()); + } + if (indexReferencedCols.isEmpty()) { + return; + } + for (int i = 0; i < miniBatchOp.size(); i++) { + Mutation m = miniBatchOp.getOperation(i); + if (!(m instanceof Put) || !this.builder.isEnabled(m)) { + continue; + } + // Exclude atomic / ON DUPLICATE KEY Puts; they are reconstructed by generateOnDupMutations. + if (this.builder.isAtomicOp(m) || this.builder.returnResult(m)) { + continue; + } + Pair rowState = context.dataRowStates.get(new ImmutableBytesPtr(m.getRow())); + Put current = rowState != null ? rowState.getFirst() : null; + if (current == null) { + // Expired/masked row or a brand new row: nothing to re-persist. + continue; + } + Put put = (Put) m; + for (ColumnReference ref : indexReferencedCols) { + byte[] family = ref.getFamily(); + byte[] qualifier = ref.getQualifier(); + if (put.has(family, qualifier)) { + // The touch already writes this column; leave it (idempotence guard). + continue; + } + List currentCells = current.get(family, qualifier); + if (currentCells == null || currentCells.isEmpty()) { + continue; + } + // Re-persist the current value at LATEST_TIMESTAMP; setTimestamps stamps it to + // batchTimestamp uniformly with the rest of this Put. + Cell currentCell = currentCells.get(0); + put.addColumn(family, qualifier, HConstants.LATEST_TIMESTAMP, + CellUtil.cloneValue(currentCell)); + } + } + } + /** * This method applies pending delete mutations on the next row states */ @@ -1159,11 +1261,42 @@ private boolean isPartialUncoveredIndexMutation(PhoenixIndexMetaData indexMetaDa return false; } + /** + * Selects an {@link IndexMaintainer} to source the data table's empty-column CF/CQ for the + * internal current-row scan. A covered global index is preferred, but the empty CF/CQ is a + * data-table property identical across all maintainers, so any works; the first is used as a + * fallback. Returns null when there are no maintainers (e.g. an atomic-only mutation with no + * index), in which case the internal scan is left unmasked (there is no index to diverge). + */ + private IndexMaintainer getDataTableMaintainerForInternalScan( + PhoenixIndexMetaData indexMetaData) { + IndexMaintainer fallback = null; + for (IndexMaintainer maintainer : indexMetaData.getIndexMaintainers()) { + if (fallback == null) { + fallback = maintainer; + } + if (!maintainer.isLocalIndex() && !maintainer.isUncovered()) { + // A covered global index — the path this masking is designed for. + return maintainer; + } + } + return fallback; + } + /** * Retrieve the data row state either from memory or disk. The rows are locked by the caller. + *

+ * The disk read is opened through {@link ServerScanUtil} so it is TTL-masked exactly like a + * client read: the internal scan otherwise bypasses the {@code postScannerOpen} coprocessor hook + * (the only place a scan is wrapped in {@code TTLRegionScanner}) and would rebuild the index from + * logically-expired-but-physically-present cells. {@code dataTableMaintainer} supplies the + * empty-column CF/CQ, {@code literalTTLForScan} carries a view's literal TTL (null for base + * tables, which use the CF-descriptor fallback), and {@code isStrictTTL} avoids over-masking a + * non-strict table. Masking is a strict no-op when Phoenix compaction is disabled. */ private void getCurrentRowStates(ObserverContext c, - BatchMutateContext context) throws IOException { + BatchMutateContext context, IndexMaintainer dataTableMaintainer, byte[] literalTTLForScan, + boolean isStrictTTL) throws IOException { Set keys = new HashSet(context.rowsToLock.size()); for (ImmutableBytesPtr rowKeyPtr : context.rowsToLock) { PendingRow pendingRow = new PendingRow(rowKeyPtr, context); @@ -1220,6 +1353,10 @@ private void getCurrentRowStates(ObserverContext c // for bloom filters scan should be a get scan.withStartRow(key.getLowerRange(), true); scan.withStopRow(key.getLowerRange(), true); + if (dataTableMaintainer != null) { + ServerScanUtil.setInternalScanAttributes(scan, dataTableMaintainer, literalTTLForScan, + isStrictTTL); + } readDataTableRows(c, context, scan); } } else { @@ -1228,13 +1365,21 @@ private void getCurrentRowStates(ObserverContext c scanRanges.initializeScan(scan); SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter(); scan.setFilter(skipScanFilter); + if (dataTableMaintainer != null) { + ServerScanUtil.setInternalScanAttributes(scan, dataTableMaintainer, literalTTLForScan, + isStrictTTL); + } readDataTableRows(c, context, scan); } } private void readDataTableRows(ObserverContext c, BatchMutateContext context, Scan scan) throws IOException { - try (RegionScanner scanner = c.getEnvironment().getRegion().getScanner(scan)) { + // Open through ServerScanUtil so the scan is wrapped in TTLRegionScanner (mirroring + // postScannerOpen) and masks TTL-expired rows exactly like a client read. Masking no-ops when + // Phoenix compaction is off or the empty-column attributes are absent. + try (RegionScanner scanner = + ServerScanUtil.openRegionScanner(c.getEnvironment(), c.getEnvironment().getRegion(), scan)) { boolean more = true; while (more) { List cells = new ArrayList(); @@ -1664,6 +1809,43 @@ private static void identifyIndexMaintainerTypes(PhoenixIndexMetaData indexMetaD } } + /** + * The client threads a view's literal TTL via the same {@code _TTL} mutation attribute that the + * server otherwise treats as conditional TTL ({@code PhoenixIndexBuilder.hasConditionalTTL}). We + * make that attribute polymorphic: inspect the representative mutation's {@code _TTL}, and if it + * deserializes to a {@link LiteralTTLExpression} (a view's literal TTL), capture its bytes for the + * internal current-row scan and remove {@code _TTL} from every mutation. If it is conditional (or + * absent), leave everything untouched so the existing conditional-TTL path runs normally. + *

+ * Removal must happen before {@link #identifyMutationTypes} so that + * {@code hasConditionalTTL(m)} returns false at every consumer (it sets + * {@code context.hasConditionalTTL}, which would otherwise reach the blind cast to + * {@code CompiledConditionalTTLExpression} in {@link #updateMutationsForConditionalTTL}). A table + * has exactly one TTL kind and a batch never mixes views, so the decision is uniform across the + * batch. HBase has no removeAttribute, so removal is {@code setAttribute(TTL, null)}. + */ + private void extractLiteralTTLForInternalScan( + MiniBatchOperationInProgress miniBatchOp, BatchMutateContext context) + throws IOException { + if (miniBatchOp.size() == 0) { + return; + } + byte[] ttlBytes = + miniBatchOp.getOperation(0).getAttribute(BaseScannerRegionObserverConstants.TTL); + if (ttlBytes == null) { + return; + } + CompiledTTLExpression ttlExpr = TTLExpressionFactory.create(ttlBytes); + if (!(ttlExpr instanceof LiteralTTLExpression)) { + // Conditional TTL: leave the attribute in place for updateMutationsForConditionalTTL. + return; + } + context.literalTTLForInternalScan = ttlBytes; + for (int i = 0; i < miniBatchOp.size(); i++) { + miniBatchOp.getOperation(i).setAttribute(BaseScannerRegionObserverConstants.TTL, null); + } + } + private void identifyMutationTypes(MiniBatchOperationInProgress miniBatchOp, BatchMutateContext context) throws IOException { for (int i = 0; i < miniBatchOp.size(); i++) { @@ -1817,6 +1999,11 @@ public void preBatchMutateWithExceptions(ObserverContext + * With the flag off, the Point 2 rewrite is skipped: a covcol-omitting touch does NOT re-inject + * covcol into the data Put, so the data-side covcol keeps its original write timestamp. This is the + * mirror of the on-by-default assertion in {@link IndexDataTableTTLSyncIT} and demonstrates that the + * pre-fix timestamp skew (which a later major compaction turns into data/index divergence) returns + * when an operator opts out. + */ +@Category(NeedsOwnMiniClusterTest.class) +public class IndexDataTableTTLSyncFlagOffIT extends BaseTest { + private ManualEnvironmentEdge injectEdge; + + @BeforeClass + public static synchronized void doSetup() throws Exception { + Map props = IndexDataTableTTLSyncIT.baseServerProps(); + props.put(IndexRegionObserver.PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED, Boolean.toString(false)); + setUpTestDriver(new ReadOnlyProps(props.entrySet().iterator())); + } + + @Before + public void beforeTest() { + EnvironmentEdgeManager.reset(); + injectEdge = new ManualEnvironmentEdge(); + injectEdge.setValue(EnvironmentEdgeManager.currentTimeMillis()); + } + + @After + public synchronized void afterTest() throws Exception { + boolean refCountLeaked = isAnyStoreRefCountLeaked(); + EnvironmentEdgeManager.reset(); + Assert.assertFalse("refCount leaked", refCountLeaked); + } + + @Test + public void testResyncDisabledLeavesCovColAtOriginalTimestamp() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.createStatement().execute("CREATE TABLE " + tableName + + " (id VARCHAR NOT NULL PRIMARY KEY, idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR) TTL=" + + IndexDataTableTTLSyncIT.TTL + ", COLUMN_ENCODED_BYTES=0"); + conn.createStatement().execute( + "CREATE INDEX " + indexName + " ON " + tableName + " (idxcol) INCLUDE (covcol)"); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); + conn.commit(); + long originalCovTs = + maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), COVCOL_VALUE); + + injectEdge.incrementValue(1000); + long touchTime = injectEdge.currentTime(); + conn.createStatement() + .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); + conn.commit(); + + // Flag off: no re-injection, so the data covcol keeps its original timestamp (< touchTime). + long dataCovTs = + maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), COVCOL_VALUE); + assertTrue("with resync disabled, covcol must NOT be re-stamped; originalCovTs=" + + originalCovTs + " touchTime=" + touchTime + " dataCovTs=" + dataCovTs, + dataCovTs == originalCovTs && dataCovTs < touchTime); + } + } +} diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java new file mode 100644 index 00000000000..4d0d508e78f --- /dev/null +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java @@ -0,0 +1,363 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.end2end.index; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.Arrays; +import java.util.Collection; +import java.util.Map; +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.CellScanner; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Admin; +import org.apache.hadoop.hbase.client.Result; +import org.apache.hadoop.hbase.client.ResultScanner; +import org.apache.hadoop.hbase.client.Scan; +import org.apache.hadoop.hbase.client.Table; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants; +import org.apache.phoenix.end2end.NeedsOwnMiniClusterTest; +import org.apache.phoenix.jdbc.PhoenixConnection; +import org.apache.phoenix.query.BaseTest; +import org.apache.phoenix.query.ConnectionQueryServices; +import org.apache.phoenix.query.QueryServices; +import org.apache.phoenix.util.EnvironmentEdgeManager; +import org.apache.phoenix.util.ManualEnvironmentEdge; +import org.apache.phoenix.util.ReadOnlyProps; +import org.apache.phoenix.util.TestUtil; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import org.apache.phoenix.thirdparty.com.google.common.collect.Maps; + +/** + * Verifies that internal server-side index-maintenance current-row reads honor Phoenix TTL exactly + * like a client read (Point 3), and that index-referenced data columns are re-persisted so the data + * table and a global index stay aligned under compaction (Point 2). + *

+ * The scenario is the RCA divergence: a covered column ({@code covcol}) is written once, then never + * re-written by later partial "touch" upserts. On the index side {@code covcol} is rebuilt at every + * touch's {@code batchTimestamp}; on the data side it would otherwise keep its original timestamp, + * and a later major compaction opens a {@code >ttl} gap on only the data side, dropping {@code covcol} + * there while the index keeps it. + *

+ * The Point 2 rewrite re-injects {@code covcol} (and any indexed column) into the touch's data Put at + * {@code HConstants.LATEST_TIMESTAMP}, so {@code setTimestamps} re-stamps it to {@code batchTimestamp} + * and it stays aligned with the index cell. Under a {@link ManualEnvironmentEdge} the server's + * {@code batchTimestamp} equals the touch's wall-clock, so the load-bearing, timing-independent signal + * is: after a covcol-omitting touch, a raw scan's maximum {@code covcol} timestamp on the data side + * advances to the touch time. Pre-fix (or with the resync flag off) it stays at the original write + * time. This class runs with the resync flag defaulting on; {@link IndexDataTableTTLSyncFlagOffIT} + * asserts the same probe stays at the original timestamp when the flag is off. + */ +@Category(NeedsOwnMiniClusterTest.class) +@RunWith(Parameterized.class) +public class IndexDataTableTTLSyncIT extends BaseTest { + static final int MAX_LOOKBACK_AGE = 10; + static final int TTL = 60; + static final String COVCOL_VALUE = "cov-original"; + + private final boolean columnEncoded; + private ManualEnvironmentEdge injectEdge; + + public IndexDataTableTTLSyncIT(boolean columnEncoded) { + this.columnEncoded = columnEncoded; + } + + @Parameterized.Parameters(name = "columnEncoded={0}") + public static synchronized Collection data() { + return Arrays.asList(new Object[][] { { false }, { true } }); + } + + @BeforeClass + public static synchronized void doSetup() throws Exception { + setUpTestDriver(new ReadOnlyProps(baseServerProps().entrySet().iterator())); + } + + /** + * Common server props for the TTL-sync ITs. Kept as a static helper so + * {@link IndexDataTableTTLSyncFlagOffIT} reuses the exact same cluster configuration and only adds + * the resync-disabled flag. + */ + static Map baseServerProps() { + Map props = Maps.newHashMapWithExpectedSize(4); + props.put(QueryServices.GLOBAL_INDEX_ROW_AGE_THRESHOLD_TO_DELETE_MS_ATTRIB, Long.toString(0)); + props.put(BaseScannerRegionObserverConstants.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, + Integer.toString(MAX_LOOKBACK_AGE)); + props.put("hbase.procedure.remote.dispatcher.delay.msec", "0"); + // The view case threads the view's literal TTL as the per-mutation _TTL attribute. + props.put(QueryServices.PHOENIX_VIEW_TTL_ENABLED, Boolean.toString(true)); + return props; + } + + @Before + public void beforeTest() { + EnvironmentEdgeManager.reset(); + injectEdge = new ManualEnvironmentEdge(); + injectEdge.setValue(EnvironmentEdgeManager.currentTimeMillis()); + } + + @After + public synchronized void afterTest() throws Exception { + boolean refCountLeaked = isAnyStoreRefCountLeaked(); + EnvironmentEdgeManager.reset(); + Assert.assertFalse("refCount leaked", refCountLeaked); + } + + /** + * Builds the comma-separated table-property clause. When {@code ttlSeconds >= 0} a {@code TTL} is + * emitted as the first property; {@code COLUMN_ENCODED_BYTES} always follows, and + * {@code IS_STRICT_TTL=false} is appended for a non-strict table. Properties are comma-separated + * (a bare space between properties is a Phoenix parser error). + */ + private String withClause(int ttlSeconds, boolean strict) { + StringBuilder sb = new StringBuilder(" "); + if (ttlSeconds >= 0) { + sb.append("TTL=").append(ttlSeconds).append(", "); + } + sb.append("COLUMN_ENCODED_BYTES=").append(columnEncoded ? 2 : 0); + if (!strict) { + sb.append(", IS_STRICT_TTL=false"); + } + return sb.toString(); + } + + /** + * Base table with a literal TTL and a covered global index. After a partial touch that never + * writes covcol, the fix re-stamps covcol on the data side to the touch's batchTimestamp; a full + * expiry after that shows the data row and the index expiring as a unit. + */ + @Test + public void testBaseTableCoveredColumnResync() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.createStatement() + .execute("CREATE TABLE " + tableName + " (id VARCHAR NOT NULL PRIMARY KEY, " + + "idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR)" + withClause(TTL, true)); + conn.createStatement().execute( + "CREATE INDEX " + indexName + " ON " + tableName + " (idxcol) INCLUDE (covcol)"); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + // Full initial upsert: covcol is written exactly once here and never again. + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); + conn.commit(); + + // Partial touch that does not write covcol, well within the TTL so the row is alive. + injectEdge.incrementValue(1000); + long touchTime = injectEdge.currentTime(); + conn.createStatement() + .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); + conn.commit(); + + // Load-bearing, deterministic signal: covcol was re-injected into the touch's data Put and + // re-stamped to batchTimestamp (== touchTime under the manual edge). Pre-fix it stays at the + // original write time. + long dataCovTs = + maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), COVCOL_VALUE); + assertTrue("covcol should be re-stamped forward on the data side; touchTime=" + touchTime + + " dataCovTs=" + dataCovTs, dataCovTs >= touchTime); + + // Data and index agree on covcol (Point 3 masked the internal scan; Point 2 kept them aligned). + assertCovColConsistent(conn, tableName, "r1", "k1", COVCOL_VALUE); + + // Advancing past TTL + max-lookback and compacting expires the row on BOTH sides together, + // because post-fix covcol sits at batchTimestamp next to empty@batchTimestamp on each side. + injectEdge.incrementValue((TTL + MAX_LOOKBACK_AGE + 1) * 1000L); + flushAndMajorCompact(conn, tableName); + flushAndMajorCompact(conn, indexName); + assertCovColAbsent(conn, tableName, "r1", "k1"); + } + } + + /** + * View with a view-level literal TTL (not on the shared CF descriptor) and a covered index on the + * view. Exercises the literal-_TTL per-mutation threading: the internal scan masks with the view's + * TTL, and covcol is re-stamped just like the base-table case. + */ + @Test + public void testViewCoveredColumnResync() throws Exception { + String baseTableName = generateUniqueName(); + String viewName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + // Base table carries NO TTL; the TTL lives only on the view. + conn.createStatement() + .execute("CREATE TABLE " + baseTableName + " (id VARCHAR NOT NULL PRIMARY KEY, " + + "idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR)" + withClause(-1, true)); + conn.createStatement().execute( + "CREATE VIEW " + viewName + " AS SELECT * FROM " + baseTableName + " TTL=" + TTL); + conn.createStatement() + .execute("CREATE INDEX " + indexName + " ON " + viewName + " (idxcol) INCLUDE (covcol)"); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + conn.createStatement().execute("UPSERT INTO " + viewName + + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); + conn.commit(); + + injectEdge.incrementValue(1000); + long touchTime = injectEdge.currentTime(); + conn.createStatement() + .execute("UPSERT INTO " + viewName + " (id, touchcol) VALUES ('r1', 'x1')"); + conn.commit(); + + long dataCovTs = maxTimestampForValue(conn, TableName.valueOf(baseTableName), + Bytes.toBytes("r1"), COVCOL_VALUE); + assertTrue("covcol should be re-stamped forward on the view's data side; touchTime=" + + touchTime + " dataCovTs=" + dataCovTs, dataCovTs >= touchTime); + + assertCovColConsistent(conn, viewName, "r1", "k1", COVCOL_VALUE); + } + } + + /** + * A non-strict table must not be over-masked: the strictness flag is threaded to the internal + * scan, so a value that a non-strict client read still returns is retained and re-stamped rather + * than dropped. If strictness were NOT ported, the internal scan would default to strict and mask + * covcol away past the TTL, so the rewrite would find nothing to re-persist and the probe would + * stay at the original timestamp. + */ + @Test + public void testNonStrictTableNotOverMasked() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.createStatement() + .execute("CREATE TABLE " + tableName + " (id VARCHAR NOT NULL PRIMARY KEY, " + + "idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR)" + withClause(TTL, false)); + conn.createStatement().execute( + "CREATE INDEX " + indexName + " ON " + tableName + " (idxcol) INCLUDE (covcol)"); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); + conn.commit(); + + // Advance beyond TTL, then touch. A strict table would mask covcol at the internal scan; + // a non-strict table must not. + injectEdge.incrementValue((TTL + 5) * 1000L); + long touchTime = injectEdge.currentTime(); + conn.createStatement() + .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); + conn.commit(); + + long dataCovTs = + maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), COVCOL_VALUE); + assertTrue("non-strict covcol should be retained and re-stamped, not masked away; touchTime=" + + touchTime + " dataCovTs=" + dataCovTs, dataCovTs >= touchTime); + + assertCovColConsistent(conn, tableName, "r1", "k1", COVCOL_VALUE); + } + } + + // ---- helpers ---- + + private void assertCovColConsistent(Connection conn, String queryTableName, String id, + String idxColValue, String expectedCovVal) throws SQLException { + // Force the data-table read. + try (ResultSet rs = conn.createStatement().executeQuery("SELECT /*+ NO_INDEX */ covcol FROM " + + queryTableName + " WHERE id = '" + id + "'")) { + assertTrue("data-table row should exist", rs.next()); + assertEquals("data-table covcol", expectedCovVal, rs.getString(1)); + assertFalse(rs.next()); + } + // Read via the index (idxcol is the leading index column). + try (ResultSet rs = conn.createStatement().executeQuery( + "SELECT covcol FROM " + queryTableName + " WHERE idxcol = '" + idxColValue + "'")) { + assertTrue("index-visible row should exist", rs.next()); + assertEquals("index covcol", expectedCovVal, rs.getString(1)); + assertFalse(rs.next()); + } + } + + private void assertCovColAbsent(Connection conn, String queryTableName, String id, + String idxColValue) throws SQLException { + try (ResultSet rs = conn.createStatement().executeQuery("SELECT /*+ NO_INDEX */ covcol FROM " + + queryTableName + " WHERE id = '" + id + "'")) { + assertFalse("data-table row should be expired", rs.next()); + } + try (ResultSet rs = conn.createStatement().executeQuery( + "SELECT covcol FROM " + queryTableName + " WHERE idxcol = '" + idxColValue + "'")) { + assertFalse("index-visible row should be expired", rs.next()); + } + } + + /** + * Raw-scans a single data-table row and returns the maximum timestamp among cells whose value + * equals {@code value}. Matching by value bytes is column-encoding independent (encoding rewrites + * qualifiers, not VARCHAR values), so this works for both COLUMN_ENCODED_BYTES=0 and 2. + */ + static long maxTimestampForValue(Connection conn, TableName tableName, byte[] rowKey, String value) + throws SQLException, IOException { + byte[] valueBytes = Bytes.toBytes(value); + ConnectionQueryServices cqs = conn.unwrap(PhoenixConnection.class).getQueryServices(); + long maxTs = -1L; + try (Table table = cqs.getTable(tableName.getName())) { + Scan scan = new Scan(); + scan.withStartRow(rowKey, true); + scan.withStopRow(rowKey, true); + scan.setRaw(true); + scan.readAllVersions(); + try (ResultScanner scanner = table.getScanner(scan)) { + Result result; + while ((result = scanner.next()) != null) { + CellScanner cellScanner = result.cellScanner(); + while (cellScanner.advance()) { + Cell cell = cellScanner.current(); + if ( + Bytes.equals(valueBytes, 0, valueBytes.length, cell.getValueArray(), + cell.getValueOffset(), cell.getValueLength()) + ) { + maxTs = Math.max(maxTs, cell.getTimestamp()); + } + } + } + } + } + return maxTs; + } + + private void flushAndMajorCompact(Connection conn, String tableName) throws Exception { + TableName tn = TableName.valueOf(tableName); + try (Admin admin = conn.unwrap(PhoenixConnection.class).getQueryServices().getAdmin()) { + admin.flush(tn); + } + TestUtil.majorCompact(getUtility(), tn); + } +} From c46a1548e80dde1620379f168635c0df1ae8d106 Mon Sep 17 00:00:00 2001 From: Sanjeet Malhotra Date: Mon, 6 Jul 2026 01:51:01 +0530 Subject: [PATCH 2/9] Thread empty-column CF/CQ unconditionally and mask no-index current-row 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. --- .../org/apache/phoenix/util/ScanUtil.java | 41 +++- .../phoenix/coprocessor/ServerScanUtil.java | 75 ++++++- .../hbase/index/IndexRegionObserver.java | 138 +++++++++---- .../index/IndexDataTableTTLSyncFlagOffIT.java | 49 ++++- .../index/IndexDataTableTTLSyncIT.java | 185 +++++++++++++++++- 5 files changed, 419 insertions(+), 69 deletions(-) diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java b/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java index 816bfb14971..cf57cd581a8 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java @@ -1884,6 +1884,17 @@ public static void annotateMutationWithConditionalTTL(PhoenixConnection connecti *

  • For either type, {@code IS_STRICT_TTL=false} is set only when the table/view is non-strict, * so absence defaults to strict, matching the read-path convention and avoiding over-masking of a * non-strict table.
    • + *
  • For either type, the empty-column CF/CQ are threaded unconditionally (for any mutable + * literal-TTL table/view, regardless of the TTL value or the view-TTL flag), exactly as the client + * read path does: {@link #setScanAttributesForClient} sets the empty column on every non-analyze + * scan. They merely identify the table's empty column and only enable masking; + * {@link org.apache.phoenix.coprocessor.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 from it. + * They are also the only source of these values for the no-index current-row read (an atomic / ON + * DUPLICATE KEY / {@code returnResult} / row-delete on a TTL table), which has no + * {@code IndexMaintainer} on the server. Derived exactly as the read path derives them via + * {@link SchemaUtil#getEmptyColumnFamily(PTable)} / {@link SchemaUtil#getEmptyColumnQualifier}.
    • * */ public static void annotateMutationWithLiteralTTL(PhoenixConnection connection, PTable table, @@ -1906,23 +1917,31 @@ public static void annotateMutationWithLiteralTTL(PhoenixConnection connection, || connection.getQueryServices().getConfiguration().getBoolean( QueryServices.PHOENIX_VIEW_TTL_ENABLED, QueryServicesOptions.DEFAULT_PHOENIX_VIEW_TTL_ENABLED); + // The view's literal TTL is threaded as _TTL only for a view with view-TTL enabled, since it is + // not on the shared CF descriptor. A view with view-TTL disabled sets no _TTL on the read path + // (it returns after only IS_STRICT_TTL), so neither do we; a base table's literal TTL is on the + // CF descriptor, so the server's TTLRegionScanner fallback derives it and we thread no _TTL. byte[] ttlForScan = null; if (isView && viewTTLEnabled) { - // A view's literal TTL is not on the shared CF descriptor, so thread it per-mutation. This - // is non-null for FOREVER and finite literals and null only for NONE, matching the read path - // at setScanAttributesForPhoenixTTL (ScanUtil non-null filter). FOREVER must be threaded, not - // skipped: the view CF-descriptor fallback carries the base table's (possibly finite) TTL, so - // an absent _TTL would wrongly mask a view whose rows should live forever. + // serialize() is non-null for FOREVER and finite literals and null only for NONE, the exact + // non-null filter the read path uses at setScanAttributesForPhoenixTTL. FOREVER must be + // threaded, not skipped: the view CF-descriptor fallback carries the base table's (possibly + // finite) TTL, so an absent _TTL would wrongly mask a view whose rows should live forever. ttlForScan = table.getCompiledTTLExpression(connection).serialize(); } + // The empty-column CF/CQ are threaded unconditionally, exactly as the client read path + // (setScanAttributesForClient) sets them 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. They are also the + // only source of these values for the no-index current-row read (atomic / ON DUPLICATE KEY / + // returnResult / row-delete), which has no IndexMaintainer on the server. + byte[] emptyCF = SchemaUtil.getEmptyColumnFamily(table); + byte[] emptyCQ = SchemaUtil.getEmptyColumnQualifier(table); + byte[] isStrictTTL = table.isStrictTTL() ? null : PBoolean.INSTANCE.toBytes(table.isStrictTTL()); - if (ttlForScan == null && isStrictTTL == null) { - // Strict base literal TTL: the CF-descriptor fallback and default-strict masking already - // reproduce the client read, so no attribute is needed. - return; - } for (Mutation mutation : mutations) { if (ttlForScan != null) { mutation.setAttribute(BaseScannerRegionObserverConstants.TTL, ttlForScan); @@ -1930,6 +1949,8 @@ public static void annotateMutationWithLiteralTTL(PhoenixConnection connection, if (isStrictTTL != null) { mutation.setAttribute(BaseScannerRegionObserverConstants.IS_STRICT_TTL, isStrictTTL); } + mutation.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_FAMILY_NAME, emptyCF); + mutation.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_QUALIFIER_NAME, emptyCQ); } } diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java b/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java index 01d1352bf90..814ceaf7a9c 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java @@ -18,13 +18,20 @@ package org.apache.phoenix.coprocessor; import java.io.IOException; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.client.Scan; import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment; import org.apache.hadoop.hbase.regionserver.Region; import org.apache.hadoop.hbase.regionserver.RegionScanner; +import org.apache.hadoop.hbase.util.Bytes; import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants; +import org.apache.phoenix.filter.PagingFilter; import org.apache.phoenix.index.IndexMaintainer; +import org.apache.phoenix.query.QueryServices; +import org.apache.phoenix.query.QueryServicesOptions; import org.apache.phoenix.schema.types.PBoolean; +import org.apache.phoenix.util.ScanUtil; /** * Utilities for internal server-side region scans that must honor Phoenix TTL exactly like a client @@ -46,24 +53,34 @@ private ServerScanUtil() { } /** - * Sets the Phoenix TTL scan attributes on an internal data-table scan so {@link TTLRegionScanner} - * masks exactly like a client read: + * Sets the Phoenix TTL and paging scan attributes on an internal data-table scan so it behaves + * exactly like a client read. + *

    + * TTL masking attributes ({@link TTLRegionScanner} reads these): *

      - *
    • the empty-column CF/CQ, sourced from the data table's {@link IndexMaintainer} (identical - * across all maintainers of a data table);
      • + *
    • the empty-column CF/CQ, supplied by the caller. When a secondary index is being maintained + * they come from the data table's {@link IndexMaintainer} (identical across all maintainers of a + * data table); when the current-row read is triggered without any index (an atomic / ON DUPLICATE + * KEY / {@code returnResult} / row-delete on a TTL table) there is no maintainer, so they are the + * bytes the client threaded on the mutation + * ({@link org.apache.phoenix.util.ScanUtil#annotateMutationWithLiteralTTL}). Both resolve to the + * same data-table empty column;
      • *
    • {@code IS_STRICT_TTL=false} when {@code isStrictTTL == false}, so a non-strict table is not * masked (absence of the attribute defaults to strict, matching the read path);
      • *
    • the view's literal TTL as the standard {@code _TTL} scan attribute when * {@code literalTTLForScan != null}. A base table's literal TTL is left unset so * {@link TTLRegionScanner}'s CF-descriptor fallback derives it.
      • *
    + * Paging setup (mirrors the client read path: {@code ScanUtil.setScanAttributeForPaging} plus the + * {@code BaseScannerRegionObserver.preScannerOpen} filter wrapping): sets + * {@code SERVER_PAGE_SIZE_MS} and wraps the scan filter in a {@link PagingFilter} so the internal + * scan is bounded by the server page budget just like a client scan. See + * {@link #setInternalScanAttributesForPaging}. */ - public static void setInternalScanAttributes(Scan scan, IndexMaintainer dataTableMaintainer, - byte[] literalTTLForScan, boolean isStrictTTL) { - scan.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_FAMILY_NAME, - dataTableMaintainer.getDataEmptyKeyValueCF()); - scan.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_QUALIFIER_NAME, - dataTableMaintainer.getEmptyKeyValueQualifierForDataTable()); + public static void setInternalScanAttributes(Configuration conf, Scan scan, byte[] emptyCF, + byte[] emptyCQ, byte[] literalTTLForScan, boolean isStrictTTL) { + scan.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_FAMILY_NAME, emptyCF); + scan.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_QUALIFIER_NAME, emptyCQ); if (!isStrictTTL) { // Absence of the attribute defaults to strict-true (ScanUtil.isStrictTTL), so only set it // when the table/view is non-strict, mirroring setScanAttributesForPhoenixTTL. @@ -74,6 +91,44 @@ public static void setInternalScanAttributes(Scan scan, IndexMaintainer dataTabl // Only views carry a literal TTL here; a base table relies on the CF-descriptor fallback. scan.setAttribute(BaseScannerRegionObserverConstants.TTL, literalTTLForScan); } + setInternalScanAttributesForPaging(conf, scan); + } + + /** + * 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. + *

    + * 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))); + } } /** diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java index 2e6cde0549b..6c76e5b33cf 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java @@ -143,6 +143,7 @@ import org.apache.phoenix.util.IndexUtil; import org.apache.phoenix.util.MutationUtil; import org.apache.phoenix.util.PhoenixKeyValueUtil; +import org.apache.phoenix.util.ScanUtil; import org.apache.phoenix.util.SchemaUtil; import org.apache.phoenix.util.ServerIndexUtil; import org.apache.phoenix.util.ServerUtil.ConnectionType; @@ -180,8 +181,8 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver { * column referenced by any index (indexed or covered) that the touch did not itself write, * sourcing the value from the masked current-row read. This stamps the data-side covered cell at * the same {@code batchTimestamp} as the index-side cell so any later compaction retains or - * expires the data and index rows together, closing the covered-column timestamp-skew divergence - * (RCA Point 2). It only fires when an effective Phoenix TTL applies (a current-row read is + * expires the data and index rows together, closing the covered-column timestamp-skew divergence. + * It only fires when an effective Phoenix TTL applies (a current-row read is * already happening for a non-immutable table with a global covered/indexed index), so non-TTL * tables are unaffected regardless of this flag. It can be disabled to avoid the write * amplification, SCN/time-travel, and CDC-fidelity costs it introduces. @@ -402,6 +403,13 @@ public static class BatchMutateContext { // the internal current-row scan masks with the view's TTL. Null for base tables (their literal // TTL is on the CF descriptor) and for conditional TTL (left on the mutation for its own path). private byte[] literalTTLForInternalScan; + // The empty-column CF/CQ threaded by the client (ScanUtil.annotateMutationWithLiteralTTL) for + // any table/view with an effective literal TTL. They let the internal current-row scan mask + // even when no IndexMaintainer is available to supply them — the no-index atomic / ON DUPLICATE + // KEY / returnResult / row-delete path on a TTL table. Null when no literal TTL applies. Unlike + // the _TTL attribute these are left on the mutations (they are inert on the write path). + private byte[] emptyCFForInternalScan; + private byte[] emptyCQForInternalScan; public BatchMutateContext() { this.clientVersion = 0; @@ -977,18 +985,28 @@ private static void setTimestampOnMutation(Mutation m, long ts) throws IOExcepti } /** - * RCA Point 2. On a partial "touch" upsert that does not re-write an index-referenced column, the - * data-side cell keeps its original timestamp while the index-side cell is rebuilt at - * {@code batchTimestamp}. A later major compaction then opens a {@code >ttl} gap on the data side - * but not the index side, dropping the value only on the data table — the exact divergence in the - * RCA. To close it, for every index-enabled non-atomic Put we inject each non-PK column referenced - * by any index ({@link IndexMaintainer#getAllColumnsForDataTable()}, the union of indexed and - * covered columns) that the touch omitted, sourcing the value from the masked current-row state. + * On a partial "touch" upsert that does not re-write an index-referenced column, the data-side + * cell keeps its original timestamp while the index side is rebuilt at {@code batchTimestamp}. A + * later major compaction then opens a {@code >ttl} gap on the data side but not the index side, + * dropping the value only on the data table, so the data row and index row diverge. To close it, + * for every index-enabled non-atomic Put we inject each non-PK column referenced by any index + * ({@link IndexMaintainer#getAllColumnsForDataTable()}, the union of indexed and covered columns) + * that the touch omitted, sourcing the value from the masked current-row state. * The cells are added at {@link HConstants#LATEST_TIMESTAMP} so the subsequent * {@link #setTimestamps} call re-stamps them to {@code batchTimestamp}, making the data-side and * index-side cells share both the timestamp and the empty-column neighbor. Any compaction then * feeds identical timestamps into the same gap analysis on both sides. *

    + * Both covered global indexes and uncovered global indexes are handled. A covered index stores + * the value as an index cell rebuilt at {@code batchTimestamp}; an uncovered index encodes its + * indexed columns positionally into the index row key, also rebuilt at {@code batchTimestamp}. + * Either way the index side survives a compaction that trims the stale data-side cell, so the + * indexed column must be re-persisted on the data side for both — otherwise an uncovered index's + * live row silently drops out of an indexed-column predicate (the read path re-verifies the key + * against the data row and excludes, rather than resurrects, the trimmed value). Local indexes + * are excluded: their prior-row state flows through the separate {@code CachedLocalTable} path, + * not this current-row read. + *

    * This runs after {@code updateMutationsForConditionalTTL}, so a conditionally-expired row has * {@code dataRowStates.put(key, null)} and the {@code current == null} guard correctly skips * resurrecting it. Atomic / ON DUPLICATE KEY Puts are excluded — they are already reconstructed by @@ -999,16 +1017,18 @@ private void rewriteIndexReferencedColumns(MiniBatchOperationInProgress indexReferencedCols = new HashSet<>(); for (IndexMaintainer maintainer : indexMetaData.getIndexMaintainers()) { - if (maintainer.isLocalIndex() || maintainer.isUncovered()) { + if (maintainer.isLocalIndex()) { continue; } indexReferencedCols.addAll(maintainer.getAllColumnsForDataTable()); @@ -1266,7 +1286,8 @@ private boolean isPartialUncoveredIndexMutation(PhoenixIndexMetaData indexMetaDa * internal current-row scan. A covered global index is preferred, but the empty CF/CQ is a * data-table property identical across all maintainers, so any works; the first is used as a * fallback. Returns null when there are no maintainers (e.g. an atomic-only mutation with no - * index), in which case the internal scan is left unmasked (there is no index to diverge). + * index); in that case {@link #getCurrentRowStates} falls back to the empty-column CF/CQ the + * client threaded on the mutation, so a no-index current-row read on a TTL table is still masked. */ private IndexMaintainer getDataTableMaintainerForInternalScan( PhoenixIndexMetaData indexMetaData) { @@ -1289,10 +1310,16 @@ private IndexMaintainer getDataTableMaintainerForInternalScan( * The disk read is opened through {@link ServerScanUtil} so it is TTL-masked exactly like a * client read: the internal scan otherwise bypasses the {@code postScannerOpen} coprocessor hook * (the only place a scan is wrapped in {@code TTLRegionScanner}) and would rebuild the index from - * logically-expired-but-physically-present cells. {@code dataTableMaintainer} supplies the - * empty-column CF/CQ, {@code literalTTLForScan} carries a view's literal TTL (null for base - * tables, which use the CF-descriptor fallback), and {@code isStrictTTL} avoids over-masking a - * non-strict table. Masking is a strict no-op when Phoenix compaction is disabled. + * logically-expired-but-physically-present cells. The empty-column CF/CQ come from + * {@code dataTableMaintainer} when a secondary index is being maintained, and otherwise (the + * no-index atomic / ON DUPLICATE KEY / {@code returnResult} / row-delete path on a TTL table) from + * the bytes the client threaded on the mutation and captured into the batch context; + * {@code literalTTLForScan} carries a view's literal TTL (null for base tables, which use the + * CF-descriptor fallback), and {@code isStrictTTL} avoids over-masking a non-strict table. Masking + * is a strict no-op when Phoenix compaction is disabled. The internal + * scan is also given the client read path's server-paging setup (a {@code SERVER_PAGE_SIZE_MS} + * attribute and a {@code PagingFilter} wrap), so it is bounded by the server page budget just like + * a client scan; {@code readDataTableRows} skips the dummy results that paging can emit. */ private void getCurrentRowStates(ObserverContext c, BatchMutateContext context, IndexMaintainer dataTableMaintainer, byte[] literalTTLForScan, @@ -1345,6 +1372,20 @@ private void getCurrentRowStates(ObserverContext c return; } + // Resolve the empty-column CF/CQ that let TTLRegionScanner mask this internal scan. When a + // secondary index is being maintained they come from the data-table maintainer; otherwise (the + // no-index atomic / ON DUPLICATE KEY / returnResult / row-delete path on a TTL table) they are + // the bytes the client threaded on the mutation and captured into the batch context. Both + // resolve to the same data-table empty column. When neither is available the scan is left + // unmasked (no effective TTL, or a non-TTL table), which is safe. + byte[] emptyCF = dataTableMaintainer != null + ? dataTableMaintainer.getDataEmptyKeyValueCF() + : context.emptyCFForInternalScan; + byte[] emptyCQ = dataTableMaintainer != null + ? dataTableMaintainer.getEmptyKeyValueQualifierForDataTable() + : context.emptyCQForInternalScan; + boolean maskInternalScan = emptyCF != null && emptyCQ != null; + if (this.useBloomFilter) { for (KeyRange key : keys) { // Scan.java usage alters scan instances, safer to create scan instance per usage @@ -1353,9 +1394,9 @@ private void getCurrentRowStates(ObserverContext c // for bloom filters scan should be a get scan.withStartRow(key.getLowerRange(), true); scan.withStopRow(key.getLowerRange(), true); - if (dataTableMaintainer != null) { - ServerScanUtil.setInternalScanAttributes(scan, dataTableMaintainer, literalTTLForScan, - isStrictTTL); + if (maskInternalScan) { + ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, + emptyCF, emptyCQ, literalTTLForScan, isStrictTTL); } readDataTableRows(c, context, scan); } @@ -1365,9 +1406,9 @@ private void getCurrentRowStates(ObserverContext c scanRanges.initializeScan(scan); SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter(); scan.setFilter(skipScanFilter); - if (dataTableMaintainer != null) { - ServerScanUtil.setInternalScanAttributes(scan, dataTableMaintainer, literalTTLForScan, - isStrictTTL); + if (maskInternalScan) { + ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, + emptyCF, emptyCQ, literalTTLForScan, isStrictTTL); } readDataTableRows(c, context, scan); } @@ -1387,6 +1428,12 @@ private void readDataTableRows(ObserverContext c, if (cells.isEmpty()) { continue; } + // 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; + } byte[] rowKey = CellUtil.cloneRow(cells.get(0)); Put put = new Put(rowKey); for (Cell cell : cells) { @@ -1810,14 +1857,26 @@ private static void identifyIndexMaintainerTypes(PhoenixIndexMetaData indexMetaD } /** - * The client threads a view's literal TTL via the same {@code _TTL} mutation attribute that the - * server otherwise treats as conditional TTL ({@code PhoenixIndexBuilder.hasConditionalTTL}). We - * make that attribute polymorphic: inspect the representative mutation's {@code _TTL}, and if it - * deserializes to a {@link LiteralTTLExpression} (a view's literal TTL), capture its bytes for the - * internal current-row scan and remove {@code _TTL} from every mutation. If it is conditional (or - * absent), leave everything untouched so the existing conditional-TTL path runs normally. + * Captures the literal-TTL internal-scan attributes the client threads + * ({@link org.apache.phoenix.util.ScanUtil#annotateMutationWithLiteralTTL}) off the representative + * mutation so the internal current-row scan can mask exactly like a client read. *

    - * Removal must happen before {@link #identifyMutationTypes} so that + * Two things are captured: + *

      + *
    • The empty-column CF/CQ — threaded whenever an effective (non-NONE) literal TTL + * applies, for both base tables and views. They let {@code setInternalScanAttributes} mask even + * when no {@link IndexMaintainer} is available to supply them: the no-index atomic / ON DUPLICATE + * KEY / {@code returnResult} / row-delete path on a TTL table. They are inert on the write path + * (nothing reads a mutation's empty-column attribute), so they are left on the mutations, not + * removed.
      • + *
    • The view's literal {@code _TTL} — the client threads it via the same {@code _TTL} + * mutation attribute the server otherwise treats as conditional TTL + * ({@code PhoenixIndexBuilder.hasConditionalTTL}). We make that attribute polymorphic: if the + * representative mutation's {@code _TTL} deserializes to a {@link LiteralTTLExpression} (a view's + * literal TTL), capture its bytes and remove {@code _TTL} from every mutation; if it is + * conditional (or absent), leave it untouched so the conditional-TTL path runs normally.
      • + *
    + * The {@code _TTL} removal must happen before {@link #identifyMutationTypes} so that * {@code hasConditionalTTL(m)} returns false at every consumer (it sets * {@code context.hasConditionalTTL}, which would otherwise reach the blind cast to * {@code CompiledConditionalTTLExpression} in {@link #updateMutationsForConditionalTTL}). A table @@ -1830,8 +1889,15 @@ private void extractLiteralTTLForInternalScan( if (miniBatchOp.size() == 0) { return; } - byte[] ttlBytes = - miniBatchOp.getOperation(0).getAttribute(BaseScannerRegionObserverConstants.TTL); + Mutation representative = miniBatchOp.getOperation(0); + // Empty-column CF/CQ are threaded independently of _TTL (a base finite-TTL table threads them + // but no _TTL), so capture them regardless of the _TTL attribute below. Left on the mutations. + context.emptyCFForInternalScan = + representative.getAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_FAMILY_NAME); + context.emptyCQForInternalScan = + representative.getAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_QUALIFIER_NAME); + + byte[] ttlBytes = representative.getAttribute(BaseScannerRegionObserverConstants.TTL); if (ttlBytes == null) { return; } @@ -2077,7 +2143,7 @@ && isPartialUncoveredIndexMutation(indexMetaData, miniBatchOp) long batchTimestamp = getBatchTimestamp(context, table); // Re-persist index-referenced columns a partial touch omitted, so the data-side covered cell // and the index-side cell share batchTimestamp and any compaction retains/expires them - // together (RCA Point 2). Must run before setTimestamps so injected LATEST cells are stamped. + // together. Must run before setTimestamps so injected LATEST cells are stamped. rewriteIndexReferencedColumns(miniBatchOp, context, indexMetaData); // Update the timestamps of the data table mutations to prevent overlapping timestamps // (which prevents index inconsistencies as this case is not handled). diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncFlagOffIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncFlagOffIT.java index b53ef313d36..d2fe335be3f 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncFlagOffIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncFlagOffIT.java @@ -18,6 +18,7 @@ package org.apache.phoenix.end2end.index; import static org.apache.phoenix.end2end.index.IndexDataTableTTLSyncIT.COVCOL_VALUE; +import static org.apache.phoenix.end2end.index.IndexDataTableTTLSyncIT.IDXCOL_VALUE; import static org.apache.phoenix.end2end.index.IndexDataTableTTLSyncIT.maxTimestampForValue; import static org.junit.Assert.assertTrue; @@ -44,11 +45,11 @@ * once server-side at coprocessor {@code start()}, so it can only be exercised with its own mini * cluster — hence a separate class from {@link IndexDataTableTTLSyncIT}. *

    - * With the flag off, the Point 2 rewrite is skipped: a covcol-omitting touch does NOT re-inject + * With the flag off, the column re-sync is skipped: a covcol-omitting touch does NOT re-inject * covcol into the data Put, so the data-side covcol keeps its original write timestamp. This is the * mirror of the on-by-default assertion in {@link IndexDataTableTTLSyncIT} and demonstrates that the - * pre-fix timestamp skew (which a later major compaction turns into data/index divergence) returns - * when an operator opts out. + * timestamp skew (which a later major compaction turns into data/index divergence) returns when an + * operator opts out. */ @Category(NeedsOwnMiniClusterTest.class) public class IndexDataTableTTLSyncFlagOffIT extends BaseTest { @@ -109,4 +110,46 @@ public void testResyncDisabledLeavesCovColAtOriginalTimestamp() throws Exception dataCovTs == originalCovTs && dataCovTs < touchTime); } } + + /** + * The uncovered-index counterpart to the covered case above: with the flag off, an + * {@code idxcol}-omitting touch does NOT re-inject the uncovered index's indexed column, so the + * data-side {@code idxcol} keeps its original write timestamp. This is the mirror of + * {@link IndexDataTableTTLSyncIT#testUncoveredIndexIndexedColumnResync} and confirms the kill + * switch governs the uncovered path too. + */ + @Test + public void testResyncDisabledLeavesUncoveredIdxColAtOriginalTimestamp() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.createStatement().execute("CREATE TABLE " + tableName + + " (id VARCHAR NOT NULL PRIMARY KEY, idxcol VARCHAR, touchcol VARCHAR) TTL=" + + IndexDataTableTTLSyncIT.TTL + ", COLUMN_ENCODED_BYTES=0"); + conn.createStatement() + .execute("CREATE UNCOVERED INDEX " + indexName + " ON " + tableName + " (idxcol)"); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, idxcol, touchcol) VALUES ('r1', '" + IDXCOL_VALUE + "', 'x0')"); + conn.commit(); + long originalIdxTs = + maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), IDXCOL_VALUE); + + injectEdge.incrementValue(1000); + long touchTime = injectEdge.currentTime(); + conn.createStatement() + .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); + conn.commit(); + + // Flag off: no re-injection, so the data idxcol keeps its original timestamp (< touchTime). + long dataIdxTs = + maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), IDXCOL_VALUE); + assertTrue("with resync disabled, uncovered idxcol must NOT be re-stamped; originalIdxTs=" + + originalIdxTs + " touchTime=" + touchTime + " dataIdxTs=" + dataIdxTs, + dataIdxTs == originalIdxTs && dataIdxTs < touchTime); + } + } } diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java index 4d0d508e78f..a432789bee2 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java @@ -19,6 +19,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import java.io.IOException; @@ -61,16 +62,16 @@ /** * Verifies that internal server-side index-maintenance current-row reads honor Phoenix TTL exactly - * like a client read (Point 3), and that index-referenced data columns are re-persisted so the data - * table and a global index stay aligned under compaction (Point 2). + * like a client read, and that index-referenced data columns are re-persisted so the data table and + * a global index stay aligned under compaction. *

    - * The scenario is the RCA divergence: a covered column ({@code covcol}) is written once, then never - * re-written by later partial "touch" upserts. On the index side {@code covcol} is rebuilt at every - * touch's {@code batchTimestamp}; on the data side it would otherwise keep its original timestamp, - * and a later major compaction opens a {@code >ttl} gap on only the data side, dropping {@code covcol} - * there while the index keeps it. + * The scenario is the divergence being fixed: a covered column ({@code covcol}) is written once, + * then never re-written by later partial "touch" upserts. On the index side {@code covcol} is + * rebuilt at every touch's {@code batchTimestamp}; on the data side it would otherwise keep its + * original timestamp, and a later major compaction opens a {@code >ttl} gap on only the data side, + * dropping {@code covcol} there while the index keeps it. *

    - * The Point 2 rewrite re-injects {@code covcol} (and any indexed column) into the touch's data Put at + * The column re-sync re-injects {@code covcol} (and any indexed column) into the touch's data Put at * {@code HConstants.LATEST_TIMESTAMP}, so {@code setTimestamps} re-stamps it to {@code batchTimestamp} * and it stays aligned with the index cell. Under a {@link ManualEnvironmentEdge} the server's * {@code batchTimestamp} equals the touch's wall-clock, so the load-bearing, timing-independent signal @@ -78,6 +79,12 @@ * advances to the touch time. Pre-fix (or with the resync flag off) it stays at the original write * time. This class runs with the resync flag defaulting on; {@link IndexDataTableTTLSyncFlagOffIT} * asserts the same probe stays at the original timestamp when the flag is off. + *

    + * {@link #testNoIndexAtomicUpsertMasksExpiredRow} covers the no-index masking path: an atomic + * {@code ON DUPLICATE KEY UPDATE} on a TTL table with no secondary index still triggers an internal + * current-row read, but there is no {@code IndexMaintainer} to supply the empty-column CF/CQ. That + * scan is masked using the CF/CQ the client threads on the mutation, so an expired row is treated as + * absent rather than resurrected. */ @Category(NeedsOwnMiniClusterTest.class) @RunWith(Parameterized.class) @@ -85,6 +92,7 @@ public class IndexDataTableTTLSyncIT extends BaseTest { static final int MAX_LOOKBACK_AGE = 10; static final int TTL = 60; static final String COVCOL_VALUE = "cov-original"; + static final String IDXCOL_VALUE = "idx-original"; private final boolean columnEncoded; private ManualEnvironmentEdge injectEdge; @@ -190,7 +198,8 @@ public void testBaseTableCoveredColumnResync() throws Exception { assertTrue("covcol should be re-stamped forward on the data side; touchTime=" + touchTime + " dataCovTs=" + dataCovTs, dataCovTs >= touchTime); - // Data and index agree on covcol (Point 3 masked the internal scan; Point 2 kept them aligned). + // Data and index agree on covcol: the masked internal scan and the column re-sync keep them + // aligned. assertCovColConsistent(conn, tableName, "r1", "k1", COVCOL_VALUE); // Advancing past TTL + max-lookback and compacting expires the row on BOTH sides together, @@ -202,6 +211,76 @@ public void testBaseTableCoveredColumnResync() throws Exception { } } + /** + * Uncovered global index on a base table with a literal TTL. An uncovered index stores its + * indexed column ({@code idxcol}) positionally in the index row key, rebuilt at every touch's + * {@code batchTimestamp}, while a partial touch that omits {@code idxcol} would otherwise leave + * the data-side {@code idxcol} cell at its original timestamp. A later major compaction can then + * trim the stale data-side {@code idxcol} while the row stays alive, so the index key encodes a + * value the data row no longer has — and because the uncovered read path re-verifies the rebuilt + * key against the data row, the live row silently drops out of an {@code idxcol} predicate rather + * than surfacing the stale value. The re-sync must therefore cover uncovered indexes too: it + * re-injects {@code idxcol} into the touch's data Put at {@code LATEST_TIMESTAMP} so + * {@code setTimestamps} advances it to {@code batchTimestamp} alongside the index key. + *

    + * Load-bearing, timing-independent signal (mirrors {@link #testBaseTableCoveredColumnResync}): + * after an {@code idxcol}-omitting touch of a still-alive row, a raw scan's maximum {@code idxcol} + * timestamp on the data side advances to the touch time. Pre-fix (uncovered skipped) it stays at + * the original write time. + */ + @Test + public void testUncoveredIndexIndexedColumnResync() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.createStatement() + .execute("CREATE TABLE " + tableName + " (id VARCHAR NOT NULL PRIMARY KEY, " + + "idxcol VARCHAR, touchcol VARCHAR)" + withClause(TTL, true)); + conn.createStatement() + .execute("CREATE UNCOVERED INDEX " + indexName + " ON " + tableName + " (idxcol)"); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + // Full initial upsert: idxcol is written exactly once here and never again. + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, idxcol, touchcol) VALUES ('r1', '" + IDXCOL_VALUE + "', 'x0')"); + conn.commit(); + + // Partial touch that does not write idxcol, well within the TTL so the row is alive and the + // masked internal scan still returns idxcol for the re-sync to re-persist. + injectEdge.incrementValue(1000); + long touchTime = injectEdge.currentTime(); + conn.createStatement() + .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); + conn.commit(); + + // The uncovered index's indexed column was re-injected into the touch's data Put and + // re-stamped to batchTimestamp (== touchTime under the manual edge). Pre-fix it stays at the + // original write time. + long dataIdxTs = + maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), IDXCOL_VALUE); + assertTrue("uncovered idxcol should be re-stamped forward on the data side; touchTime=" + + touchTime + " dataIdxTs=" + dataIdxTs, dataIdxTs >= touchTime); + + // The row is retrievable through the uncovered index (join-back + key re-verification passes + // because the data-side idxcol still encodes the same key) and touchcol reflects the touch. + assertEquals("touchcol via uncovered index", "x1", + touchColViaUncoveredIndex(conn, tableName, indexName, IDXCOL_VALUE)); + + // Advancing past TTL + max-lookback and compacting expires the row on BOTH sides together. + injectEdge.incrementValue((TTL + MAX_LOOKBACK_AGE + 1) * 1000L); + flushAndMajorCompact(conn, tableName); + flushAndMajorCompact(conn, indexName); + try (ResultSet rs = conn.createStatement().executeQuery("SELECT /*+ NO_INDEX */ touchcol FROM " + + tableName + " WHERE id = 'r1'")) { + assertFalse("data-table row should be expired", rs.next()); + } + assertNull("row should be expired via the uncovered index too", + touchColViaUncoveredIndex(conn, tableName, indexName, IDXCOL_VALUE)); + } + } + /** * View with a view-level literal TTL (not on the shared CF descriptor) and a covered index on the * view. Exercises the literal-_TTL per-mutation threading: the internal scan masks with the view's @@ -286,8 +365,72 @@ public void testNonStrictTableNotOverMasked() throws Exception { } } + /** + * The no-index masking path: an atomic {@code ON DUPLICATE KEY UPDATE} on a base table with a + * literal TTL and NO secondary index. The atomic path still opens an internal current-row read + * ({@code getCurrentRowStates} fires for {@code hasAtomic}), but there is no + * {@code IndexMaintainer} to supply the empty-column CF/CQ that let {@code TTLRegionScanner} mask + * the scan. Those are instead threaded on the mutation by the client + * ({@code ScanUtil.annotateMutationWithLiteralTTL}) and captured server-side, so the internal scan + * masks an expired row exactly like a client read — treating it as absent rather than resurrecting + * it. + *

    + * {@code counter = counter + 1} reads the current {@code counter} from the masked current-row + * state, so it is the sharpest probe of masking. After the row TTL-expires, a masked scan returns + * no current row, the {@code ON DUPLICATE KEY} clause is skipped, and the {@code UPSERT VALUES} + * ({@code counter = 0}) are inserted fresh. Pre-fix the unmasked scan resurrects the expired row + * and increments {@code counter} to 1. No flush/compaction is involved: the expired cells are + * still physically present, so read masking alone — not compaction — governs the outcome. + */ + @Test + public void testNoIndexAtomicUpsertMasksExpiredRow() throws Exception { + String tableName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.createStatement().execute("CREATE TABLE " + tableName + + " (id VARCHAR NOT NULL PRIMARY KEY, counter INTEGER)" + withClause(TTL, true)); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + // First atomic upsert: the row does not exist, so the ON DUPLICATE clause is skipped and the + // UPSERT VALUES insert counter = 0. + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, counter) VALUES ('r1', 0) ON DUPLICATE KEY UPDATE counter = counter + 1"); + conn.commit(); + assertEquals("fresh insert of a new row", 0, atomicCounter(conn, tableName)); + + // Advance past the TTL. No flush/compact: the row's cells are still physically present, so + // only read masking - not compaction - can hide the now-expired row from the internal scan. + injectEdge.incrementValue((TTL + 1) * 1000L); + + // Second atomic upsert on the now-expired row. Post-fix the masked internal scan returns no + // current row, so this inserts counter = 0 again; pre-fix the unmasked scan resurrects the + // row and the ON DUPLICATE clause increments counter to 1. + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, counter) VALUES ('r1', 0) ON DUPLICATE KEY UPDATE counter = counter + 1"); + conn.commit(); + + assertEquals("expired row must be treated as absent, not resurrected and incremented", 0, + atomicCounter(conn, tableName)); + } + } + // ---- helpers ---- + /** + * Reads the single {@code counter} value for row {@code r1}, asserting exactly one live row. Used + * by {@link #testNoIndexAtomicUpsertMasksExpiredRow} to observe the atomic upsert's result. + */ + private static int atomicCounter(Connection conn, String tableName) throws SQLException { + try (ResultSet rs = conn.createStatement() + .executeQuery("SELECT counter FROM " + tableName + " WHERE id = 'r1'")) { + assertTrue("row should be present", rs.next()); + int value = rs.getInt(1); + assertFalse("expected exactly one row", rs.next()); + return value; + } + } + private void assertCovColConsistent(Connection conn, String queryTableName, String id, String idxColValue, String expectedCovVal) throws SQLException { // Force the data-table read. @@ -353,11 +496,33 @@ static long maxTimestampForValue(Connection conn, TableName tableName, byte[] ro return maxTs; } - private void flushAndMajorCompact(Connection conn, String tableName) throws Exception { + static void flushAndMajorCompact(Connection conn, String tableName) throws Exception { TableName tn = TableName.valueOf(tableName); try (Admin admin = conn.unwrap(PhoenixConnection.class).getQueryServices().getAdmin()) { admin.flush(tn); } TestUtil.majorCompact(getUtility(), tn); } + + /** + * Reads {@code touchcol} for the given indexed value through the uncovered index, forcing the + * join-back-and-verify read path ({@code touchcol} is neither an index key nor a covered column, + * so the query must join back to the data table and the uncovered scanner re-verifies the index + * key against the data row). Returns the {@code touchcol} value, or {@code null} if the uncovered + * read excludes the row — which happens when the data-side indexed column was trimmed by + * compaction so the rebuilt index key no longer matches the stored one. + */ + static String touchColViaUncoveredIndex(Connection conn, String tableName, String indexName, + String idxColValue) throws SQLException { + String sql = "SELECT /*+ INDEX(" + tableName + " " + indexName + ") */ touchcol FROM " + + tableName + " WHERE idxcol = '" + idxColValue + "'"; + try (ResultSet rs = conn.createStatement().executeQuery(sql)) { + if (!rs.next()) { + return null; + } + String value = rs.getString(1); + assertFalse("expected at most one row via the uncovered index", rs.next()); + return value; + } + } } From 4d5dfb53e18b80f0d558c2193c42c3c58260db48 Mon Sep 17 00:00:00 2001 From: Sanjeet Malhotra Date: Mon, 6 Jul 2026 13:32:41 +0530 Subject: [PATCH 3/9] Drop IndexMaintainer as emptyCF/CQ source for internal current-row scan 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. --- .../hbase/index/IndexRegionObserver.java | 61 ++++++------------- 1 file changed, 18 insertions(+), 43 deletions(-) diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java index 6c76e5b33cf..d548aafb088 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java @@ -1281,39 +1281,16 @@ private boolean isPartialUncoveredIndexMutation(PhoenixIndexMetaData indexMetaDa return false; } - /** - * Selects an {@link IndexMaintainer} to source the data table's empty-column CF/CQ for the - * internal current-row scan. A covered global index is preferred, but the empty CF/CQ is a - * data-table property identical across all maintainers, so any works; the first is used as a - * fallback. Returns null when there are no maintainers (e.g. an atomic-only mutation with no - * index); in that case {@link #getCurrentRowStates} falls back to the empty-column CF/CQ the - * client threaded on the mutation, so a no-index current-row read on a TTL table is still masked. - */ - private IndexMaintainer getDataTableMaintainerForInternalScan( - PhoenixIndexMetaData indexMetaData) { - IndexMaintainer fallback = null; - for (IndexMaintainer maintainer : indexMetaData.getIndexMaintainers()) { - if (fallback == null) { - fallback = maintainer; - } - if (!maintainer.isLocalIndex() && !maintainer.isUncovered()) { - // A covered global index — the path this masking is designed for. - return maintainer; - } - } - return fallback; - } - /** * Retrieve the data row state either from memory or disk. The rows are locked by the caller. *

    * The disk read is opened through {@link ServerScanUtil} so it is TTL-masked exactly like a * client read: the internal scan otherwise bypasses the {@code postScannerOpen} coprocessor hook * (the only place a scan is wrapped in {@code TTLRegionScanner}) and would rebuild the index from - * logically-expired-but-physically-present cells. The empty-column CF/CQ come from - * {@code dataTableMaintainer} when a secondary index is being maintained, and otherwise (the - * no-index atomic / ON DUPLICATE KEY / {@code returnResult} / row-delete path on a TTL table) from - * the bytes the client threaded on the mutation and captured into the batch context; + * logically-expired-but-physically-present cells. The empty-column CF/CQ that enable masking are + * the bytes the client threaded on the mutation + * ({@link org.apache.phoenix.util.ScanUtil#annotateMutationWithLiteralTTL}) and captured into the + * batch context — the single source for every path that reaches here, index or no-index alike; * {@code literalTTLForScan} carries a view's literal TTL (null for base tables, which use the * CF-descriptor fallback), and {@code isStrictTTL} avoids over-masking a non-strict table. Masking * is a strict no-op when Phoenix compaction is disabled. The internal @@ -1322,8 +1299,7 @@ private IndexMaintainer getDataTableMaintainerForInternalScan( * a client scan; {@code readDataTableRows} skips the dummy results that paging can emit. */ private void getCurrentRowStates(ObserverContext c, - BatchMutateContext context, IndexMaintainer dataTableMaintainer, byte[] literalTTLForScan, - boolean isStrictTTL) throws IOException { + BatchMutateContext context, byte[] literalTTLForScan, boolean isStrictTTL) throws IOException { Set keys = new HashSet(context.rowsToLock.size()); for (ImmutableBytesPtr rowKeyPtr : context.rowsToLock) { PendingRow pendingRow = new PendingRow(rowKeyPtr, context); @@ -1372,18 +1348,17 @@ private void getCurrentRowStates(ObserverContext c return; } - // Resolve the empty-column CF/CQ that let TTLRegionScanner mask this internal scan. When a - // secondary index is being maintained they come from the data-table maintainer; otherwise (the - // no-index atomic / ON DUPLICATE KEY / returnResult / row-delete path on a TTL table) they are - // the bytes the client threaded on the mutation and captured into the batch context. Both - // resolve to the same data-table empty column. When neither is available the scan is left - // unmasked (no effective TTL, or a non-TTL table), which is safe. - byte[] emptyCF = dataTableMaintainer != null - ? dataTableMaintainer.getDataEmptyKeyValueCF() - : context.emptyCFForInternalScan; - byte[] emptyCQ = dataTableMaintainer != null - ? dataTableMaintainer.getEmptyKeyValueQualifierForDataTable() - : context.emptyCQForInternalScan; + // The empty-column CF/CQ that let TTLRegionScanner mask this internal scan are the bytes the + // client threaded on the mutation (ScanUtil.annotateMutationWithLiteralTTL) and captured into + // the batch context. This is the single source for every path that reaches here — the + // secondary-index case and the no-index atomic / ON DUPLICATE KEY / returnResult / row-delete + // case alike — and it ties internal-scan masking to the same client signal that governs client + // read masking (setScanAttributesForClient), so the two stay consistent. When they are absent + // (a mutation not annotated by the client) the scan is left unmasked, which is safe: such a + // client's own reads are likewise unmasked, and TTLRegionScanner masking is a no-op anyway on a + // non-TTL table (CF-descriptor FOREVER) or when Phoenix compaction is disabled. + byte[] emptyCF = context.emptyCFForInternalScan; + byte[] emptyCQ = context.emptyCQForInternalScan; boolean maskInternalScan = emptyCF != null && emptyCQ != null; if (this.useBloomFilter) { @@ -2107,8 +2082,8 @@ public void preBatchMutateWithExceptions(ObserverContext Date: Mon, 6 Jul 2026 20:32:44 +0530 Subject: [PATCH 4/9] Extend index-referenced column re-sync to atomic / ON DUPLICATE KEY path; 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. --- .../org/apache/phoenix/util/ScanUtil.java | 3 +- .../phoenix/coprocessor/ServerScanUtil.java | 5 - .../hbase/index/IndexRegionObserver.java | 136 ++++++++++----- .../index/IndexDataTableTTLSyncFlagOffIT.java | 155 ------------------ .../index/IndexDataTableTTLSyncIT.java | 9 +- 5 files changed, 97 insertions(+), 211 deletions(-) delete mode 100644 phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncFlagOffIT.java diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java b/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java index cf57cd581a8..dd37a2a9d52 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java @@ -1925,8 +1925,7 @@ public static void annotateMutationWithLiteralTTL(PhoenixConnection connection, if (isView && viewTTLEnabled) { // serialize() is non-null for FOREVER and finite literals and null only for NONE, the exact // non-null filter the read path uses at setScanAttributesForPhoenixTTL. FOREVER must be - // threaded, not skipped: the view CF-descriptor fallback carries the base table's (possibly - // finite) TTL, so an absent _TTL would wrongly mask a view whose rows should live forever. + // threaded, not skipped. ttlForScan = table.getCompiledTTLExpression(connection).serialize(); } diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java b/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java index 814ceaf7a9c..baa98ce9999 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java @@ -71,11 +71,6 @@ private ServerScanUtil() { * {@code literalTTLForScan != null}. A base table's literal TTL is left unset so * {@link TTLRegionScanner}'s CF-descriptor fallback derives it. * - * Paging setup (mirrors the client read path: {@code ScanUtil.setScanAttributeForPaging} plus the - * {@code BaseScannerRegionObserver.preScannerOpen} filter wrapping): sets - * {@code SERVER_PAGE_SIZE_MS} and wraps the scan filter in a {@link PagingFilter} so the internal - * scan is bounded by the server page budget just like a client scan. See - * {@link #setInternalScanAttributesForPaging}. */ public static void setInternalScanAttributes(Configuration conf, Scan scan, byte[] emptyCF, byte[] emptyCQ, byte[] literalTTLForScan, boolean isStrictTTL) { diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java index d548aafb088..8e44dbd7442 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java @@ -176,20 +176,6 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver { private static final OperationStatus NOWRITE = new OperationStatus(SUCCESS); public static final String PHOENIX_APPEND_METADATA_TO_WAL = "phoenix.append.metadata.to.wal"; public static final boolean DEFAULT_PHOENIX_APPEND_METADATA_TO_WAL = false; - /** - * When true (default), a partial "touch" upsert re-persists into the data-table Put every non-PK - * column referenced by any index (indexed or covered) that the touch did not itself write, - * sourcing the value from the masked current-row read. This stamps the data-side covered cell at - * the same {@code batchTimestamp} as the index-side cell so any later compaction retains or - * expires the data and index rows together, closing the covered-column timestamp-skew divergence. - * It only fires when an effective Phoenix TTL applies (a current-row read is - * already happening for a non-immutable table with a global covered/indexed index), so non-TTL - * tables are unaffected regardless of this flag. It can be disabled to avoid the write - * amplification, SCN/time-travel, and CDC-fidelity costs it introduces. - */ - public static final String PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED = - "phoenix.index.ttl.column.resync.enabled"; - public static final boolean DEFAULT_PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED = true; public static final String PHOENIX_INDEX_CDC_CONSUMER_ENABLED = "phoenix.index.cdc.consumer.enabled"; public static final boolean DEFAULT_PHOENIX_INDEX_CDC_CONSUMER_ENABLED = true; @@ -516,7 +502,6 @@ public int getMaxPendingRowCount() { private boolean serializeCDCMutations = DEFAULT_PHOENIX_INDEX_CDC_MUTATION_SERIALIZE; private boolean isNamespaceEnabled = false; private boolean useBloomFilter = false; - private boolean indexTTLColumnResyncEnabled = DEFAULT_PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED; private long lastTimestamp = 0; private List> batchesWithLastTimestamp = new ArrayList<>(); private IndexCDCConsumer indexCDCConsumer; @@ -571,9 +556,6 @@ public void start(CoprocessorEnvironment e) throws IOException { this.dataTableName = env.getRegionInfo().getTable().getNameAsString(); this.shouldWALAppend = env.getConfiguration().getBoolean(PHOENIX_APPEND_METADATA_TO_WAL, DEFAULT_PHOENIX_APPEND_METADATA_TO_WAL); - this.indexTTLColumnResyncEnabled = - env.getConfiguration().getBoolean(PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED, - DEFAULT_PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED); this.indexCDCConsumerEnabled = env.getConfiguration() .getBoolean(PHOENIX_INDEX_CDC_CONSUMER_ENABLED, DEFAULT_PHOENIX_INDEX_CDC_CONSUMER_ENABLED); this.compressCDCMutations = @@ -755,11 +737,12 @@ private void addOnDupMutationsToBatch(MiniBatchOperationInProgress min } private void addOnDupMutationsToBatch(MiniBatchOperationInProgress miniBatchOp, - BatchMutateContext context) throws IOException { + BatchMutateContext context, PhoenixIndexMetaData indexMetaData) throws IOException { for (int i = 0; i < miniBatchOp.size(); i++) { Mutation m = miniBatchOp.getOperation(i); if ((this.builder.isAtomicOp(m) || this.builder.returnResult(m)) && m instanceof Put) { - List mutations = generateOnDupMutations(context, (Put) m, miniBatchOp); + List mutations = generateOnDupMutations(context, (Put) m, miniBatchOp, + indexMetaData); if (!mutations.isEmpty()) { addOnDupMutationsToBatch(miniBatchOp, i, mutations); } else { @@ -1003,9 +986,7 @@ private static void setTimestampOnMutation(Mutation m, long ts) throws IOExcepti * Either way the index side survives a compaction that trims the stale data-side cell, so the * indexed column must be re-persisted on the data side for both — otherwise an uncovered index's * live row silently drops out of an indexed-column predicate (the read path re-verifies the key - * against the data row and excludes, rather than resurrects, the trimmed value). Local indexes - * are excluded: their prior-row state flows through the separate {@code CachedLocalTable} path, - * not this current-row read. + * against the data row and excludes, rather than resurrects, the trimmed value). *

    * This runs after {@code updateMutationsForConditionalTTL}, so a conditionally-expired row has * {@code dataRowStates.put(key, null)} and the {@code current == null} guard correctly skips @@ -1014,25 +995,12 @@ private static void setTimestampOnMutation(Mutation m, long ts) throws IOExcepti */ private void rewriteIndexReferencedColumns(MiniBatchOperationInProgress miniBatchOp, BatchMutateContext context, PhoenixIndexMetaData indexMetaData) throws IOException { - if (!indexTTLColumnResyncEnabled) { - return; - } if ((!context.hasGlobalIndex && !context.hasUncoveredIndex) || context.dataRowStates == null) { // Only covered global or uncovered global indexes can diverge this way; without a current-row // read there is nothing to re-persist from. return; } - // Union of non-PK columns referenced by any covered or uncovered global index (indexed + - // covered). PK columns live in the row key, never as cells in a Put, so they are naturally - // excluded by the has()/get() checks. Local indexes are skipped (separate CachedLocalTable - // maintenance path). - Set indexReferencedCols = new HashSet<>(); - for (IndexMaintainer maintainer : indexMetaData.getIndexMaintainers()) { - if (maintainer.isLocalIndex()) { - continue; - } - indexReferencedCols.addAll(maintainer.getAllColumnsForDataTable()); - } + Set indexReferencedCols = getIndexReferencedColumns(indexMetaData); if (indexReferencedCols.isEmpty()) { return; } @@ -1072,6 +1040,64 @@ private void rewriteIndexReferencedColumns(MiniBatchOperationInProgress getIndexReferencedColumns( + PhoenixIndexMetaData indexMetaData) { + Set indexReferencedCols = new HashSet<>(); + for (IndexMaintainer maintainer : indexMetaData.getIndexMaintainers()) { + if (maintainer.isLocalIndex()) { + continue; + } + indexReferencedCols.addAll(maintainer.getAllColumnsForDataTable()); + } + return indexReferencedCols; + } + + /** + * The atomic / ON DUPLICATE KEY analogue of {@code rewriteIndexReferencedColumns}. Those Puts are + * reconstructed by {@code generateOnDupMutations} rather than written verbatim, so they are + * excluded from {@code rewriteIndexReferencedColumns} and must be re-synced inline here. + *

    + * Injects every index-referenced non-PK column that this upsert omitted, sourcing the value from + * the pre-upsert row snapshot ({@code oldRowColumnCellExprMap}). Each injected cell is added at + * {@code HConstants.LATEST_TIMESTAMP} so the later {@code setTimestamps} re-stamps it to + * {@code batchTimestamp} uniformly with the rest of the Put — making the data-side cell and the + * index-side cell share both timestamp and empty-column neighbor, so any compaction retains or + * expires them together. + *

    + * Idempotent: a column the upsert already writes is left alone ({@code put.has(...)} guard), and a + * column absent from the current row (new row / never set) is skipped. + */ + private void rewriteIndexReferencedColumnsForAtomicPut(Put put, + Set indexReferencedCols, + Map> oldRowColumnCellExprMap) { + if (indexReferencedCols.isEmpty() || oldRowColumnCellExprMap.isEmpty()) { + return; + } + for (ColumnReference ref : indexReferencedCols) { + byte[] family = ref.getFamily(); + byte[] qualifier = ref.getQualifier(); + if (put.has(family, qualifier)) { + // The upsert already writes this column; leave it (idempotence guard). + continue; + } + Pair oldPair = oldRowColumnCellExprMap.get(ref); + if (oldPair == null || oldPair.getFirst() == null) { + // Absent from the pre-upsert row (new row or never set): nothing to re-persist. + continue; + } + put.addColumn(family, qualifier, HConstants.LATEST_TIMESTAMP, + CellUtil.cloneValue(oldPair.getFirst())); + } + } + /** * This method applies pending delete mutations on the next row states */ @@ -2100,7 +2126,7 @@ && isPartialUncoveredIndexMutation(indexMetaData, miniBatchOp) if (context.hasAtomic || context.returnResult) { long start = EnvironmentEdgeManager.currentTimeMillis(); // add the mutations for conditional updates to the mini batch - addOnDupMutationsToBatch(miniBatchOp, context); + addOnDupMutationsToBatch(miniBatchOp, context, indexMetaData); // release locks for ON DUPLICATE KEY IGNORE since we won't be changing those rows // this is needed so that we can exit early @@ -2472,7 +2498,8 @@ public Void visit(KeyValueColumnExpression expression) { * Delete mutation (with DeleteColumn type cells for all columns set to null). */ private List generateOnDupMutations(BatchMutateContext context, Put atomicPut, - MiniBatchOperationInProgress miniBatchOp) throws IOException { + MiniBatchOperationInProgress miniBatchOp, PhoenixIndexMetaData indexMetaData) + throws IOException { List mutations = Lists.newArrayListWithExpectedSize(2); byte[] opBytes = atomicPut.getAttribute(ATOMIC_OP_ATTRIB); byte[] returnResult = atomicPut.getAttribute(RETURN_RESULT); @@ -2496,22 +2523,38 @@ private List generateOnDupMutations(BatchMutateContext context, Put at // true, will be null if there is no expression on this column, otherwise false Map> currColumnCellExprMap = new HashMap<>(); + // Snapshot of the current data row's cells, keyed by ColumnReference. Populated once, up front, + // before any conditional update logic mutates state. It serves two purposes: answering an + // OLD_ROW return request, and re-persisting index-referenced columns this atomic upsert omits + // (see rewriteIndexReferencedColumnsForAtomicPut). It is a local; only exposed on the context + // for an OLD_ROW return request (unchanged semantics). + Map> oldRowColumnCellExprMap = new HashMap<>(); + byte[] rowKey = atomicPut.getRow(); ImmutableBytesPtr rowKeyPtr = new ImmutableBytesPtr(rowKey); // Get the latest data row state Pair dataRowState = context.dataRowStates.get(rowKeyPtr); Put currentDataRowState = dataRowState != null ? dataRowState.getFirst() : null; - // Create separate map for old row data when OLD_ROW is requested - // This must be done before any conditional update logic to preserve original state + // Capture the original row state (no-op when currentDataRowState is null for a new row). + updateCurrColumnCellExpr(currentDataRowState, oldRowColumnCellExprMap); if (context.returnResult && context.returnOldRow && currentDataRowState != null) { - context.oldRowColumnCellExprMap = new HashMap<>(); - updateCurrColumnCellExpr(currentDataRowState, context.oldRowColumnCellExprMap); + context.oldRowColumnCellExprMap = oldRowColumnCellExprMap; } + // Union of non-PK columns referenced by any covered/uncovered global index. Used to re-persist + // columns this atomic upsert omits so the data-side cell and the index-side cell share + // batchTimestamp and a later compaction retains/expires them together. + Set indexReferencedCols = getIndexReferencedColumns(indexMetaData); + // if result needs to be returned but the DML does not have ON DUPLICATE KEY present, // perform the mutation and return the result. if (opBytes == null) { + // Re-persist any index-referenced column this plain upsert omitted (excluded from + // rewriteIndexReferencedColumns since returnResult(m) is true), so the data and index rows + // expire together under compaction. + rewriteIndexReferencedColumnsForAtomicPut(atomicPut, indexReferencedCols, + oldRowColumnCellExprMap); mutations.add(atomicPut); updateCurrColumnCellExpr(currentDataRowState != null ? currentDataRowState : atomicPut, currColumnCellExprMap); @@ -2681,6 +2724,13 @@ private List generateOnDupMutations(BatchMutateContext context, Put at delete.add(cell); } } + // Re-persist any index-referenced column this reconstructed upsert omitted, but only when the + // Put already writes something — so the empty column (row liveness) advances at batchTimestamp + // anyway. Injecting into an otherwise-empty Put would turn a genuine no-op ON DUPLICATE KEY + // UPDATE into a spurious write. + if (!put.isEmpty()) { + rewriteIndexReferencedColumnsForAtomicPut(put, indexReferencedCols, oldRowColumnCellExprMap); + } if (!put.isEmpty() || !delete.isEmpty()) { PTable table = operations.get(0).getFirst(); diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncFlagOffIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncFlagOffIT.java deleted file mode 100644 index d2fe335be3f..00000000000 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncFlagOffIT.java +++ /dev/null @@ -1,155 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.phoenix.end2end.index; - -import static org.apache.phoenix.end2end.index.IndexDataTableTTLSyncIT.COVCOL_VALUE; -import static org.apache.phoenix.end2end.index.IndexDataTableTTLSyncIT.IDXCOL_VALUE; -import static org.apache.phoenix.end2end.index.IndexDataTableTTLSyncIT.maxTimestampForValue; -import static org.junit.Assert.assertTrue; - -import java.sql.Connection; -import java.sql.DriverManager; -import java.util.Map; -import org.apache.hadoop.hbase.TableName; -import org.apache.hadoop.hbase.util.Bytes; -import org.apache.phoenix.end2end.NeedsOwnMiniClusterTest; -import org.apache.phoenix.hbase.index.IndexRegionObserver; -import org.apache.phoenix.query.BaseTest; -import org.apache.phoenix.util.EnvironmentEdgeManager; -import org.apache.phoenix.util.ManualEnvironmentEdge; -import org.apache.phoenix.util.ReadOnlyProps; -import org.junit.After; -import org.junit.Assert; -import org.junit.Before; -import org.junit.BeforeClass; -import org.junit.Test; -import org.junit.experimental.categories.Category; - -/** - * Confirms the {@code phoenix.index.ttl.column.resync.enabled} kill switch. The resync flag is read - * once server-side at coprocessor {@code start()}, so it can only be exercised with its own mini - * cluster — hence a separate class from {@link IndexDataTableTTLSyncIT}. - *

    - * With the flag off, the column re-sync is skipped: a covcol-omitting touch does NOT re-inject - * covcol into the data Put, so the data-side covcol keeps its original write timestamp. This is the - * mirror of the on-by-default assertion in {@link IndexDataTableTTLSyncIT} and demonstrates that the - * timestamp skew (which a later major compaction turns into data/index divergence) returns when an - * operator opts out. - */ -@Category(NeedsOwnMiniClusterTest.class) -public class IndexDataTableTTLSyncFlagOffIT extends BaseTest { - private ManualEnvironmentEdge injectEdge; - - @BeforeClass - public static synchronized void doSetup() throws Exception { - Map props = IndexDataTableTTLSyncIT.baseServerProps(); - props.put(IndexRegionObserver.PHOENIX_INDEX_TTL_COLUMN_RESYNC_ENABLED, Boolean.toString(false)); - setUpTestDriver(new ReadOnlyProps(props.entrySet().iterator())); - } - - @Before - public void beforeTest() { - EnvironmentEdgeManager.reset(); - injectEdge = new ManualEnvironmentEdge(); - injectEdge.setValue(EnvironmentEdgeManager.currentTimeMillis()); - } - - @After - public synchronized void afterTest() throws Exception { - boolean refCountLeaked = isAnyStoreRefCountLeaked(); - EnvironmentEdgeManager.reset(); - Assert.assertFalse("refCount leaked", refCountLeaked); - } - - @Test - public void testResyncDisabledLeavesCovColAtOriginalTimestamp() throws Exception { - String tableName = generateUniqueName(); - String indexName = generateUniqueName(); - try (Connection conn = DriverManager.getConnection(getUrl())) { - conn.createStatement().execute("CREATE TABLE " + tableName - + " (id VARCHAR NOT NULL PRIMARY KEY, idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR) TTL=" - + IndexDataTableTTLSyncIT.TTL + ", COLUMN_ENCODED_BYTES=0"); - conn.createStatement().execute( - "CREATE INDEX " + indexName + " ON " + tableName + " (idxcol) INCLUDE (covcol)"); - conn.commit(); - - EnvironmentEdgeManager.injectEdge(injectEdge); - - conn.createStatement().execute("UPSERT INTO " + tableName - + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); - conn.commit(); - long originalCovTs = - maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), COVCOL_VALUE); - - injectEdge.incrementValue(1000); - long touchTime = injectEdge.currentTime(); - conn.createStatement() - .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); - conn.commit(); - - // Flag off: no re-injection, so the data covcol keeps its original timestamp (< touchTime). - long dataCovTs = - maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), COVCOL_VALUE); - assertTrue("with resync disabled, covcol must NOT be re-stamped; originalCovTs=" - + originalCovTs + " touchTime=" + touchTime + " dataCovTs=" + dataCovTs, - dataCovTs == originalCovTs && dataCovTs < touchTime); - } - } - - /** - * The uncovered-index counterpart to the covered case above: with the flag off, an - * {@code idxcol}-omitting touch does NOT re-inject the uncovered index's indexed column, so the - * data-side {@code idxcol} keeps its original write timestamp. This is the mirror of - * {@link IndexDataTableTTLSyncIT#testUncoveredIndexIndexedColumnResync} and confirms the kill - * switch governs the uncovered path too. - */ - @Test - public void testResyncDisabledLeavesUncoveredIdxColAtOriginalTimestamp() throws Exception { - String tableName = generateUniqueName(); - String indexName = generateUniqueName(); - try (Connection conn = DriverManager.getConnection(getUrl())) { - conn.createStatement().execute("CREATE TABLE " + tableName - + " (id VARCHAR NOT NULL PRIMARY KEY, idxcol VARCHAR, touchcol VARCHAR) TTL=" - + IndexDataTableTTLSyncIT.TTL + ", COLUMN_ENCODED_BYTES=0"); - conn.createStatement() - .execute("CREATE UNCOVERED INDEX " + indexName + " ON " + tableName + " (idxcol)"); - conn.commit(); - - EnvironmentEdgeManager.injectEdge(injectEdge); - - conn.createStatement().execute("UPSERT INTO " + tableName - + " (id, idxcol, touchcol) VALUES ('r1', '" + IDXCOL_VALUE + "', 'x0')"); - conn.commit(); - long originalIdxTs = - maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), IDXCOL_VALUE); - - injectEdge.incrementValue(1000); - long touchTime = injectEdge.currentTime(); - conn.createStatement() - .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); - conn.commit(); - - // Flag off: no re-injection, so the data idxcol keeps its original timestamp (< touchTime). - long dataIdxTs = - maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), IDXCOL_VALUE); - assertTrue("with resync disabled, uncovered idxcol must NOT be re-stamped; originalIdxTs=" - + originalIdxTs + " touchTime=" + touchTime + " dataIdxTs=" + dataIdxTs, - dataIdxTs == originalIdxTs && dataIdxTs < touchTime); - } - } -} diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java index a432789bee2..0b820c588cb 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java @@ -76,9 +76,7 @@ * and it stays aligned with the index cell. Under a {@link ManualEnvironmentEdge} the server's * {@code batchTimestamp} equals the touch's wall-clock, so the load-bearing, timing-independent signal * is: after a covcol-omitting touch, a raw scan's maximum {@code covcol} timestamp on the data side - * advances to the touch time. Pre-fix (or with the resync flag off) it stays at the original write - * time. This class runs with the resync flag defaulting on; {@link IndexDataTableTTLSyncFlagOffIT} - * asserts the same probe stays at the original timestamp when the flag is off. + * advances to the touch time. Pre-fix it stays at the original write time. *

    * {@link #testNoIndexAtomicUpsertMasksExpiredRow} covers the no-index masking path: an atomic * {@code ON DUPLICATE KEY UPDATE} on a TTL table with no secondary index still triggers an internal @@ -112,9 +110,8 @@ public static synchronized void doSetup() throws Exception { } /** - * Common server props for the TTL-sync ITs. Kept as a static helper so - * {@link IndexDataTableTTLSyncFlagOffIT} reuses the exact same cluster configuration and only adds - * the resync-disabled flag. + * Common server props for the TTL-sync IT: max-lookback and immediate global-index row aging so a + * major compaction deterministically exercises the covered-column timestamp-skew divergence. */ static Map baseServerProps() { Map props = Maps.newHashMapWithExpectedSize(4); From 83cdb4520b2df6641153dc7a366f63ba05c49e25 Mon Sep 17 00:00:00 2001 From: Sanjeet Malhotra Date: Tue, 7 Jul 2026 00:34:36 +0530 Subject: [PATCH 5/9] Consolidate atomic / ON DUPLICATE KEY re-sync into rewriteIndexReferencedColumns 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. --- .../hbase/index/IndexRegionObserver.java | 172 +++++++----------- 1 file changed, 68 insertions(+), 104 deletions(-) diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java index 8e44dbd7442..039b4ce684a 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java @@ -737,12 +737,11 @@ private void addOnDupMutationsToBatch(MiniBatchOperationInProgress min } private void addOnDupMutationsToBatch(MiniBatchOperationInProgress miniBatchOp, - BatchMutateContext context, PhoenixIndexMetaData indexMetaData) throws IOException { + BatchMutateContext context) throws IOException { for (int i = 0; i < miniBatchOp.size(); i++) { Mutation m = miniBatchOp.getOperation(i); if ((this.builder.isAtomicOp(m) || this.builder.returnResult(m)) && m instanceof Put) { - List mutations = generateOnDupMutations(context, (Put) m, miniBatchOp, - indexMetaData); + List mutations = generateOnDupMutations(context, (Put) m, miniBatchOp); if (!mutations.isEmpty()) { addOnDupMutationsToBatch(miniBatchOp, i, mutations); } else { @@ -974,11 +973,21 @@ private static void setTimestampOnMutation(Mutation m, long ts) throws IOExcepti * dropping the value only on the data table, so the data row and index row diverge. To close it, * for every index-enabled non-atomic Put we inject each non-PK column referenced by any index * ({@link IndexMaintainer#getAllColumnsForDataTable()}, the union of indexed and covered columns) - * that the touch omitted, sourcing the value from the masked current-row state. - * The cells are added at {@link HConstants#LATEST_TIMESTAMP} so the subsequent - * {@link #setTimestamps} call re-stamps them to {@code batchTimestamp}, making the data-side and - * index-side cells share both the timestamp and the empty-column neighbor. Any compaction then - * feeds identical timestamps into the same gap analysis on both sides. + * that the touch left unchanged, sourcing the value from the merged next row state + * ({@code dataRowStates.getSecond()}). + * The cells are added directly at {@code batchTimestamp} (this runs after {@link #setTimestamps}), + * making the data-side and index-side cells share both the timestamp and the empty-column + * neighbor. Any compaction then feeds identical timestamps into the same gap analysis on both + * sides. + *

    + * The value is sourced from the merged next row state rather than the raw current row so that a + * column the touch sets to NULL is honored: such a column is emitted as a separate {@code Delete} + * (not a cell in the Put) and is therefore absent from {@code getSecond()} after + * {@code applyPendingDeleteMutations}, so it is correctly not re-persisted. Reading from the + * current row ({@code getFirst()}) would instead resurrect the just-deleted value. Conversely, a + * column the touch neither writes nor deletes is carried over into {@code getSecond()} from the + * current row and is the one that must be re-persisted. Because the index is likewise generated + * from {@code getSecond()}, both sides always agree on the value. *

    * Both covered global indexes and uncovered global indexes are handled. A covered index stores * the value as an index cell rebuilt at {@code batchTimestamp}; an uncovered index encodes its @@ -988,13 +997,21 @@ private static void setTimestampOnMutation(Mutation m, long ts) throws IOExcepti * live row silently drops out of an indexed-column predicate (the read path re-verifies the key * against the data row and excludes, rather than resurrects, the trimmed value). *

    - * This runs after {@code updateMutationsForConditionalTTL}, so a conditionally-expired row has - * {@code dataRowStates.put(key, null)} and the {@code current == null} guard correctly skips - * resurrecting it. Atomic / ON DUPLICATE KEY Puts are excluded — they are already reconstructed by - * {@code generateOnDupMutations} — and immutable tables no-op because no current-row read happened. + * This runs after {@code prepareDataRowStates}, so {@code getSecond()} reflects this batch's puts + * and deletes. A conditionally-expired row was reset by {@code updateMutationsForConditionalTTL} + * ({@code dataRowStates.put(key, null)}), so its rebuilt next state contains only the touch's own + * columns and nothing extra is re-persisted; a full row delete leaves {@code getSecond() == null}, + * which the guard skips. Atomic / ON DUPLICATE KEY Puts are handled here too: {@code + * generateOnDupMutations} reconstructs them and merges the result into the in-batch Put (and thus + * into its next row state) before this runs, so sourcing from {@code getSecond()} re-persists the + * index-referenced columns they omit — including correctly honoring a column an ON DUPLICATE KEY + * UPDATE set to NULL, which a snapshot of the pre-upsert row would instead resurrect. A no-op + * atomic op is skipped via the {@code isAtomicOperationComplete} guard (nothing is written), and + * immutable tables no-op because no current-row read happened. */ private void rewriteIndexReferencedColumns(MiniBatchOperationInProgress miniBatchOp, - BatchMutateContext context, PhoenixIndexMetaData indexMetaData) throws IOException { + BatchMutateContext context, PhoenixIndexMetaData indexMetaData, long batchTimestamp) + throws IOException { if ((!context.hasGlobalIndex && !context.hasUncoveredIndex) || context.dataRowStates == null) { // Only covered global or uncovered global indexes can diverge this way; without a current-row // read there is nothing to re-persist from. @@ -1005,18 +1022,23 @@ private void rewriteIndexReferencedColumns(MiniBatchOperationInProgress rowState = context.dataRowStates.get(new ImmutableBytesPtr(m.getRow())); - Put current = rowState != null ? rowState.getFirst() : null; - if (current == null) { - // Expired/masked row or a brand new row: nothing to re-persist. + Put next = rowState != null ? rowState.getSecond() : null; + if (next == null) { + // Expired/masked row, a brand new row with no carried-over state, or a row fully deleted in + // this batch: nothing to re-persist. continue; } Put put = (Put) m; @@ -1027,15 +1049,16 @@ private void rewriteIndexReferencedColumns(MiniBatchOperationInProgress currentCells = current.get(family, qualifier); - if (currentCells == null || currentCells.isEmpty()) { + List nextCells = next.get(family, qualifier); + if (nextCells == null || nextCells.isEmpty()) { + // Absent from the merged next state: either never set, or set to NULL by this touch + // (a Delete removed it). Either way it must not be re-persisted. continue; } - // Re-persist the current value at LATEST_TIMESTAMP; setTimestamps stamps it to - // batchTimestamp uniformly with the rest of this Put. - Cell currentCell = currentCells.get(0); - put.addColumn(family, qualifier, HConstants.LATEST_TIMESTAMP, - CellUtil.cloneValue(currentCell)); + // Re-persist the merged value directly at batchTimestamp (setTimestamps has already run), + // so the data-side cell matches the index-side cell built at the same timestamp. + Cell nextCell = nextCells.get(0); + put.addColumn(family, qualifier, batchTimestamp, CellUtil.cloneValue(nextCell)); } } } @@ -1060,44 +1083,6 @@ private static Set getIndexReferencedColumns( return indexReferencedCols; } - /** - * The atomic / ON DUPLICATE KEY analogue of {@code rewriteIndexReferencedColumns}. Those Puts are - * reconstructed by {@code generateOnDupMutations} rather than written verbatim, so they are - * excluded from {@code rewriteIndexReferencedColumns} and must be re-synced inline here. - *

    - * Injects every index-referenced non-PK column that this upsert omitted, sourcing the value from - * the pre-upsert row snapshot ({@code oldRowColumnCellExprMap}). Each injected cell is added at - * {@code HConstants.LATEST_TIMESTAMP} so the later {@code setTimestamps} re-stamps it to - * {@code batchTimestamp} uniformly with the rest of the Put — making the data-side cell and the - * index-side cell share both timestamp and empty-column neighbor, so any compaction retains or - * expires them together. - *

    - * Idempotent: a column the upsert already writes is left alone ({@code put.has(...)} guard), and a - * column absent from the current row (new row / never set) is skipped. - */ - private void rewriteIndexReferencedColumnsForAtomicPut(Put put, - Set indexReferencedCols, - Map> oldRowColumnCellExprMap) { - if (indexReferencedCols.isEmpty() || oldRowColumnCellExprMap.isEmpty()) { - return; - } - for (ColumnReference ref : indexReferencedCols) { - byte[] family = ref.getFamily(); - byte[] qualifier = ref.getQualifier(); - if (put.has(family, qualifier)) { - // The upsert already writes this column; leave it (idempotence guard). - continue; - } - Pair oldPair = oldRowColumnCellExprMap.get(ref); - if (oldPair == null || oldPair.getFirst() == null) { - // Absent from the pre-upsert row (new row or never set): nothing to re-persist. - continue; - } - put.addColumn(family, qualifier, HConstants.LATEST_TIMESTAMP, - CellUtil.cloneValue(oldPair.getFirst())); - } - } - /** * This method applies pending delete mutations on the next row states */ @@ -1385,7 +1370,6 @@ private void getCurrentRowStates(ObserverContext c // non-TTL table (CF-descriptor FOREVER) or when Phoenix compaction is disabled. byte[] emptyCF = context.emptyCFForInternalScan; byte[] emptyCQ = context.emptyCQForInternalScan; - boolean maskInternalScan = emptyCF != null && emptyCQ != null; if (this.useBloomFilter) { for (KeyRange key : keys) { @@ -1395,10 +1379,8 @@ private void getCurrentRowStates(ObserverContext c // for bloom filters scan should be a get scan.withStartRow(key.getLowerRange(), true); scan.withStopRow(key.getLowerRange(), true); - if (maskInternalScan) { - ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, - emptyCF, emptyCQ, literalTTLForScan, isStrictTTL); - } + ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, + emptyCF, emptyCQ, literalTTLForScan, isStrictTTL); readDataTableRows(c, context, scan); } } else { @@ -1407,10 +1389,8 @@ private void getCurrentRowStates(ObserverContext c scanRanges.initializeScan(scan); SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter(); scan.setFilter(skipScanFilter); - if (maskInternalScan) { - ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, - emptyCF, emptyCQ, literalTTLForScan, isStrictTTL); - } + ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, + emptyCF, emptyCQ, literalTTLForScan, isStrictTTL); readDataTableRows(c, context, scan); } } @@ -2126,7 +2106,7 @@ && isPartialUncoveredIndexMutation(indexMetaData, miniBatchOp) if (context.hasAtomic || context.returnResult) { long start = EnvironmentEdgeManager.currentTimeMillis(); // add the mutations for conditional updates to the mini batch - addOnDupMutationsToBatch(miniBatchOp, context, indexMetaData); + addOnDupMutationsToBatch(miniBatchOp, context); // release locks for ON DUPLICATE KEY IGNORE since we won't be changing those rows // this is needed so that we can exit early @@ -2142,16 +2122,19 @@ && isPartialUncoveredIndexMutation(indexMetaData, miniBatchOp) TableName table = c.getEnvironment().getRegion().getRegionInfo().getTable(); long batchTimestamp = getBatchTimestamp(context, table); - // Re-persist index-referenced columns a partial touch omitted, so the data-side covered cell - // and the index-side cell share batchTimestamp and any compaction retains/expires them - // together. Must run before setTimestamps so injected LATEST cells are stamped. - rewriteIndexReferencedColumns(miniBatchOp, context, indexMetaData); // Update the timestamps of the data table mutations to prevent overlapping timestamps // (which prevents index inconsistencies as this case is not handled). setTimestamps(miniBatchOp, builder, batchTimestamp, isStrictTTLEnabled(miniBatchOp)); if (context.hasGlobalIndex || context.hasUncoveredIndex || context.hasTransform) { // Prepare next data rows states for pending mutations (for global indexes) prepareDataRowStates(c, miniBatchOp, context, batchTimestamp); + // Re-persist index-referenced columns a partial touch left unchanged, sourcing from the + // merged next row state so the data-side cell and the index-side cell share batchTimestamp + // and any compaction retains/expires them together. Must run after prepareDataRowStates so + // the next state reflects this batch's puts and deletes (a column set to NULL is absent from + // it and is correctly not re-persisted); injects at batchTimestamp since setTimestamps has + // already run. + rewriteIndexReferencedColumns(miniBatchOp, context, indexMetaData, batchTimestamp); // early exit if it turns out we don't have any edits long start = EnvironmentEdgeManager.currentTimeMillis(); preparePreIndexMutations(context, batchTimestamp, indexMetaData); @@ -2498,8 +2481,7 @@ public Void visit(KeyValueColumnExpression expression) { * Delete mutation (with DeleteColumn type cells for all columns set to null). */ private List generateOnDupMutations(BatchMutateContext context, Put atomicPut, - MiniBatchOperationInProgress miniBatchOp, PhoenixIndexMetaData indexMetaData) - throws IOException { + MiniBatchOperationInProgress miniBatchOp) throws IOException { List mutations = Lists.newArrayListWithExpectedSize(2); byte[] opBytes = atomicPut.getAttribute(ATOMIC_OP_ATTRIB); byte[] returnResult = atomicPut.getAttribute(RETURN_RESULT); @@ -2524,10 +2506,8 @@ private List generateOnDupMutations(BatchMutateContext context, Put at Map> currColumnCellExprMap = new HashMap<>(); // Snapshot of the current data row's cells, keyed by ColumnReference. Populated once, up front, - // before any conditional update logic mutates state. It serves two purposes: answering an - // OLD_ROW return request, and re-persisting index-referenced columns this atomic upsert omits - // (see rewriteIndexReferencedColumnsForAtomicPut). It is a local; only exposed on the context - // for an OLD_ROW return request (unchanged semantics). + // before any conditional update logic mutates state. It answers an OLD_ROW return request. It + // is a local; only exposed on the context for an OLD_ROW return request (unchanged semantics). Map> oldRowColumnCellExprMap = new HashMap<>(); byte[] rowKey = atomicPut.getRow(); @@ -2542,19 +2522,11 @@ private List generateOnDupMutations(BatchMutateContext context, Put at context.oldRowColumnCellExprMap = oldRowColumnCellExprMap; } - // Union of non-PK columns referenced by any covered/uncovered global index. Used to re-persist - // columns this atomic upsert omits so the data-side cell and the index-side cell share - // batchTimestamp and a later compaction retains/expires them together. - Set indexReferencedCols = getIndexReferencedColumns(indexMetaData); - // if result needs to be returned but the DML does not have ON DUPLICATE KEY present, - // perform the mutation and return the result. + // perform the mutation and return the result. Any index-referenced column this upsert omits is + // re-persisted later by rewriteIndexReferencedColumns, which now also covers atomic / + // returnResult Puts once they have been merged into the batch's next row state. if (opBytes == null) { - // Re-persist any index-referenced column this plain upsert omitted (excluded from - // rewriteIndexReferencedColumns since returnResult(m) is true), so the data and index rows - // expire together under compaction. - rewriteIndexReferencedColumnsForAtomicPut(atomicPut, indexReferencedCols, - oldRowColumnCellExprMap); mutations.add(atomicPut); updateCurrColumnCellExpr(currentDataRowState != null ? currentDataRowState : atomicPut, currColumnCellExprMap); @@ -2724,14 +2696,6 @@ private List generateOnDupMutations(BatchMutateContext context, Put at delete.add(cell); } } - // Re-persist any index-referenced column this reconstructed upsert omitted, but only when the - // Put already writes something — so the empty column (row liveness) advances at batchTimestamp - // anyway. Injecting into an otherwise-empty Put would turn a genuine no-op ON DUPLICATE KEY - // UPDATE into a spurious write. - if (!put.isEmpty()) { - rewriteIndexReferencedColumnsForAtomicPut(put, indexReferencedCols, oldRowColumnCellExprMap); - } - if (!put.isEmpty() || !delete.isEmpty()) { PTable table = operations.get(0).getFirst(); addEmptyKVCellToPut(put, tuple, table); From 2e9ccfd379d8602e121f8ef20521c2656d904511 Mon Sep 17 00:00:00 2001 From: Sanjeet Malhotra Date: Tue, 7 Jul 2026 01:41:16 +0530 Subject: [PATCH 6/9] Fix rewriteIndexReferencedColumns doc and clean up post-consolidation 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. --- .../hbase/index/IndexRegionObserver.java | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java index 039b4ce684a..89ce2ceb96a 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java @@ -971,7 +971,8 @@ private static void setTimestampOnMutation(Mutation m, long ts) throws IOExcepti * cell keeps its original timestamp while the index side is rebuilt at {@code batchTimestamp}. A * later major compaction then opens a {@code >ttl} gap on the data side but not the index side, * dropping the value only on the data table, so the data row and index row diverge. To close it, - * for every index-enabled non-atomic Put we inject each non-PK column referenced by any index + * for every index-enabled Put — a plain upsert or a reconstructed atomic / ON DUPLICATE KEY + * upsert alike — we inject each non-PK column referenced by any index * ({@link IndexMaintainer#getAllColumnsForDataTable()}, the union of indexed and covered columns) * that the touch left unchanged, sourcing the value from the merged next row state * ({@code dataRowStates.getSecond()}). @@ -1066,10 +1067,7 @@ private void rewriteIndexReferencedColumns(MiniBatchOperationInProgress getIndexReferencedColumns( PhoenixIndexMetaData indexMetaData) { @@ -2505,27 +2503,21 @@ private List generateOnDupMutations(BatchMutateContext context, Put at // true, will be null if there is no expression on this column, otherwise false Map> currColumnCellExprMap = new HashMap<>(); - // Snapshot of the current data row's cells, keyed by ColumnReference. Populated once, up front, - // before any conditional update logic mutates state. It answers an OLD_ROW return request. It - // is a local; only exposed on the context for an OLD_ROW return request (unchanged semantics). - Map> oldRowColumnCellExprMap = new HashMap<>(); - byte[] rowKey = atomicPut.getRow(); ImmutableBytesPtr rowKeyPtr = new ImmutableBytesPtr(rowKey); // Get the latest data row state Pair dataRowState = context.dataRowStates.get(rowKeyPtr); Put currentDataRowState = dataRowState != null ? dataRowState.getFirst() : null; - // Capture the original row state (no-op when currentDataRowState is null for a new row). - updateCurrColumnCellExpr(currentDataRowState, oldRowColumnCellExprMap); + // Create separate map for old row data when OLD_ROW is requested + // This must be done before any conditional update logic to preserve original state if (context.returnResult && context.returnOldRow && currentDataRowState != null) { - context.oldRowColumnCellExprMap = oldRowColumnCellExprMap; + context.oldRowColumnCellExprMap = new HashMap<>(); + updateCurrColumnCellExpr(currentDataRowState, context.oldRowColumnCellExprMap); } // if result needs to be returned but the DML does not have ON DUPLICATE KEY present, - // perform the mutation and return the result. Any index-referenced column this upsert omits is - // re-persisted later by rewriteIndexReferencedColumns, which now also covers atomic / - // returnResult Puts once they have been merged into the batch's next row state. + // perform the mutation and return the result. if (opBytes == null) { mutations.add(atomicPut); updateCurrColumnCellExpr(currentDataRowState != null ? currentDataRowState : atomicPut, From 5f13615ac8c02532b9f2cdce47d9cbd084b3a35f Mon Sep 17 00:00:00 2001 From: Sanjeet Malhotra Date: Tue, 7 Jul 2026 18:47:39 +0530 Subject: [PATCH 7/9] Add IndexDataTableTTLBoundaryConsistencyIT regression test for TTL-boundary 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. --- ...ndexDataTableTTLBoundaryConsistencyIT.java | 458 ++++++++++++++++++ 1 file changed, 458 insertions(+) create mode 100644 phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexDataTableTTLBoundaryConsistencyIT.java diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexDataTableTTLBoundaryConsistencyIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexDataTableTTLBoundaryConsistencyIT.java new file mode 100644 index 00000000000..56226f033e4 --- /dev/null +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexDataTableTTLBoundaryConsistencyIT.java @@ -0,0 +1,458 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.end2end; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import java.io.IOException; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.CellUtil; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.ConnectionFactory; +import org.apache.hadoop.hbase.client.Result; +import org.apache.hadoop.hbase.client.ResultScanner; +import org.apache.hadoop.hbase.client.Scan; +import org.apache.hadoop.hbase.client.Table; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants; +import org.apache.phoenix.jdbc.PhoenixConnection; +import org.apache.phoenix.query.BaseTest; +import org.apache.phoenix.schema.PTable; +import org.apache.phoenix.schema.PTableKey; +import org.apache.phoenix.util.EnvironmentEdgeManager; +import org.apache.phoenix.util.ManualEnvironmentEdge; +import org.apache.phoenix.util.ReadOnlyProps; +import org.apache.phoenix.util.TestUtil; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Regression coverage for the production defect from the RCA "Why LEFT_ID & LEFT_ID_SPACE are NULL in + * ID_MAPPING_V2 data table but non NULL in index table": it reproduces the divergence against pre-fix + * code and verifies the fix keeps the data table and its index consistent. + *

    + * The schema is {@code IDMAPPER.ID_MAPPING_V2}: PK (organization_id, map_name, map_id), + * MULTI_TENANT, a NON-FOREVER TTL, and a global covered index on (map_name, left_id, right_id) + * INCLUDE (map_id_space, right_id_space, left_id_space). The id-mapper client refreshes a row's TTL + * by issuing a partial self-referential {@code UPSERT SELECT} that names only + * (organization_id, map_name, map_id, map_id_space) - it does NOT name left_id / left_id_space. + *

    + * Mechanism (all confirmed against the code): + *

      + *
    1. The initial full row lands with every cell at one timestamp {@code T0}.
      2. + *
    2. The partial touch is READ while the row is still alive (at {@code T0 + TTL - 1}, before the + * boundary), but its mutation is committed a couple of ticks later, at + * {@code T_commit = T0 + TTL + 1}. The touch names neither left_id nor left_id_space, so as + * written it carries only map_id_space + the empty column, both landing at {@code T_commit}.
      3. + *
    3. PRE-FIX (the bug): index maintenance in IndexRegionObserver reads the current data row via an + * internal scan that is never TTL-masked, so it sees left_id / left_id_space (still physically + * present at {@code T0}) as live, carries them forward, and rebuilds the whole index row at the + * single fresh {@code T_commit}. The DATA row, meanwhile, still holds those cells only at + * {@code T0}: {@code maxTs - minTs = T_commit - T0 > TTL}, so {@code TTLRegionScanner}'s gap + * analysis trims them and a data-side SELECT reads NULL (major compaction later purges them + * physically). Index keeps left_id / left_id_space, data drops them - the production divergence + * (data NULL, index non-null).
      4. + *
    4. POST-FIX: two coordinated changes keep the two tables aligned - (a) the internal current-row + * read is TTL-masked exactly like a client read (so the index is never rebuilt off + * logically-expired data), and (b) the index-referenced columns are re-persisted into the data + * mutation so, when they are carried forward at all, data and index carry them at the same + * {@code T_commit}. Net effect: left_id / left_id_space are trimmed-or-retained as a UNIT on + * both sides. Because this touch commits one tick PAST the boundary, gap analysis trims them on + * both, so data and index both read NULL - in agreement. (Had the touch landed BEFORE the + * boundary, or in a dense sub-TTL stream, both sides would instead retain the value - still in + * agreement.)
      5. + *
    + * This is a REGRESSION test asserting agreement, robust to whichever route the boundary takes: each + * test is GREEN when the data table and the index agree on left_id / left_id_space for the same + * logical row (the fixed behavior), and RED when they diverge (the pre-fix bug: data NULL, index + * non-null). It deliberately does not pin a specific retain-vs-drop value, so it is deterministic + * regardless of which side of the boundary the single touch lands on. + */ +@Category(NeedsOwnMiniClusterTest.class) +public class IndexDataTableTTLBoundaryConsistencyIT extends BaseTest { + + private static final Logger LOGGER = + LoggerFactory.getLogger(IndexDataTableTTLBoundaryConsistencyIT.class); + + private static final String SCHEMA_NAME = "IDMAPPER"; + + // Unique per test method (assigned in setUpSchema). Hardcoded names would let one method's + // future-dated cells (the injected clock commits ~TTL ahead of real wall time) survive the next + // method's DROP on the shared per-class mini-cluster and pollute its physical table. + private String tableName; + private String indexName; + + // Short, NON-FOREVER TTL so the injected clock can cross it deterministically. A FOREVER TTL + // would short-circuit TTLRegionScanner/CompactionScanner and the bug could not reproduce. + private static final int TTL_SECONDS = 30; + private static final long TTL_MS = TTL_SECONDS * 1000L; + + private static final String TENANT_ID = "00Dxx0000001gER"; // VARCHAR(15) + private static final String MAP_NAME = "MY_MAP"; + private static final String MAP_ID = "uuid:abc"; + + private static final String MAP_ID_SPACE = "SPACE_1"; // the touched (named) covered column + private static final String LEFT_ID = "LEFT_A"; // index key column, distinctive value + private static final String LEFT_ID_SPACE = "LEFTSPACE_1"; // covered column, distinctive value + + private ManualEnvironmentEdge injectEdge; + + @BeforeClass + public static synchronized void doSetup() throws Exception { + // max-lookback must be 0, otherwise the old (T0) row version is retained through major + // compaction for the lookback window and the data-table cells are never physically purged. + Map props = new HashMap<>(); + props.put(BaseScannerRegionObserverConstants.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, + Integer.toString(0)); + // Speed up compaction scheduling in the mini-cluster. + props.put("hbase.procedure.remote.dispatcher.delay.msec", "0"); + setUpTestDriver(new ReadOnlyProps(props.entrySet().iterator())); + } + + @Before + public void setUpSchema() throws Exception { + // DDL runs on the real clock; the manual edge is injected per-scenario, after the schema + // exists, so it only governs data mutation / scan timestamps. + EnvironmentEdgeManager.reset(); + // Unique physical tables per method: the injected clock commits cells ~TTL into the future, + // so a shared hardcoded table would let one method's future-dated cells outlive the next + // method's DROP (run at real wall time) and bridge its TTL gap, corrupting the scenario. + tableName = SCHEMA_NAME + "." + generateUniqueName(); + indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl()); + Statement stmt = conn.createStatement()) { + conn.setAutoCommit(true); + stmt.execute("CREATE TABLE IF NOT EXISTS " + tableName + " (\n" + + " organization_id VARCHAR(15) NOT NULL,\n" + + " map_name VARCHAR(80) NOT NULL,\n" + + " map_id_space VARCHAR(255),\n" + + " map_id VARCHAR(2047) NOT NULL,\n" + + " left_id_space VARCHAR(255),\n" + + " left_id VARCHAR(2047),\n" + + " right_id_space VARCHAR(255),\n" + + " right_id VARCHAR(2047),\n" + + " CONSTRAINT PK PRIMARY KEY (organization_id, map_name, map_id)\n" + + ") TTL=" + TTL_SECONDS + ", MULTI_TENANT=true, REPLICATION_SCOPE=1"); + stmt.execute("CREATE INDEX " + indexName + "\n" + + " ON " + tableName + " (map_name, left_id, right_id)\n" + + " INCLUDE (map_id_space, right_id_space, left_id_space)"); + } + injectEdge = new ManualEnvironmentEdge(); + } + + @After + public void tearDown() { + EnvironmentEdgeManager.reset(); + } + + /** Full initial row: PK + map_id_space + non-null left_id / left_id_space; right_* NULL. */ + private void insertInitialRow(Connection conn) throws Exception { + try (PreparedStatement ps = conn.prepareStatement("UPSERT INTO " + tableName + + " (organization_id, map_name, map_id, map_id_space, left_id, left_id_space, " + + "right_id, right_id_space) VALUES (?, ?, ?, ?, ?, ?, NULL, NULL)")) { + ps.setString(1, TENANT_ID); + ps.setString(2, MAP_NAME); + ps.setString(3, MAP_ID); + ps.setString(4, MAP_ID_SPACE); + ps.setString(5, LEFT_ID); + ps.setString(6, LEFT_ID_SPACE); + ps.executeUpdate(); + } + } + + /** + * The exact id-mapper "mappings in use" touch: a self-referential UPSERT SELECT naming only + * PK + map_id_space. Only {@code executeUpdate()} - the inner SELECT runs and the mutation is + * buffered here; the caller controls when (and at which timestamp) it commits. + */ + private void touchWithUpsertSelect(Connection conn) throws Exception { + try (PreparedStatement ps = conn.prepareStatement("UPSERT INTO " + tableName + + " (organization_id, map_name, map_id, map_id_space) " + + "SELECT organization_id, map_name, map_id, map_id_space FROM " + tableName + + " WHERE organization_id = ? AND map_name = ? AND map_id = ?")) { + ps.setString(1, TENANT_ID); + ps.setString(2, MAP_NAME); + ps.setString(3, MAP_ID); + int affected = ps.executeUpdate(); + LOGGER.info("UPSERT SELECT touch affected {} row(s)", affected); + assertEquals("UPSERT SELECT must read the still-alive row and buffer exactly one touch", + 1, affected); + } + } + + /** Plain partial UPSERT touch naming only PK + map_id_space. Buffered only (no commit here). */ + private void touchWithPlainUpsert(Connection conn) throws Exception { + try (PreparedStatement ps = conn.prepareStatement("UPSERT INTO " + tableName + + " (organization_id, map_name, map_id, map_id_space) VALUES (?, ?, ?, ?)")) { + ps.setString(1, TENANT_ID); + ps.setString(2, MAP_NAME); + ps.setString(3, MAP_ID); + ps.setString(4, MAP_ID_SPACE); + ps.executeUpdate(); + } + } + + /** Resolves the physical HBase name of a Phoenix table/index. */ + private byte[] physicalName(Connection conn, String phoenixName, boolean isIndex) + throws Exception { + PhoenixConnection pconn = conn.unwrap(PhoenixConnection.class); + PTable dataTable = pconn.getTable(new PTableKey(null, tableName)); + if (!isIndex) { + return dataTable.getPhysicalName().getBytes(); + } + for (PTable index : dataTable.getIndexes()) { + if (index.getTableName().getString().equals(phoenixName)) { + return index.getPhysicalName().getBytes(); + } + } + fail("Could not resolve physical name for index " + phoenixName); + return null; // unreachable + } + + /** Live-cell snapshot of a single physical row: its key (binary) and all cell values as UTF-8. */ + private static final class RawRow { + final int rowCount; + final String rowKeyBinary; + final List cellValues; + + RawRow(int rowCount, String rowKeyBinary, List cellValues) { + this.rowCount = rowCount; + this.rowKeyBinary = rowKeyBinary; + this.cellValues = cellValues; + } + } + + /** + * Raw-scans a physical table for live cells only (latest version, non-deleted), expecting at + * most one row, and returns its key plus the UTF-8 rendering of every cell value. Column + * qualifiers are opaque under the default encoding, so callers match cells by their distinctive + * value (LEFT_A, LEFTSPACE_1, SPACE_1). + */ + private RawRow rawScanSingleRow(byte[] physicalTable) throws Exception { + List rows = new ArrayList<>(); + try (org.apache.hadoop.hbase.client.Connection hconn = + ConnectionFactory.createConnection(getUtility().getConfiguration()); + Table htable = hconn.getTable(TableName.valueOf(physicalTable))) { + Scan scan = new Scan(); + scan.setRaw(false); // live cells only, latest version of each + try (ResultScanner scanner = htable.getScanner(scan)) { + for (Result r = scanner.next(); r != null; r = scanner.next()) { + rows.add(r); + } + } + } + if (rows.size() != 1) { + return new RawRow(rows.size(), null, new ArrayList<>()); + } + Result row = rows.get(0); + String rowKeyBinary = Bytes.toStringBinary(CellUtil.cloneRow(row.rawCells()[0])); + List values = new ArrayList<>(); + for (Cell cell : row.rawCells()) { + values.add(Bytes.toString(CellUtil.cloneValue(cell))); + } + return new RawRow(1, rowKeyBinary, values); + } + + private void flush(byte[] physicalTable) throws IOException { + getUtility().getAdmin().flush(TableName.valueOf(physicalTable)); + } + + private void majorCompact(byte[] physicalTable) throws Exception { + TestUtil.majorCompact(getUtility(), TableName.valueOf(physicalTable)); + } + + /** + * @param useUpsertSelect when true the touch is the production self-referential UPSERT SELECT; + * when false it is a plain partial UPSERT (a control that drives the + * identical data/index (dis)agreement, isolating the cause as the + * partiality of the write, not the SELECT read side). + */ + private void runScenario(boolean useUpsertSelect) throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + + // Freeze the clock at T0 and write the full initial row; every cell lands at T0. + long t0 = EnvironmentEdgeManager.currentTimeMillis(); + injectEdge.setValue(t0); + EnvironmentEdgeManager.injectEdge(injectEdge); + + insertInitialRow(conn); + conn.commit(); + + byte[] dataPhysical = physicalName(conn, tableName, false); + byte[] indexPhysical = physicalName(conn, indexName, true); + + // Push the T0 cells onto an HFile so the later major compaction merges two files. + flush(dataPhysical); + flush(indexPhysical); + + injectEdge.incrementValue(TTL_MS - 1); + long tRead = injectEdge.currentTime(); + if (useUpsertSelect) { + touchWithUpsertSelect(conn); + } else { + touchWithPlainUpsert(conn); + } + // NOTE: no flush / compaction happens between the touch and its commit, so (for the + // UPSERT SELECT variant) the inner SELECT read cannot be disturbed mid-statement. + + // --- Advance two ticks (past the boundary) and commit: as written, the touch cells + // (map_id_space + empty) land at T_commit = T0 + TTL + 1, while left_id / left_id_space + // stay at T0. The gap on the data row is now (TTL + 1) > TTL. PRE-FIX, gap analysis trims + // that on the DATA side while the index (rebuilt off an unmasked read at T_commit) keeps + // it -> divergence. POST-FIX, the masked internal read also drops it on the INDEX side + // and the column re-persist keeps the two aligned -> both trimmed, in agreement. --- + injectEdge.incrementValue(2); + long tCommit = injectEdge.currentTime(); + conn.commit(); + + LOGGER.info("t0={} tRead={} tCommit={} gap={}ms ttl={}ms", t0, tRead, tCommit, + tCommit - t0, TTL_MS); + + // Major-compact both tables so any gap-analysis trimming is applied physically, not just + // masked at read time. This is the step that exposes the pre-fix divergence: PRE-FIX the + // data row loses left_id while the index keeps it; POST-FIX both are trimmed together. + flush(dataPhysical); + flush(indexPhysical); + majorCompact(dataPhysical); + majorCompact(indexPhysical); + + // ============ ASSERTIONS: data and index must AGREE (regression guard) ============ + // The bug is a DISAGREEMENT between the data table and its index on left_id / + // left_id_space for the same logical row. This test pins agreement, NOT a specific + // retain-vs-drop value, so it is deterministic regardless of which side of the TTL + // boundary the single touch lands on: + // - PRE-FIX -> data reads NULL (T0 cells gap-trimmed) while the index still carries + // the value rebuilt at T_commit -> NOT equal -> RED. + // - POST-FIX -> both drop it (commit is past the boundary) or both keep it (touch + // before the boundary / dense stream) -> equal -> GREEN. + + // (1) Read left_id / left_id_space from the DATA table (data path). + String dataLeftId; + String dataLeftIdSpace; + String dataMapIdSpace; + try (PreparedStatement ps = conn.prepareStatement( + "SELECT left_id, left_id_space, map_id_space FROM " + tableName + + " WHERE organization_id = ? AND map_name = ? AND map_id = ?")) { + ps.setString(1, TENANT_ID); + ps.setString(2, MAP_NAME); + ps.setString(3, MAP_ID); + try (ResultSet rs = ps.executeQuery()) { + assertTrue("DATA row must still exist (map_id_space touch keeps the row alive)", + rs.next()); + dataLeftId = rs.getString("left_id"); + dataLeftIdSpace = rs.getString("left_id_space"); + dataMapIdSpace = rs.getString("map_id_space"); + } + } + + // (2) Read left_id / left_id_space for the same row through the INDEX (forced index). + String indexLeftId; + String indexLeftIdSpace; + try (PreparedStatement ps = conn.prepareStatement( + "SELECT /*+ INDEX(" + tableName + " " + indexName + ") */ left_id, left_id_space " + + "FROM " + tableName + + " WHERE organization_id = ? AND map_name = ?")) { + ps.setString(1, TENANT_ID); + ps.setString(2, MAP_NAME); + try (ResultSet rs = ps.executeQuery()) { + assertTrue("INDEX query must return the row", rs.next()); + indexLeftId = rs.getString("left_id"); + indexLeftIdSpace = rs.getString("left_id_space"); + } + } + + System.out.println("\n===== TTL-boundary consistency (" + + (useUpsertSelect ? "UPSERT SELECT" : "plain UPSERT") + ") ====="); + System.out.println("DATA left_id=" + dataLeftId + " left_id_space=" + dataLeftIdSpace + + " map_id_space=" + dataMapIdSpace); + System.out.println("INDEX left_id=" + indexLeftId + " left_id_space=" + indexLeftIdSpace); + + // (3) False-negative guard: the touched column must survive on the data side, proving the + // row itself did NOT wholesale-expire (which would make a trivial NULL==NULL pass). + assertEquals("DATA map_id_space must survive at T_commit (row is alive, not expired)", + MAP_ID_SPACE, dataMapIdSpace); + + // (4) Query-layer agreement: data and index must read the SAME value for each column, + // whether that value is the retained one or NULL. This is the core regression guard. + assertEquals("left_id must agree between DATA and INDEX (data=" + dataLeftId + " index=" + + indexLeftId + ")", dataLeftId, indexLeftId); + assertEquals("left_id_space must agree between DATA and INDEX (data=" + dataLeftIdSpace + + " index=" + indexLeftIdSpace + ")", dataLeftIdSpace, indexLeftIdSpace); + + // (5) Physical-layer agreement (defense in depth): the presence of the left_id value in + // the index (row key embeds it) must match its presence as a live data cell. This + // catches a divergence that could hide behind query-layer masking. + RawRow dataRaw = rawScanSingleRow(dataPhysical); + RawRow indexRaw = rawScanSingleRow(indexPhysical); + assertEquals("DATA physical table must have exactly one live row", 1, dataRaw.rowCount); + assertEquals("INDEX physical table must have exactly one live row", 1, indexRaw.rowCount); + System.out.println("DATA rawcells values=" + dataRaw.cellValues); + System.out.println("INDEX rowkey=" + indexRaw.rowKeyBinary); + System.out.println("INDEX rawcells values=" + indexRaw.cellValues); + System.out.println("=====================================================\n"); + boolean dataHasLeftId = dataRaw.cellValues.contains(LEFT_ID); + boolean indexEmbedsLeftId = indexRaw.rowKeyBinary.contains(LEFT_ID); + assertEquals("left_id presence must agree between DATA cell (" + dataHasLeftId + + ") and INDEX row key (" + indexEmbedsLeftId + ")", dataHasLeftId, + indexEmbedsLeftId); + boolean dataHasLeftIdSpace = dataRaw.cellValues.contains(LEFT_ID_SPACE); + boolean indexHasLeftIdSpace = indexRaw.cellValues.contains(LEFT_ID_SPACE); + assertEquals("left_id_space presence must agree between DATA cell (" + dataHasLeftIdSpace + + ") and INDEX covered cell (" + indexHasLeftIdSpace + ")", dataHasLeftIdSpace, + indexHasLeftIdSpace); + } + } + + /** + * The production self-referential UPSERT SELECT touch landing across the TTL boundary. GREEN when + * the data table and the index agree on left_id / left_id_space (fixed); RED on the pre-fix + * divergence (data NULL, index non-null). + */ + @Test + public void testUpsertSelectTouchNearTtlBoundaryKeepsDataAndIndexConsistent() throws Exception { + runScenario(true); + } + + /** + * Control: a plain partial UPSERT exercises the identical path and must keep the data table and + * the index consistent, isolating the cause as the partiality of the write, not the SELECT read. + */ + @Test + public void testPlainPartialUpsertTouchNearTtlBoundaryKeepsDataAndIndexConsistent() + throws Exception { + runScenario(false); + } +} From 339fe94f80f8edc91c2573e8f19502860625d8f3 Mon Sep 17 00:00:00 2001 From: Sanjeet Malhotra Date: Wed, 15 Jul 2026 03:54:52 +0530 Subject: [PATCH 8/9] Anchor internal current-row masked scan at batchTimestamp; drop rewriteIndexReferencedColumns 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. --- .../org/apache/phoenix/util/ScanUtil.java | 18 +- .../hbase/index/IndexRegionObserver.java | 169 +++++------------- ...ndexDataTableTTLBoundaryConsistencyIT.java | 152 ++++++---------- .../index/IndexDataTableTTLSyncIT.java | 97 +++++----- 4 files changed, 163 insertions(+), 273 deletions(-) diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java b/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java index dd37a2a9d52..573684dd759 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java @@ -1905,11 +1905,19 @@ public static void annotateMutationWithLiteralTTL(PhoenixConnection connection, // thread. Only a literal TTL (finite or FOREVER) reaches the internal-scan masking path. return; } - if (table.isImmutableRows()) { - // optimization for immutable tables since we don't need to read the current row - // before writing - return; - } + // NOTE: unlike annotateMutationWithConditionalTTL, we do NOT skip immutable tables here. For + // conditional TTL the server reads the current row only when context.hasConditionalTTL, which is + // itself derived from the _TTL mutation attribute this annotation would set - so skipping + // immutable tables is self-consistent (no attribute => no conditional read). For a LITERAL TTL + // the server-side current-row read in IndexRegionObserver.getCurrentRowStates is triggered by + // table/index structure and mutation type, NOT by any attribute set here: a global index whose + // data/index storage schemes differ leaves context.immutableRows false (see + // identifyIndexMaintainerTypes) and reads the row to rebuild the full index entry, and the + // atomic / returnResult / row-delete paths read it regardless of immutability. If we skipped + // immutable tables, that read would be unmasked and could rebuild the index from TTL-expired + // cells - the very divergence this masking exists to prevent. The threaded attributes are inert + // (they only enable masking on a scan TTLRegionScanner independently gates) when no current-row + // read happens, so annotating immutable tables is safe. // For a view, honor the view-TTL feature flag exactly as setScanAttributesForPhoenixTTL does. boolean isView = table.getType() == PTableType.VIEW; diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java index 89ce2ceb96a..884e2fe2833 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java @@ -966,121 +966,6 @@ private static void setTimestampOnMutation(Mutation m, long ts) throws IOExcepti } } - /** - * On a partial "touch" upsert that does not re-write an index-referenced column, the data-side - * cell keeps its original timestamp while the index side is rebuilt at {@code batchTimestamp}. A - * later major compaction then opens a {@code >ttl} gap on the data side but not the index side, - * dropping the value only on the data table, so the data row and index row diverge. To close it, - * for every index-enabled Put — a plain upsert or a reconstructed atomic / ON DUPLICATE KEY - * upsert alike — we inject each non-PK column referenced by any index - * ({@link IndexMaintainer#getAllColumnsForDataTable()}, the union of indexed and covered columns) - * that the touch left unchanged, sourcing the value from the merged next row state - * ({@code dataRowStates.getSecond()}). - * The cells are added directly at {@code batchTimestamp} (this runs after {@link #setTimestamps}), - * making the data-side and index-side cells share both the timestamp and the empty-column - * neighbor. Any compaction then feeds identical timestamps into the same gap analysis on both - * sides. - *

    - * The value is sourced from the merged next row state rather than the raw current row so that a - * column the touch sets to NULL is honored: such a column is emitted as a separate {@code Delete} - * (not a cell in the Put) and is therefore absent from {@code getSecond()} after - * {@code applyPendingDeleteMutations}, so it is correctly not re-persisted. Reading from the - * current row ({@code getFirst()}) would instead resurrect the just-deleted value. Conversely, a - * column the touch neither writes nor deletes is carried over into {@code getSecond()} from the - * current row and is the one that must be re-persisted. Because the index is likewise generated - * from {@code getSecond()}, both sides always agree on the value. - *

    - * Both covered global indexes and uncovered global indexes are handled. A covered index stores - * the value as an index cell rebuilt at {@code batchTimestamp}; an uncovered index encodes its - * indexed columns positionally into the index row key, also rebuilt at {@code batchTimestamp}. - * Either way the index side survives a compaction that trims the stale data-side cell, so the - * indexed column must be re-persisted on the data side for both — otherwise an uncovered index's - * live row silently drops out of an indexed-column predicate (the read path re-verifies the key - * against the data row and excludes, rather than resurrects, the trimmed value). - *

    - * This runs after {@code prepareDataRowStates}, so {@code getSecond()} reflects this batch's puts - * and deletes. A conditionally-expired row was reset by {@code updateMutationsForConditionalTTL} - * ({@code dataRowStates.put(key, null)}), so its rebuilt next state contains only the touch's own - * columns and nothing extra is re-persisted; a full row delete leaves {@code getSecond() == null}, - * which the guard skips. Atomic / ON DUPLICATE KEY Puts are handled here too: {@code - * generateOnDupMutations} reconstructs them and merges the result into the in-batch Put (and thus - * into its next row state) before this runs, so sourcing from {@code getSecond()} re-persists the - * index-referenced columns they omit — including correctly honoring a column an ON DUPLICATE KEY - * UPDATE set to NULL, which a snapshot of the pre-upsert row would instead resurrect. A no-op - * atomic op is skipped via the {@code isAtomicOperationComplete} guard (nothing is written), and - * immutable tables no-op because no current-row read happened. - */ - private void rewriteIndexReferencedColumns(MiniBatchOperationInProgress miniBatchOp, - BatchMutateContext context, PhoenixIndexMetaData indexMetaData, long batchTimestamp) - throws IOException { - if ((!context.hasGlobalIndex && !context.hasUncoveredIndex) || context.dataRowStates == null) { - // Only covered global or uncovered global indexes can diverge this way; without a current-row - // read there is nothing to re-persist from. - return; - } - Set indexReferencedCols = getIndexReferencedColumns(indexMetaData); - if (indexReferencedCols.isEmpty()) { - return; - } - for (int i = 0; i < miniBatchOp.size(); i++) { - // Skip completed atomic ops (ON DUPLICATE KEY IGNORE on an existing row, or a no-op ON - // DUPLICATE KEY UPDATE): nothing is written for them, so there is nothing to re-persist and - // injecting would turn a genuine no-op into a spurious write. A non-no-op atomic Put is not - // complete here; its generateOnDupMutations result has already been merged into the in-batch - // Put and its next row state, so it is processed like any other Put below. - if (isAtomicOperationComplete(miniBatchOp.getOperationStatus(i))) { - continue; - } - Mutation m = miniBatchOp.getOperation(i); - if (!(m instanceof Put) || !this.builder.isEnabled(m)) { - continue; - } - Pair rowState = context.dataRowStates.get(new ImmutableBytesPtr(m.getRow())); - Put next = rowState != null ? rowState.getSecond() : null; - if (next == null) { - // Expired/masked row, a brand new row with no carried-over state, or a row fully deleted in - // this batch: nothing to re-persist. - continue; - } - Put put = (Put) m; - for (ColumnReference ref : indexReferencedCols) { - byte[] family = ref.getFamily(); - byte[] qualifier = ref.getQualifier(); - if (put.has(family, qualifier)) { - // The touch already writes this column; leave it (idempotence guard). - continue; - } - List nextCells = next.get(family, qualifier); - if (nextCells == null || nextCells.isEmpty()) { - // Absent from the merged next state: either never set, or set to NULL by this touch - // (a Delete removed it). Either way it must not be re-persisted. - continue; - } - // Re-persist the merged value directly at batchTimestamp (setTimestamps has already run), - // so the data-side cell matches the index-side cell built at the same timestamp. - Cell nextCell = nextCells.get(0); - put.addColumn(family, qualifier, batchTimestamp, CellUtil.cloneValue(nextCell)); - } - } - } - - /** - * Union of non-PK columns referenced by any covered or uncovered global index (indexed + - * covered). PK columns live in the row key, never as cells in a Put, so they are naturally - * excluded by callers' has()/get() checks. - */ - private static Set getIndexReferencedColumns( - PhoenixIndexMetaData indexMetaData) { - Set indexReferencedCols = new HashSet<>(); - for (IndexMaintainer maintainer : indexMetaData.getIndexMaintainers()) { - if (maintainer.isLocalIndex()) { - continue; - } - indexReferencedCols.addAll(maintainer.getAllColumnsForDataTable()); - } - return indexReferencedCols; - } - /** * This method applies pending delete mutations on the next row states */ @@ -1306,9 +1191,22 @@ private boolean isPartialUncoveredIndexMutation(PhoenixIndexMetaData indexMetaDa * scan is also given the client read path's server-paging setup (a {@code SERVER_PAGE_SIZE_MS} * attribute and a {@code PagingFilter} wrap), so it is bounded by the server page budget just like * a client scan; {@code readDataTableRows} skips the dummy results that paging can emit. + *

    + * The scan's time range is set to {@code [0, batchTimestamp)} so {@link TTLRegionScanner} anchors + * its masking clock ({@code currentTime = scan.getTimeRange().getMax()}) at {@code batchTimestamp} + * — the exact timestamp at which the index side is rebuilt — rather than at the scan-open wall + * clock. On a partial "touch" upsert, {@code getBatchTimestamp} bumps {@code batchTimestamp} + * slightly past that wall clock to avoid timestamp collisions, so a cell whose TTL boundary falls + * in that sub-millisecond window would otherwise be masked here (as of the scan-open instant) yet + * be visible to the index build (as of {@code batchTimestamp}), leaving the data row and index row + * inconsistent. Anchoring both at {@code batchTimestamp} makes the read trim expired cells as of + * exactly the timestamp the index is built at, so the two never disagree. The half-open range + * drops nothing current: every pre-existing cell of a locked row was written by an earlier batch + * at {@code ts < batchTimestamp} under the Phoenix write path. */ private void getCurrentRowStates(ObserverContext c, - BatchMutateContext context, byte[] literalTTLForScan, boolean isStrictTTL) throws IOException { + BatchMutateContext context, byte[] literalTTLForScan, boolean isStrictTTL, long batchTimestamp) + throws IOException { Set keys = new HashSet(context.rowsToLock.size()); for (ImmutableBytesPtr rowKeyPtr : context.rowsToLock) { PendingRow pendingRow = new PendingRow(rowKeyPtr, context); @@ -1377,6 +1275,10 @@ private void getCurrentRowStates(ObserverContext c // for bloom filters scan should be a get scan.withStartRow(key.getLowerRange(), true); scan.withStopRow(key.getLowerRange(), true); + // Anchor TTLRegionScanner's masking clock at batchTimestamp (see method comment): the + // half-open [0, batchTimestamp) range makes getTimeRange().getMax() == batchTimestamp, so + // the read trims expired cells as of exactly the timestamp the index is built at. + scan.setTimeRange(0, batchTimestamp); ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, emptyCF, emptyCQ, literalTTLForScan, isStrictTTL); readDataTableRows(c, context, scan); @@ -1387,6 +1289,10 @@ private void getCurrentRowStates(ObserverContext c scanRanges.initializeScan(scan); SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter(); scan.setFilter(skipScanFilter); + // Anchor TTLRegionScanner's masking clock at batchTimestamp (see method comment): the + // half-open [0, batchTimestamp) range makes getTimeRange().getMax() == batchTimestamp, so + // the read trims expired cells as of exactly the timestamp the index is built at. + scan.setTimeRange(0, batchTimestamp); ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, emptyCF, emptyCQ, literalTTLForScan, isStrictTTL); readDataTableRows(c, context, scan); @@ -2008,13 +1914,19 @@ private long getBatchTimestamp(BatchMutateContext context, TableName table) // The timestamp for this batch will be different from the last batch processed. lastTimestamp = ts; batchesWithLastTimestamp.clear(); - batchesWithLastTimestamp.add(context.rowsToLock); + // Register a snapshot, not the live rowsToLock: this now runs before + // releaseLocksForOnDupIgnoreMutations may structurally mutate rowsToLock (remove) outside + // synchronized(this), which would corrupt a concurrent shouldSleep iteration under + // synchronized(this). The snapshot is a superset of every later state (rowsToLock only + // shrinks after lockRows), so shouldSleep stays conservative: at most an extra sleep, never + // a missed one. + batchesWithLastTimestamp.add(new TreeSet<>(context.rowsToLock)); return ts; } else { if (!shouldSleep(context)) { // There is no need to sleep as the last batches with the same timestamp // do not have a common row this batch - batchesWithLastTimestamp.add(context.rowsToLock); + batchesWithLastTimestamp.add(new TreeSet<>(context.rowsToLock)); return ts; } } @@ -2033,7 +1945,7 @@ private long getBatchTimestamp(BatchMutateContext context, TableName table) // We do not have to check again if we need to sleep again since we got the next // timestamp while holding the row locks. This mean there cannot be a new // mutation with the same row attempting get the same timestamp - batchesWithLastTimestamp.add(context.rowsToLock); + batchesWithLastTimestamp.add(new TreeSet<>(context.rowsToLock)); return ts; } } @@ -2070,6 +1982,14 @@ public void preBatchMutateWithExceptions(ObserverContext TTL}, so {@code TTLRegionScanner}'s gap - * analysis trims them and a data-side SELECT reads NULL (major compaction later purges them - * physically). Index keeps left_id / left_id_space, data drops them - the production divergence - * (data NULL, index non-null). - *

  • POST-FIX: two coordinated changes keep the two tables aligned - (a) the internal current-row - * read is TTL-masked exactly like a client read (so the index is never rebuilt off - * logically-expired data), and (b) the index-referenced columns are re-persisted into the data - * mutation so, when they are carried forward at all, data and index carry them at the same - * {@code T_commit}. Net effect: left_id / left_id_space are trimmed-or-retained as a UNIT on - * both sides. Because this touch commits one tick PAST the boundary, gap analysis trims them on - * both, so data and index both read NULL - in agreement. (Had the touch landed BEFORE the - * boundary, or in a dense sub-TTL stream, both sides would instead retain the value - still in - * agreement.)
    • + * analysis masks them and a data-side SELECT reads NULL. Index returns left_id / left_id_space, + * data reads NULL - the production divergence (data NULL, index non-null). + *
  • POST-FIX: the internal current-row read is TTL-masked as of exactly {@code batchTimestamp} - + * {@code scan.setTimeRange(0, batchTimestamp)} anchors {@code TTLRegionScanner}'s masking clock + * at the very timestamp the index is built at (see {@code IndexRegionObserver.getCurrentRowStates}). + * So the index is rebuilt from the same logically-expired-or-alive view the data side sees: + * left_id / left_id_space are trimmed-or-retained as a UNIT on both sides. Because this touch + * commits one tick PAST the boundary, the anchored read drops them on the index side just as the + * data-side read masks them, so data and index both read NULL - in agreement. (Had the touch + * landed BEFORE the boundary, or in a dense sub-TTL stream, both sides would instead retain the + * value - still in agreement.)
    • * * This is a REGRESSION test asserting agreement, robust to whichever route the boundary takes: each * test is GREEN when the data table and the index agree on left_id / left_id_space for the same * logical row (the fixed behavior), and RED when they diverge (the pre-fix bug: data NULL, index * non-null). It deliberately does not pin a specific retain-vs-drop value, so it is deterministic * regardless of which side of the boundary the single touch lands on. + *

    + * Agreement is asserted at the QUERY layer (both the data-path and index-path SELECTs read through + * {@code TTLRegionScanner}), which is the user-visible guarantee. It is NOT asserted with a raw HBase + * scan: under the production-like non-zero max-lookback this test runs with (see {@code doSetup}), the + * {@code T0} row version lingers physically inside the time-travel window on the data side even after + * major compaction, so a raw (unmasked) scan would report physical state that no query ever observes. */ @Category(NeedsOwnMiniClusterTest.class) public class IndexDataTableTTLBoundaryConsistencyIT extends BaseTest { @@ -131,11 +126,26 @@ public class IndexDataTableTTLBoundaryConsistencyIT extends BaseTest { @BeforeClass public static synchronized void doSetup() throws Exception { - // max-lookback must be 0, otherwise the old (T0) row version is retained through major - // compaction for the lookback window and the data-table cells are never physically purged. + // Run under the production-like config the anchored-masking fix recommends: a non-zero + // max-lookback set just BELOW the TTL. This is deliberately NOT zero. Two reasons: + // 1. It matches how indexed TTL tables are actually configured, and it closes the narrow + // race where a concurrent major compaction could physically collect a pre-existing cell + // between the current-row read and the mutation being persisted. + // 2. It keeps RowContext.setTTL's effective-TTL floor Math.max(ttlInSecs*1000, + // maxLookbackInMillis + 1) equal to TTL_MS: with max-lookback below TTL the floor stays + // at TTL_MS, so the injected (TTL + 1) gap still exceeds the effective TTL and the + // boundary drop path is exercised. Had max-lookback reached or exceeded TTL the floor + // would rise, the gap would no longer exceed it, and the scenario would stop reproducing. + // Note: under a non-zero max-lookback the T0 row version lingers PHYSICALLY inside the + // lookback (time-travel) window on the data side even after major compaction. That is + // expected retention, not divergence: no live query sees it, because every read - data or + // index - is masked by TTLRegionScanner, which trims the (T0, T_commit] gap identically on + // both sides. Consistency is therefore asserted at the query layer (masked reads), not by a + // raw physical scan (which bypasses TTLRegionScanner and would report the unmasked physical + // state that users never observe). Map props = new HashMap<>(); props.put(BaseScannerRegionObserverConstants.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, - Integer.toString(0)); + Integer.toString(TTL_SECONDS - 1)); // Speed up compaction scheduling in the mini-cluster. props.put("hbase.procedure.remote.dispatcher.delay.msec", "0"); setUpTestDriver(new ReadOnlyProps(props.entrySet().iterator())); @@ -241,50 +251,6 @@ private byte[] physicalName(Connection conn, String phoenixName, boolean isIndex return null; // unreachable } - /** Live-cell snapshot of a single physical row: its key (binary) and all cell values as UTF-8. */ - private static final class RawRow { - final int rowCount; - final String rowKeyBinary; - final List cellValues; - - RawRow(int rowCount, String rowKeyBinary, List cellValues) { - this.rowCount = rowCount; - this.rowKeyBinary = rowKeyBinary; - this.cellValues = cellValues; - } - } - - /** - * Raw-scans a physical table for live cells only (latest version, non-deleted), expecting at - * most one row, and returns its key plus the UTF-8 rendering of every cell value. Column - * qualifiers are opaque under the default encoding, so callers match cells by their distinctive - * value (LEFT_A, LEFTSPACE_1, SPACE_1). - */ - private RawRow rawScanSingleRow(byte[] physicalTable) throws Exception { - List rows = new ArrayList<>(); - try (org.apache.hadoop.hbase.client.Connection hconn = - ConnectionFactory.createConnection(getUtility().getConfiguration()); - Table htable = hconn.getTable(TableName.valueOf(physicalTable))) { - Scan scan = new Scan(); - scan.setRaw(false); // live cells only, latest version of each - try (ResultScanner scanner = htable.getScanner(scan)) { - for (Result r = scanner.next(); r != null; r = scanner.next()) { - rows.add(r); - } - } - } - if (rows.size() != 1) { - return new RawRow(rows.size(), null, new ArrayList<>()); - } - Result row = rows.get(0); - String rowKeyBinary = Bytes.toStringBinary(CellUtil.cloneRow(row.rawCells()[0])); - List values = new ArrayList<>(); - for (Cell cell : row.rawCells()) { - values.add(Bytes.toString(CellUtil.cloneValue(cell))); - } - return new RawRow(1, rowKeyBinary, values); - } - private void flush(byte[] physicalTable) throws IOException { getUtility().getAdmin().flush(TableName.valueOf(physicalTable)); } @@ -332,8 +298,10 @@ private void runScenario(boolean useUpsertSelect) throws Exception { // (map_id_space + empty) land at T_commit = T0 + TTL + 1, while left_id / left_id_space // stay at T0. The gap on the data row is now (TTL + 1) > TTL. PRE-FIX, gap analysis trims // that on the DATA side while the index (rebuilt off an unmasked read at T_commit) keeps - // it -> divergence. POST-FIX, the masked internal read also drops it on the INDEX side - // and the column re-persist keeps the two aligned -> both trimmed, in agreement. --- + // it -> divergence. POST-FIX, the internal current-row read is masked as of exactly + // batchTimestamp (scan.setTimeRange(0, batchTimestamp) anchors TTLRegionScanner's clock at + // the timestamp the index is built at), so it trims left_id on the INDEX side identically + // to how a data-side read trims it -> both drop it, in agreement. --- injectEdge.incrementValue(2); long tCommit = injectEdge.currentTime(); conn.commit(); @@ -341,9 +309,13 @@ private void runScenario(boolean useUpsertSelect) throws Exception { LOGGER.info("t0={} tRead={} tCommit={} gap={}ms ttl={}ms", t0, tRead, tCommit, tCommit - t0, TTL_MS); - // Major-compact both tables so any gap-analysis trimming is applied physically, not just - // masked at read time. This is the step that exposes the pre-fix divergence: PRE-FIX the - // data row loses left_id while the index keeps it; POST-FIX both are trimmed together. + // Major-compact both tables to settle each side's physical state before the reads. Note + // that under the non-zero max-lookback (see doSetup) the T0 cells are NOT physically + // purged from the data side - they linger inside the time-travel window - but they remain + // masked from every query by TTLRegionScanner's gap analysis. Consistency is therefore + // observed at the query layer: PRE-FIX the masked data SELECT reads NULL while the index + // (rebuilt off an unmasked internal read at T_commit) still returns the value -> RED; + // POST-FIX the anchored masked internal read drops it on the index side too -> both NULL. flush(dataPhysical); flush(indexPhysical); majorCompact(dataPhysical); @@ -406,33 +378,19 @@ private void runScenario(boolean useUpsertSelect) throws Exception { MAP_ID_SPACE, dataMapIdSpace); // (4) Query-layer agreement: data and index must read the SAME value for each column, - // whether that value is the retained one or NULL. This is the core regression guard. + // whether that value is the retained one or NULL. This is the core regression guard, + // and it is asserted at the query layer on purpose. Both the data-path SELECT and the + // index-path SELECT read through TTLRegionScanner, so both are masked by the SAME gap + // analysis - which is exactly the user-visible consistency guarantee the fix makes. + // A raw HBase scan is deliberately NOT used here: under the production-like non-zero + // max-lookback (see doSetup), the T0 row version lingers PHYSICALLY on the data side + // inside the time-travel window even after major compaction, so a raw (unmasked) scan + // would report a physical state that diverges from what every query sees - a false + // signal, not a divergence. The query-layer check below is the meaningful assertion. assertEquals("left_id must agree between DATA and INDEX (data=" + dataLeftId + " index=" + indexLeftId + ")", dataLeftId, indexLeftId); assertEquals("left_id_space must agree between DATA and INDEX (data=" + dataLeftIdSpace + " index=" + indexLeftIdSpace + ")", dataLeftIdSpace, indexLeftIdSpace); - - // (5) Physical-layer agreement (defense in depth): the presence of the left_id value in - // the index (row key embeds it) must match its presence as a live data cell. This - // catches a divergence that could hide behind query-layer masking. - RawRow dataRaw = rawScanSingleRow(dataPhysical); - RawRow indexRaw = rawScanSingleRow(indexPhysical); - assertEquals("DATA physical table must have exactly one live row", 1, dataRaw.rowCount); - assertEquals("INDEX physical table must have exactly one live row", 1, indexRaw.rowCount); - System.out.println("DATA rawcells values=" + dataRaw.cellValues); - System.out.println("INDEX rowkey=" + indexRaw.rowKeyBinary); - System.out.println("INDEX rawcells values=" + indexRaw.cellValues); - System.out.println("=====================================================\n"); - boolean dataHasLeftId = dataRaw.cellValues.contains(LEFT_ID); - boolean indexEmbedsLeftId = indexRaw.rowKeyBinary.contains(LEFT_ID); - assertEquals("left_id presence must agree between DATA cell (" + dataHasLeftId - + ") and INDEX row key (" + indexEmbedsLeftId + ")", dataHasLeftId, - indexEmbedsLeftId); - boolean dataHasLeftIdSpace = dataRaw.cellValues.contains(LEFT_ID_SPACE); - boolean indexHasLeftIdSpace = indexRaw.cellValues.contains(LEFT_ID_SPACE); - assertEquals("left_id_space presence must agree between DATA cell (" + dataHasLeftIdSpace - + ") and INDEX covered cell (" + indexHasLeftIdSpace + ")", dataHasLeftIdSpace, - indexHasLeftIdSpace); } } diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java index 0b820c588cb..ee491ab464c 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java @@ -62,21 +62,24 @@ /** * Verifies that internal server-side index-maintenance current-row reads honor Phoenix TTL exactly - * like a client read, and that index-referenced data columns are re-persisted so the data table and - * a global index stay aligned under compaction. + * like a client read, and that the read is anchored at the batch timestamp so the data table and a + * global index stay aligned under compaction. *

    * The scenario is the divergence being fixed: a covered column ({@code covcol}) is written once, * then never re-written by later partial "touch" upserts. On the index side {@code covcol} is - * rebuilt at every touch's {@code batchTimestamp}; on the data side it would otherwise keep its - * original timestamp, and a later major compaction opens a {@code >ttl} gap on only the data side, - * dropping {@code covcol} there while the index keeps it. + * rebuilt at every touch's {@code batchTimestamp} from the masked current-row read; on the data side + * {@code covcol} keeps its original timestamp. The fix anchors that internal read's masking clock at + * {@code batchTimestamp} (the same instant the index is built at) via + * {@code scan.setTimeRange(0, batchTimestamp)}, so both sides always agree on whether {@code covcol} + * is still alive: whatever the read masks the index rebuild omits, and whatever the read keeps a + * later major compaction keeps on the data side too. The row and its index then expire as a unit + * rather than {@code covcol} being dropped on only one side. *

    - * The column re-sync re-injects {@code covcol} (and any indexed column) into the touch's data Put at - * {@code HConstants.LATEST_TIMESTAMP}, so {@code setTimestamps} re-stamps it to {@code batchTimestamp} - * and it stays aligned with the index cell. Under a {@link ManualEnvironmentEdge} the server's - * {@code batchTimestamp} equals the touch's wall-clock, so the load-bearing, timing-independent signal - * is: after a covcol-omitting touch, a raw scan's maximum {@code covcol} timestamp on the data side - * advances to the touch time. Pre-fix it stays at the original write time. + * Because the fix no longer re-stamps the data-side cell, the load-bearing, timing-independent + * signal is: after a covcol-omitting touch, a raw scan's maximum {@code covcol} timestamp on the + * data side stays at its original write time (it does NOT advance to the touch time), while the + * data read and the index read still agree on the value. Under a {@link ManualEnvironmentEdge} the + * server's {@code batchTimestamp} equals the touch's wall clock, making this deterministic. *

    * {@link #testNoIndexAtomicUpsertMasksExpiredRow} covers the no-index masking path: an atomic * {@code ON DUPLICATE KEY UPDATE} on a TTL table with no secondary index still triggers an internal @@ -158,8 +161,10 @@ private String withClause(int ttlSeconds, boolean strict) { /** * Base table with a literal TTL and a covered global index. After a partial touch that never - * writes covcol, the fix re-stamps covcol on the data side to the touch's batchTimestamp; a full - * expiry after that shows the data row and the index expiring as a unit. + * writes covcol, the data-side covcol keeps its original timestamp (the fix does not re-stamp it); + * the masked internal read anchored at batchTimestamp keeps the data and index agreeing on covcol + * while the row is alive, and a full expiry after that shows the data row and the index expiring as + * a unit. */ @Test public void testBaseTableCoveredColumnResync() throws Exception { @@ -187,20 +192,21 @@ public void testBaseTableCoveredColumnResync() throws Exception { .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); conn.commit(); - // Load-bearing, deterministic signal: covcol was re-injected into the touch's data Put and - // re-stamped to batchTimestamp (== touchTime under the manual edge). Pre-fix it stays at the - // original write time. + // Load-bearing, deterministic signal: the touch does NOT write covcol and the fix does not + // re-stamp it, so the data-side covcol keeps its original write timestamp - it stays strictly + // below the touch time. (Pre-fix the re-sync advanced it to batchTimestamp == touchTime.) long dataCovTs = maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), COVCOL_VALUE); - assertTrue("covcol should be re-stamped forward on the data side; touchTime=" + touchTime - + " dataCovTs=" + dataCovTs, dataCovTs >= touchTime); + assertTrue("covcol should keep its original write timestamp on the data side; touchTime=" + + touchTime + " dataCovTs=" + dataCovTs, dataCovTs < touchTime); - // Data and index agree on covcol: the masked internal scan and the column re-sync keep them - // aligned. + // Data and index agree on covcol: the internal scan anchored at batchTimestamp masks covcol + // identically to how the index is rebuilt, so both sides see the same live value. assertCovColConsistent(conn, tableName, "r1", "k1", COVCOL_VALUE); - // Advancing past TTL + max-lookback and compacting expires the row on BOTH sides together, - // because post-fix covcol sits at batchTimestamp next to empty@batchTimestamp on each side. + // Advancing past TTL + max-lookback and compacting expires the row on BOTH sides together: the + // whole row version is older than maxLookback + ttl, so compaction purges the data row and the + // index row as a unit. injectEdge.incrementValue((TTL + MAX_LOOKBACK_AGE + 1) * 1000L); flushAndMajorCompact(conn, tableName); flushAndMajorCompact(conn, indexName); @@ -216,14 +222,16 @@ public void testBaseTableCoveredColumnResync() throws Exception { * trim the stale data-side {@code idxcol} while the row stays alive, so the index key encodes a * value the data row no longer has — and because the uncovered read path re-verifies the rebuilt * key against the data row, the live row silently drops out of an {@code idxcol} predicate rather - * than surfacing the stale value. The re-sync must therefore cover uncovered indexes too: it - * re-injects {@code idxcol} into the touch's data Put at {@code LATEST_TIMESTAMP} so - * {@code setTimestamps} advances it to {@code batchTimestamp} alongside the index key. + * than surfacing the stale value. Anchoring the internal read at {@code batchTimestamp} covers + * uncovered indexes too: the index key is rebuilt from the same masked current-row state, so it + * only ever encodes a value the anchored read still sees alive, and compaction converges the data + * side to the same keep/drop decision. *

    * Load-bearing, timing-independent signal (mirrors {@link #testBaseTableCoveredColumnResync}): * after an {@code idxcol}-omitting touch of a still-alive row, a raw scan's maximum {@code idxcol} - * timestamp on the data side advances to the touch time. Pre-fix (uncovered skipped) it stays at - * the original write time. + * timestamp on the data side stays at its original write time (the fix does not re-stamp it), while + * the row is still retrievable through the uncovered index because both sides agree the value is + * alive. */ @Test public void testUncoveredIndexIndexedColumnResync() throws Exception { @@ -245,20 +253,20 @@ public void testUncoveredIndexIndexedColumnResync() throws Exception { conn.commit(); // Partial touch that does not write idxcol, well within the TTL so the row is alive and the - // masked internal scan still returns idxcol for the re-sync to re-persist. + // internal scan anchored at batchTimestamp still returns idxcol for the index rebuild. injectEdge.incrementValue(1000); long touchTime = injectEdge.currentTime(); conn.createStatement() .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); conn.commit(); - // The uncovered index's indexed column was re-injected into the touch's data Put and - // re-stamped to batchTimestamp (== touchTime under the manual edge). Pre-fix it stays at the - // original write time. + // The touch does not write idxcol and the fix does not re-stamp it, so the data-side idxcol + // keeps its original write timestamp - strictly below the touch time. (Pre-fix the re-sync + // advanced it to batchTimestamp == touchTime.) long dataIdxTs = maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), IDXCOL_VALUE); - assertTrue("uncovered idxcol should be re-stamped forward on the data side; touchTime=" - + touchTime + " dataIdxTs=" + dataIdxTs, dataIdxTs >= touchTime); + assertTrue("uncovered idxcol should keep its original write timestamp on the data side; " + + "touchTime=" + touchTime + " dataIdxTs=" + dataIdxTs, dataIdxTs < touchTime); // The row is retrievable through the uncovered index (join-back + key re-verification passes // because the data-side idxcol still encodes the same key) and touchcol reflects the touch. @@ -281,7 +289,8 @@ public void testUncoveredIndexIndexedColumnResync() throws Exception { /** * View with a view-level literal TTL (not on the shared CF descriptor) and a covered index on the * view. Exercises the literal-_TTL per-mutation threading: the internal scan masks with the view's - * TTL, and covcol is re-stamped just like the base-table case. + * TTL, anchored at batchTimestamp, and covcol keeps its original data-side timestamp just like the + * base-table case while the data and index reads stay consistent. */ @Test public void testViewCoveredColumnResync() throws Exception { @@ -313,8 +322,8 @@ public void testViewCoveredColumnResync() throws Exception { long dataCovTs = maxTimestampForValue(conn, TableName.valueOf(baseTableName), Bytes.toBytes("r1"), COVCOL_VALUE); - assertTrue("covcol should be re-stamped forward on the view's data side; touchTime=" - + touchTime + " dataCovTs=" + dataCovTs, dataCovTs >= touchTime); + assertTrue("covcol should keep its original write timestamp on the view's data side; touchTime=" + + touchTime + " dataCovTs=" + dataCovTs, dataCovTs < touchTime); assertCovColConsistent(conn, viewName, "r1", "k1", COVCOL_VALUE); } @@ -322,10 +331,12 @@ public void testViewCoveredColumnResync() throws Exception { /** * A non-strict table must not be over-masked: the strictness flag is threaded to the internal - * scan, so a value that a non-strict client read still returns is retained and re-stamped rather - * than dropped. If strictness were NOT ported, the internal scan would default to strict and mask - * covcol away past the TTL, so the rewrite would find nothing to re-persist and the probe would - * stay at the original timestamp. + * scan, so a value that a non-strict client read still returns is retained rather than masked out + * of the index rebuild. If strictness were NOT ported, the internal scan would default to strict + * and mask covcol away past the TTL, so the index would be rebuilt without covcol and the + * data/index consistency check would fail. The load-bearing check here is therefore + * {@link #assertCovColConsistent}; the raw-scan timestamp probe only confirms the covcol cell is + * still physically present at its original write time (raw scans see cells regardless of masking). */ @Test public void testNonStrictTableNotOverMasked() throws Exception { @@ -355,9 +366,11 @@ public void testNonStrictTableNotOverMasked() throws Exception { long dataCovTs = maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), COVCOL_VALUE); - assertTrue("non-strict covcol should be retained and re-stamped, not masked away; touchTime=" - + touchTime + " dataCovTs=" + dataCovTs, dataCovTs >= touchTime); + assertTrue("non-strict covcol should be retained at its original write timestamp, not masked " + + "away; touchTime=" + touchTime + " dataCovTs=" + dataCovTs, dataCovTs < touchTime); + // The decisive check: a non-strict internal read is not over-masked, so the index is rebuilt + // with covcol and both the data read and the index read still return it. assertCovColConsistent(conn, tableName, "r1", "k1", COVCOL_VALUE); } } From 1b328d182351efe8a43ca32f1c33b0a459075445 Mon Sep 17 00:00:00 2001 From: Sanjeet Malhotra Date: Fri, 17 Jul 2026 01:40:48 +0530 Subject: [PATCH 9/9] Add concurrent-major-compaction regression test; rename and prune TTL-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 339fe94f80). --- .../apache/phoenix/execute/MutationState.java | 5 +- .../query/ConnectionQueryServicesImpl.java | 6 +- .../apache/phoenix/schema/MetaDataClient.java | 6 +- .../org/apache/phoenix/util/ScanUtil.java | 43 +- .../phoenix/coprocessor/ServerScanUtil.java | 27 +- .../hbase/index/IndexRegionObserver.java | 67 +- ...ndexDataTableTTLBoundaryConsistencyIT.java | 416 --------- .../phoenix/end2end/MetaDataEndPointIT.java | 10 +- .../end2end/MetadataGetTableReadLockIT.java | 6 +- .../end2end/index/IndexDataTableSyncIT.java | 796 ++++++++++++++++++ .../index/IndexDataTableTTLSyncIT.java | 538 ------------ .../iterate/PhoenixQueryTimeoutIT.java | 6 +- 12 files changed, 886 insertions(+), 1040 deletions(-) delete mode 100644 phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexDataTableTTLBoundaryConsistencyIT.java create mode 100644 phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableSyncIT.java delete mode 100644 phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java b/phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java index 3b06a664d62..b0bfac0b047 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java @@ -1528,8 +1528,9 @@ private void sendMutations(Iterator>> mutationsI // no-op if table doesn't have Conditional TTL ScanUtil.annotateMutationWithConditionalTTL(connection, tableInfo.getPTable(), mutationList); - // no-op unless table/view has a literal TTL; threads the view's literal TTL and any - // non-strict flag so the server-side internal current-row scan masks like a client read + // 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); // If we haven't retried yet, retry for this case only, as it's possible that // a split will occur after we send the index metadata cache to all known diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java b/phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java index 5a8334ed63e..2c27a57fd37 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java @@ -5471,14 +5471,14 @@ private void createSnapshot(String snapshotName, String tableName) throws SQLExc // is safe. // A second flavor of transient failure is "already running another snapshot on the same table", // produced by RPC-level retries. The original snapshot() RPC has already been accepted by the - // master and the snapshot procedure is in flight, but the client retries the call and the master + // master and the snapshot procedure is in flight, but the client retries the call and the + // master // rejects the duplicate. In that case the existing snapshot procedure will succeed, so we treat // it as success once the snapshot of that name shows up in listSnapshots(). If it never appears // within the polling window we fall through to a normal retry. final int maxAttempts = 5; final long backoffMs = 1000L; - final long alreadyRunningWaitMs = - TimeUnit.MINUTES.toMillis(2L); + final long alreadyRunningWaitMs = TimeUnit.MINUTES.toMillis(2L); SQLException sqlE = null; for (int attempt = 1; attempt <= maxAttempts; attempt++) { sqlE = null; diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java b/phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java index 22833e24945..e70aba774ff 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java @@ -4923,9 +4923,9 @@ public MutationState addColumn(PTable table, List origColumnDefs, /** * To check if TTL is defined at any of the child below we are checking it at * {@link org.apache.phoenix.coprocessor.MetaDataEndpointImpl#mutateColumn(List, ColumnMutator, int, PTable, PTable, boolean)} - * level where in function - * {@link org.apache.phoenix.coprocessor.MetaDataEndpointImpl# validateIfMutationAllowedOnParent(PTable, List, PTableType, long, byte[], byte[], byte[], List, int)} - * we are already traversing through allDescendantViews. + * level where in function {@link org.apache.phoenix.coprocessor.MetaDataEndpointImpl# + * validateIfMutationAllowedOnParent(PTable, List, PTableType, long, byte[], byte[], + * byte[], List, int)} we are already traversing through allDescendantViews. */ } diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java b/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java index 573684dd759..0f40f68ad4e 100644 --- a/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java +++ b/phoenix-core-client/src/main/java/org/apache/phoenix/util/ScanUtil.java @@ -1865,10 +1865,10 @@ public static void annotateMutationWithConditionalTTL(PhoenixConnection connecti /** * Annotates mutations for a table/view with a literal TTL so the server-side internal current-row - * scan (IndexRegionObserver.getCurrentRowStates) can mask expired rows exactly like a client read. - * This is the literal-TTL sibling of {@link #annotateMutationWithConditionalTTL}; the two are - * disjoint (one is guarded on a literal expression, the other on a conditional expression) so at - * most one fires for a given table. + * scan (IndexRegionObserver.getCurrentRowStates) can mask expired rows exactly like a client + * read. This is the literal-TTL sibling of {@link #annotateMutationWithConditionalTTL}; the two + * are disjoint (one is guarded on a literal expression, the other on a conditional expression) so + * at most one fires for a given table. *

    * The guiding principle is to thread precisely what the read path * ({@link #setScanAttributesForPhoenixTTL}) would set as the {@code _TTL} scan attribute: @@ -1884,17 +1884,18 @@ public static void annotateMutationWithConditionalTTL(PhoenixConnection connecti *

  • For either type, {@code IS_STRICT_TTL=false} is set only when the table/view is non-strict, * so absence defaults to strict, matching the read-path convention and avoiding over-masking of a * non-strict table.
    • - *
  • For either type, the empty-column CF/CQ are threaded unconditionally (for any mutable - * literal-TTL table/view, regardless of the TTL value or the view-TTL flag), exactly as the client - * read path does: {@link #setScanAttributesForClient} sets the empty column on every non-analyze - * scan. They merely identify the table's empty column and only enable masking; - * {@link org.apache.phoenix.coprocessor.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 from it. - * They are also the only source of these values for the no-index current-row read (an atomic / ON - * DUPLICATE KEY / {@code returnResult} / row-delete on a TTL table), which has no - * {@code IndexMaintainer} on the server. Derived exactly as the read path derives them via - * {@link SchemaUtil#getEmptyColumnFamily(PTable)} / {@link SchemaUtil#getEmptyColumnQualifier}.
    • + *
  • For either type, the empty-column CF/CQ are threaded unconditionally (for any + * mutable literal-TTL table/view, regardless of the TTL value or the view-TTL flag), exactly as + * the client read path does: {@link #setScanAttributesForClient} sets the empty column on every + * non-analyze scan. They merely identify the table's empty column and only enable masking; + * {@link org.apache.phoenix.coprocessor.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 from it. They are also the only source of these values for the no-index current-row + * read (an atomic / ON DUPLICATE KEY / {@code returnResult} / row-delete on a TTL table), which + * has no {@code IndexMaintainer} on the server. Derived exactly as the read path derives them via + * {@link SchemaUtil#getEmptyColumnFamily(PTable)} / + * {@link SchemaUtil#getEmptyColumnQualifier}.
    • * */ public static void annotateMutationWithLiteralTTL(PhoenixConnection connection, PTable table, @@ -1906,7 +1907,8 @@ public static void annotateMutationWithLiteralTTL(PhoenixConnection connection, return; } // NOTE: unlike annotateMutationWithConditionalTTL, we do NOT skip immutable tables here. For - // conditional TTL the server reads the current row only when context.hasConditionalTTL, which is + // conditional TTL the server reads the current row only when context.hasConditionalTTL, which + // is // itself derived from the _TTL mutation attribute this annotation would set - so skipping // immutable tables is self-consistent (no attribute => no conditional read). For a LITERAL TTL // the server-side current-row read in IndexRegionObserver.getCurrentRowStates is triggered by @@ -1921,9 +1923,9 @@ public static void annotateMutationWithLiteralTTL(PhoenixConnection connection, // For a view, honor the view-TTL feature flag exactly as setScanAttributesForPhoenixTTL does. boolean isView = table.getType() == PTableType.VIEW; - boolean viewTTLEnabled = !isView - || connection.getQueryServices().getConfiguration().getBoolean( - QueryServices.PHOENIX_VIEW_TTL_ENABLED, QueryServicesOptions.DEFAULT_PHOENIX_VIEW_TTL_ENABLED); + boolean viewTTLEnabled = !isView || connection.getQueryServices().getConfiguration().getBoolean( + QueryServices.PHOENIX_VIEW_TTL_ENABLED, + QueryServicesOptions.DEFAULT_PHOENIX_VIEW_TTL_ENABLED); // The view's literal TTL is threaded as _TTL only for a view with view-TTL enabled, since it is // not on the shared CF descriptor. A view with view-TTL disabled sets no _TTL on the read path @@ -1957,7 +1959,8 @@ public static void annotateMutationWithLiteralTTL(PhoenixConnection connection, mutation.setAttribute(BaseScannerRegionObserverConstants.IS_STRICT_TTL, isStrictTTL); } mutation.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_FAMILY_NAME, emptyCF); - mutation.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_QUALIFIER_NAME, emptyCQ); + mutation.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_QUALIFIER_NAME, + emptyCQ); } } diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java b/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java index baa98ce9999..fa61cb0b9a8 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java @@ -27,7 +27,6 @@ import org.apache.hadoop.hbase.util.Bytes; import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants; import org.apache.phoenix.filter.PagingFilter; -import org.apache.phoenix.index.IndexMaintainer; import org.apache.phoenix.query.QueryServices; import org.apache.phoenix.query.QueryServicesOptions; import org.apache.phoenix.schema.types.PBoolean; @@ -58,13 +57,10 @@ private ServerScanUtil() { *

    * TTL masking attributes ({@link TTLRegionScanner} reads these): *

      - *
    • the empty-column CF/CQ, supplied by the caller. When a secondary index is being maintained - * they come from the data table's {@link IndexMaintainer} (identical across all maintainers of a - * data table); when the current-row read is triggered without any index (an atomic / ON DUPLICATE - * KEY / {@code returnResult} / row-delete on a TTL table) there is no maintainer, so they are the - * bytes the client threaded on the mutation - * ({@link org.apache.phoenix.util.ScanUtil#annotateMutationWithLiteralTTL}). Both resolve to the - * same data-table empty column;
      • + *
    • the empty-column CF/CQ, supplied by the caller from the bytes the client threaded on the + * mutation ({@link org.apache.phoenix.util.ScanUtil#annotateMutationWithLiteralTTL}) — the single + * source for every path, secondary-index and no-index (atomic / ON DUPLICATE KEY / + * {@code returnResult} / row-delete) alike;
      • *
    • {@code IS_STRICT_TTL=false} when {@code isStrictTTL == false}, so a non-strict table is not * masked (absence of the attribute defaults to strict, matching the read path);
      • *
    • the view's literal TTL as the standard {@code _TTL} scan attribute when @@ -92,11 +88,11 @@ public static void setInternalScanAttributes(Configuration conf, Scan scan, byte /** * 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. + * {@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. *

      * Ordering matters: {@code PagingRegionScanner}'s constructor reads the {@link PagingFilter} and * the page size off the scan, so this must run before @@ -113,8 +109,9 @@ public static void setInternalScanAttributesForPaging(Configuration conf, Scan s 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); + 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))); diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java index 884e2fe2833..f11b79f455c 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java @@ -390,10 +390,10 @@ public static class BatchMutateContext { // TTL is on the CF descriptor) and for conditional TTL (left on the mutation for its own path). private byte[] literalTTLForInternalScan; // The empty-column CF/CQ threaded by the client (ScanUtil.annotateMutationWithLiteralTTL) for - // any table/view with an effective literal TTL. They let the internal current-row scan mask - // even when no IndexMaintainer is available to supply them — the no-index atomic / ON DUPLICATE - // KEY / returnResult / row-delete path on a TTL table. Null when no literal TTL applies. Unlike - // the _TTL attribute these are left on the mutations (they are inert on the write path). + // any table/view with an effective literal TTL — the single source that lets the internal + // current-row scan mask, for the secondary-index and no-index (atomic / ON DUPLICATE KEY / + // returnResult / row-delete) paths alike. Null when no literal TTL applies. Unlike the _TTL + // attribute these are left on the mutations (they are inert on the write path). private byte[] emptyCFForInternalScan; private byte[] emptyCQForInternalScan; @@ -1186,23 +1186,23 @@ private boolean isPartialUncoveredIndexMutation(PhoenixIndexMetaData indexMetaDa * ({@link org.apache.phoenix.util.ScanUtil#annotateMutationWithLiteralTTL}) and captured into the * batch context — the single source for every path that reaches here, index or no-index alike; * {@code literalTTLForScan} carries a view's literal TTL (null for base tables, which use the - * CF-descriptor fallback), and {@code isStrictTTL} avoids over-masking a non-strict table. Masking - * is a strict no-op when Phoenix compaction is disabled. The internal - * scan is also given the client read path's server-paging setup (a {@code SERVER_PAGE_SIZE_MS} - * attribute and a {@code PagingFilter} wrap), so it is bounded by the server page budget just like - * a client scan; {@code readDataTableRows} skips the dummy results that paging can emit. + * CF-descriptor fallback), and {@code isStrictTTL} avoids over-masking a non-strict table. + * Masking is a strict no-op when Phoenix compaction is disabled. The internal scan is also given + * the client read path's server-paging setup (a {@code SERVER_PAGE_SIZE_MS} attribute and a + * {@code PagingFilter} wrap), so it is bounded by the server page budget just like a client scan; + * {@code readDataTableRows} skips the dummy results that paging can emit. *

      * The scan's time range is set to {@code [0, batchTimestamp)} so {@link TTLRegionScanner} anchors - * its masking clock ({@code currentTime = scan.getTimeRange().getMax()}) at {@code batchTimestamp} - * — the exact timestamp at which the index side is rebuilt — rather than at the scan-open wall - * clock. On a partial "touch" upsert, {@code getBatchTimestamp} bumps {@code batchTimestamp} - * slightly past that wall clock to avoid timestamp collisions, so a cell whose TTL boundary falls - * in that sub-millisecond window would otherwise be masked here (as of the scan-open instant) yet - * be visible to the index build (as of {@code batchTimestamp}), leaving the data row and index row - * inconsistent. Anchoring both at {@code batchTimestamp} makes the read trim expired cells as of - * exactly the timestamp the index is built at, so the two never disagree. The half-open range - * drops nothing current: every pre-existing cell of a locked row was written by an earlier batch - * at {@code ts < batchTimestamp} under the Phoenix write path. + * its masking clock ({@code currentTime = scan.getTimeRange().getMax()}) at + * {@code batchTimestamp} — the exact timestamp at which the index side is rebuilt — rather than + * at the scan-open wall clock. On a partial "touch" upsert, {@code getBatchTimestamp} bumps + * {@code batchTimestamp} slightly past that wall clock to avoid timestamp collisions, so a cell + * whose TTL boundary falls in that sub-millisecond window would otherwise be masked here (as of + * the scan-open instant) yet be visible to the index build (as of {@code batchTimestamp}), + * leaving the data row and index row inconsistent. Anchoring both at {@code batchTimestamp} makes + * the read trim expired cells as of exactly the timestamp the index is built at, so the two never + * disagree. The half-open range drops nothing current: every pre-existing cell of a locked row + * was written by an earlier batch at {@code ts < batchTimestamp} under the Phoenix write path. */ private void getCurrentRowStates(ObserverContext c, BatchMutateContext context, byte[] literalTTLForScan, boolean isStrictTTL, long batchTimestamp) @@ -1293,8 +1293,8 @@ private void getCurrentRowStates(ObserverContext c // half-open [0, batchTimestamp) range makes getTimeRange().getMax() == batchTimestamp, so // the read trims expired cells as of exactly the timestamp the index is built at. scan.setTimeRange(0, batchTimestamp); - ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, - emptyCF, emptyCQ, literalTTLForScan, isStrictTTL); + ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, emptyCF, + emptyCQ, literalTTLForScan, isStrictTTL); readDataTableRows(c, context, scan); } } @@ -1313,7 +1313,8 @@ private void readDataTableRows(ObserverContext c, if (cells.isEmpty()) { continue; } - // With server paging wired in (ServerScanUtil.setInternalScanAttributes), PagingRegionScanner + // 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)) { @@ -1743,17 +1744,17 @@ private static void identifyIndexMaintainerTypes(PhoenixIndexMetaData indexMetaD /** * Captures the literal-TTL internal-scan attributes the client threads - * ({@link org.apache.phoenix.util.ScanUtil#annotateMutationWithLiteralTTL}) off the representative - * mutation so the internal current-row scan can mask exactly like a client read. + * ({@link org.apache.phoenix.util.ScanUtil#annotateMutationWithLiteralTTL}) off the + * representative mutation so the internal current-row scan can mask exactly like a client read. *

      * Two things are captured: *

        *
      • The empty-column CF/CQ — threaded whenever an effective (non-NONE) literal TTL - * applies, for both base tables and views. They let {@code setInternalScanAttributes} mask even - * when no {@link IndexMaintainer} is available to supply them: the no-index atomic / ON DUPLICATE - * KEY / {@code returnResult} / row-delete path on a TTL table. They are inert on the write path - * (nothing reads a mutation's empty-column attribute), so they are left on the mutations, not - * removed.
        • + * applies, for both base tables and views. They are the single source that lets + * {@code setInternalScanAttributes} mask, for the secondary-index and no-index (atomic / ON + * DUPLICATE KEY / {@code returnResult} / row-delete) paths alike. They are inert on the write + * path (nothing reads a mutation's empty-column attribute), so they are left on the mutations, + * not removed. *
      • The view's literal {@code _TTL} — the client threads it via the same {@code _TTL} * mutation attribute the server otherwise treats as conditional TTL * ({@code PhoenixIndexBuilder.hasConditionalTTL}). We make that attribute polymorphic: if the @@ -1768,9 +1769,8 @@ private static void identifyIndexMaintainerTypes(PhoenixIndexMetaData indexMetaD * has exactly one TTL kind and a batch never mixes views, so the decision is uniform across the * batch. HBase has no removeAttribute, so removal is {@code setAttribute(TTL, null)}. */ - private void extractLiteralTTLForInternalScan( - MiniBatchOperationInProgress miniBatchOp, BatchMutateContext context) - throws IOException { + private void extractLiteralTTLForInternalScan(MiniBatchOperationInProgress miniBatchOp, + BatchMutateContext context) throws IOException { if (miniBatchOp.size() == 0) { return; } @@ -1983,7 +1983,8 @@ public void preBatchMutateWithExceptions(ObserverContext - * The schema is {@code IDMAPPER.ID_MAPPING_V2}: PK (organization_id, map_name, map_id), - * MULTI_TENANT, a NON-FOREVER TTL, and a global covered index on (map_name, left_id, right_id) - * INCLUDE (map_id_space, right_id_space, left_id_space). The id-mapper client refreshes a row's TTL - * by issuing a partial self-referential {@code UPSERT SELECT} that names only - * (organization_id, map_name, map_id, map_id_space) - it does NOT name left_id / left_id_space. - *

        - * Mechanism (all confirmed against the code): - *

          - *
        1. The initial full row lands with every cell at one timestamp {@code T0}.
          2. - *
        2. The partial touch is READ while the row is still alive (at {@code T0 + TTL - 1}, before the - * boundary), but its mutation is committed a couple of ticks later, at - * {@code T_commit = T0 + TTL + 1}. The touch names neither left_id nor left_id_space, so as - * written it carries only map_id_space + the empty column, both landing at {@code T_commit}.
          3. - *
        3. PRE-FIX (the bug): index maintenance in IndexRegionObserver reads the current data row via an - * internal scan that is never TTL-masked, so it sees left_id / left_id_space (still physically - * present at {@code T0}) as live, carries them forward, and rebuilds the whole index row at the - * single fresh {@code T_commit}. The DATA row, meanwhile, still holds those cells only at - * {@code T0}: {@code maxTs - minTs = T_commit - T0 > TTL}, so {@code TTLRegionScanner}'s gap - * analysis masks them and a data-side SELECT reads NULL. Index returns left_id / left_id_space, - * data reads NULL - the production divergence (data NULL, index non-null).
          4. - *
        4. POST-FIX: the internal current-row read is TTL-masked as of exactly {@code batchTimestamp} - - * {@code scan.setTimeRange(0, batchTimestamp)} anchors {@code TTLRegionScanner}'s masking clock - * at the very timestamp the index is built at (see {@code IndexRegionObserver.getCurrentRowStates}). - * So the index is rebuilt from the same logically-expired-or-alive view the data side sees: - * left_id / left_id_space are trimmed-or-retained as a UNIT on both sides. Because this touch - * commits one tick PAST the boundary, the anchored read drops them on the index side just as the - * data-side read masks them, so data and index both read NULL - in agreement. (Had the touch - * landed BEFORE the boundary, or in a dense sub-TTL stream, both sides would instead retain the - * value - still in agreement.)
          5. - *
        - * This is a REGRESSION test asserting agreement, robust to whichever route the boundary takes: each - * test is GREEN when the data table and the index agree on left_id / left_id_space for the same - * logical row (the fixed behavior), and RED when they diverge (the pre-fix bug: data NULL, index - * non-null). It deliberately does not pin a specific retain-vs-drop value, so it is deterministic - * regardless of which side of the boundary the single touch lands on. - *

        - * Agreement is asserted at the QUERY layer (both the data-path and index-path SELECTs read through - * {@code TTLRegionScanner}), which is the user-visible guarantee. It is NOT asserted with a raw HBase - * scan: under the production-like non-zero max-lookback this test runs with (see {@code doSetup}), the - * {@code T0} row version lingers physically inside the time-travel window on the data side even after - * major compaction, so a raw (unmasked) scan would report physical state that no query ever observes. - */ -@Category(NeedsOwnMiniClusterTest.class) -public class IndexDataTableTTLBoundaryConsistencyIT extends BaseTest { - - private static final Logger LOGGER = - LoggerFactory.getLogger(IndexDataTableTTLBoundaryConsistencyIT.class); - - private static final String SCHEMA_NAME = "IDMAPPER"; - - // Unique per test method (assigned in setUpSchema). Hardcoded names would let one method's - // future-dated cells (the injected clock commits ~TTL ahead of real wall time) survive the next - // method's DROP on the shared per-class mini-cluster and pollute its physical table. - private String tableName; - private String indexName; - - // Short, NON-FOREVER TTL so the injected clock can cross it deterministically. A FOREVER TTL - // would short-circuit TTLRegionScanner/CompactionScanner and the bug could not reproduce. - private static final int TTL_SECONDS = 30; - private static final long TTL_MS = TTL_SECONDS * 1000L; - - private static final String TENANT_ID = "00Dxx0000001gER"; // VARCHAR(15) - private static final String MAP_NAME = "MY_MAP"; - private static final String MAP_ID = "uuid:abc"; - - private static final String MAP_ID_SPACE = "SPACE_1"; // the touched (named) covered column - private static final String LEFT_ID = "LEFT_A"; // index key column, distinctive value - private static final String LEFT_ID_SPACE = "LEFTSPACE_1"; // covered column, distinctive value - - private ManualEnvironmentEdge injectEdge; - - @BeforeClass - public static synchronized void doSetup() throws Exception { - // Run under the production-like config the anchored-masking fix recommends: a non-zero - // max-lookback set just BELOW the TTL. This is deliberately NOT zero. Two reasons: - // 1. It matches how indexed TTL tables are actually configured, and it closes the narrow - // race where a concurrent major compaction could physically collect a pre-existing cell - // between the current-row read and the mutation being persisted. - // 2. It keeps RowContext.setTTL's effective-TTL floor Math.max(ttlInSecs*1000, - // maxLookbackInMillis + 1) equal to TTL_MS: with max-lookback below TTL the floor stays - // at TTL_MS, so the injected (TTL + 1) gap still exceeds the effective TTL and the - // boundary drop path is exercised. Had max-lookback reached or exceeded TTL the floor - // would rise, the gap would no longer exceed it, and the scenario would stop reproducing. - // Note: under a non-zero max-lookback the T0 row version lingers PHYSICALLY inside the - // lookback (time-travel) window on the data side even after major compaction. That is - // expected retention, not divergence: no live query sees it, because every read - data or - // index - is masked by TTLRegionScanner, which trims the (T0, T_commit] gap identically on - // both sides. Consistency is therefore asserted at the query layer (masked reads), not by a - // raw physical scan (which bypasses TTLRegionScanner and would report the unmasked physical - // state that users never observe). - Map props = new HashMap<>(); - props.put(BaseScannerRegionObserverConstants.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, - Integer.toString(TTL_SECONDS - 1)); - // Speed up compaction scheduling in the mini-cluster. - props.put("hbase.procedure.remote.dispatcher.delay.msec", "0"); - setUpTestDriver(new ReadOnlyProps(props.entrySet().iterator())); - } - - @Before - public void setUpSchema() throws Exception { - // DDL runs on the real clock; the manual edge is injected per-scenario, after the schema - // exists, so it only governs data mutation / scan timestamps. - EnvironmentEdgeManager.reset(); - // Unique physical tables per method: the injected clock commits cells ~TTL into the future, - // so a shared hardcoded table would let one method's future-dated cells outlive the next - // method's DROP (run at real wall time) and bridge its TTL gap, corrupting the scenario. - tableName = SCHEMA_NAME + "." + generateUniqueName(); - indexName = generateUniqueName(); - try (Connection conn = DriverManager.getConnection(getUrl()); - Statement stmt = conn.createStatement()) { - conn.setAutoCommit(true); - stmt.execute("CREATE TABLE IF NOT EXISTS " + tableName + " (\n" - + " organization_id VARCHAR(15) NOT NULL,\n" - + " map_name VARCHAR(80) NOT NULL,\n" - + " map_id_space VARCHAR(255),\n" - + " map_id VARCHAR(2047) NOT NULL,\n" - + " left_id_space VARCHAR(255),\n" - + " left_id VARCHAR(2047),\n" - + " right_id_space VARCHAR(255),\n" - + " right_id VARCHAR(2047),\n" - + " CONSTRAINT PK PRIMARY KEY (organization_id, map_name, map_id)\n" - + ") TTL=" + TTL_SECONDS + ", MULTI_TENANT=true, REPLICATION_SCOPE=1"); - stmt.execute("CREATE INDEX " + indexName + "\n" - + " ON " + tableName + " (map_name, left_id, right_id)\n" - + " INCLUDE (map_id_space, right_id_space, left_id_space)"); - } - injectEdge = new ManualEnvironmentEdge(); - } - - @After - public void tearDown() { - EnvironmentEdgeManager.reset(); - } - - /** Full initial row: PK + map_id_space + non-null left_id / left_id_space; right_* NULL. */ - private void insertInitialRow(Connection conn) throws Exception { - try (PreparedStatement ps = conn.prepareStatement("UPSERT INTO " + tableName - + " (organization_id, map_name, map_id, map_id_space, left_id, left_id_space, " - + "right_id, right_id_space) VALUES (?, ?, ?, ?, ?, ?, NULL, NULL)")) { - ps.setString(1, TENANT_ID); - ps.setString(2, MAP_NAME); - ps.setString(3, MAP_ID); - ps.setString(4, MAP_ID_SPACE); - ps.setString(5, LEFT_ID); - ps.setString(6, LEFT_ID_SPACE); - ps.executeUpdate(); - } - } - - /** - * The exact id-mapper "mappings in use" touch: a self-referential UPSERT SELECT naming only - * PK + map_id_space. Only {@code executeUpdate()} - the inner SELECT runs and the mutation is - * buffered here; the caller controls when (and at which timestamp) it commits. - */ - private void touchWithUpsertSelect(Connection conn) throws Exception { - try (PreparedStatement ps = conn.prepareStatement("UPSERT INTO " + tableName - + " (organization_id, map_name, map_id, map_id_space) " - + "SELECT organization_id, map_name, map_id, map_id_space FROM " + tableName - + " WHERE organization_id = ? AND map_name = ? AND map_id = ?")) { - ps.setString(1, TENANT_ID); - ps.setString(2, MAP_NAME); - ps.setString(3, MAP_ID); - int affected = ps.executeUpdate(); - LOGGER.info("UPSERT SELECT touch affected {} row(s)", affected); - assertEquals("UPSERT SELECT must read the still-alive row and buffer exactly one touch", - 1, affected); - } - } - - /** Plain partial UPSERT touch naming only PK + map_id_space. Buffered only (no commit here). */ - private void touchWithPlainUpsert(Connection conn) throws Exception { - try (PreparedStatement ps = conn.prepareStatement("UPSERT INTO " + tableName - + " (organization_id, map_name, map_id, map_id_space) VALUES (?, ?, ?, ?)")) { - ps.setString(1, TENANT_ID); - ps.setString(2, MAP_NAME); - ps.setString(3, MAP_ID); - ps.setString(4, MAP_ID_SPACE); - ps.executeUpdate(); - } - } - - /** Resolves the physical HBase name of a Phoenix table/index. */ - private byte[] physicalName(Connection conn, String phoenixName, boolean isIndex) - throws Exception { - PhoenixConnection pconn = conn.unwrap(PhoenixConnection.class); - PTable dataTable = pconn.getTable(new PTableKey(null, tableName)); - if (!isIndex) { - return dataTable.getPhysicalName().getBytes(); - } - for (PTable index : dataTable.getIndexes()) { - if (index.getTableName().getString().equals(phoenixName)) { - return index.getPhysicalName().getBytes(); - } - } - fail("Could not resolve physical name for index " + phoenixName); - return null; // unreachable - } - - private void flush(byte[] physicalTable) throws IOException { - getUtility().getAdmin().flush(TableName.valueOf(physicalTable)); - } - - private void majorCompact(byte[] physicalTable) throws Exception { - TestUtil.majorCompact(getUtility(), TableName.valueOf(physicalTable)); - } - - /** - * @param useUpsertSelect when true the touch is the production self-referential UPSERT SELECT; - * when false it is a plain partial UPSERT (a control that drives the - * identical data/index (dis)agreement, isolating the cause as the - * partiality of the write, not the SELECT read side). - */ - private void runScenario(boolean useUpsertSelect) throws Exception { - try (Connection conn = DriverManager.getConnection(getUrl())) { - conn.setAutoCommit(false); - - // Freeze the clock at T0 and write the full initial row; every cell lands at T0. - long t0 = EnvironmentEdgeManager.currentTimeMillis(); - injectEdge.setValue(t0); - EnvironmentEdgeManager.injectEdge(injectEdge); - - insertInitialRow(conn); - conn.commit(); - - byte[] dataPhysical = physicalName(conn, tableName, false); - byte[] indexPhysical = physicalName(conn, indexName, true); - - // Push the T0 cells onto an HFile so the later major compaction merges two files. - flush(dataPhysical); - flush(indexPhysical); - - injectEdge.incrementValue(TTL_MS - 1); - long tRead = injectEdge.currentTime(); - if (useUpsertSelect) { - touchWithUpsertSelect(conn); - } else { - touchWithPlainUpsert(conn); - } - // NOTE: no flush / compaction happens between the touch and its commit, so (for the - // UPSERT SELECT variant) the inner SELECT read cannot be disturbed mid-statement. - - // --- Advance two ticks (past the boundary) and commit: as written, the touch cells - // (map_id_space + empty) land at T_commit = T0 + TTL + 1, while left_id / left_id_space - // stay at T0. The gap on the data row is now (TTL + 1) > TTL. PRE-FIX, gap analysis trims - // that on the DATA side while the index (rebuilt off an unmasked read at T_commit) keeps - // it -> divergence. POST-FIX, the internal current-row read is masked as of exactly - // batchTimestamp (scan.setTimeRange(0, batchTimestamp) anchors TTLRegionScanner's clock at - // the timestamp the index is built at), so it trims left_id on the INDEX side identically - // to how a data-side read trims it -> both drop it, in agreement. --- - injectEdge.incrementValue(2); - long tCommit = injectEdge.currentTime(); - conn.commit(); - - LOGGER.info("t0={} tRead={} tCommit={} gap={}ms ttl={}ms", t0, tRead, tCommit, - tCommit - t0, TTL_MS); - - // Major-compact both tables to settle each side's physical state before the reads. Note - // that under the non-zero max-lookback (see doSetup) the T0 cells are NOT physically - // purged from the data side - they linger inside the time-travel window - but they remain - // masked from every query by TTLRegionScanner's gap analysis. Consistency is therefore - // observed at the query layer: PRE-FIX the masked data SELECT reads NULL while the index - // (rebuilt off an unmasked internal read at T_commit) still returns the value -> RED; - // POST-FIX the anchored masked internal read drops it on the index side too -> both NULL. - flush(dataPhysical); - flush(indexPhysical); - majorCompact(dataPhysical); - majorCompact(indexPhysical); - - // ============ ASSERTIONS: data and index must AGREE (regression guard) ============ - // The bug is a DISAGREEMENT between the data table and its index on left_id / - // left_id_space for the same logical row. This test pins agreement, NOT a specific - // retain-vs-drop value, so it is deterministic regardless of which side of the TTL - // boundary the single touch lands on: - // - PRE-FIX -> data reads NULL (T0 cells gap-trimmed) while the index still carries - // the value rebuilt at T_commit -> NOT equal -> RED. - // - POST-FIX -> both drop it (commit is past the boundary) or both keep it (touch - // before the boundary / dense stream) -> equal -> GREEN. - - // (1) Read left_id / left_id_space from the DATA table (data path). - String dataLeftId; - String dataLeftIdSpace; - String dataMapIdSpace; - try (PreparedStatement ps = conn.prepareStatement( - "SELECT left_id, left_id_space, map_id_space FROM " + tableName - + " WHERE organization_id = ? AND map_name = ? AND map_id = ?")) { - ps.setString(1, TENANT_ID); - ps.setString(2, MAP_NAME); - ps.setString(3, MAP_ID); - try (ResultSet rs = ps.executeQuery()) { - assertTrue("DATA row must still exist (map_id_space touch keeps the row alive)", - rs.next()); - dataLeftId = rs.getString("left_id"); - dataLeftIdSpace = rs.getString("left_id_space"); - dataMapIdSpace = rs.getString("map_id_space"); - } - } - - // (2) Read left_id / left_id_space for the same row through the INDEX (forced index). - String indexLeftId; - String indexLeftIdSpace; - try (PreparedStatement ps = conn.prepareStatement( - "SELECT /*+ INDEX(" + tableName + " " + indexName + ") */ left_id, left_id_space " - + "FROM " + tableName - + " WHERE organization_id = ? AND map_name = ?")) { - ps.setString(1, TENANT_ID); - ps.setString(2, MAP_NAME); - try (ResultSet rs = ps.executeQuery()) { - assertTrue("INDEX query must return the row", rs.next()); - indexLeftId = rs.getString("left_id"); - indexLeftIdSpace = rs.getString("left_id_space"); - } - } - - System.out.println("\n===== TTL-boundary consistency (" - + (useUpsertSelect ? "UPSERT SELECT" : "plain UPSERT") + ") ====="); - System.out.println("DATA left_id=" + dataLeftId + " left_id_space=" + dataLeftIdSpace - + " map_id_space=" + dataMapIdSpace); - System.out.println("INDEX left_id=" + indexLeftId + " left_id_space=" + indexLeftIdSpace); - - // (3) False-negative guard: the touched column must survive on the data side, proving the - // row itself did NOT wholesale-expire (which would make a trivial NULL==NULL pass). - assertEquals("DATA map_id_space must survive at T_commit (row is alive, not expired)", - MAP_ID_SPACE, dataMapIdSpace); - - // (4) Query-layer agreement: data and index must read the SAME value for each column, - // whether that value is the retained one or NULL. This is the core regression guard, - // and it is asserted at the query layer on purpose. Both the data-path SELECT and the - // index-path SELECT read through TTLRegionScanner, so both are masked by the SAME gap - // analysis - which is exactly the user-visible consistency guarantee the fix makes. - // A raw HBase scan is deliberately NOT used here: under the production-like non-zero - // max-lookback (see doSetup), the T0 row version lingers PHYSICALLY on the data side - // inside the time-travel window even after major compaction, so a raw (unmasked) scan - // would report a physical state that diverges from what every query sees - a false - // signal, not a divergence. The query-layer check below is the meaningful assertion. - assertEquals("left_id must agree between DATA and INDEX (data=" + dataLeftId + " index=" - + indexLeftId + ")", dataLeftId, indexLeftId); - assertEquals("left_id_space must agree between DATA and INDEX (data=" + dataLeftIdSpace - + " index=" + indexLeftIdSpace + ")", dataLeftIdSpace, indexLeftIdSpace); - } - } - - /** - * The production self-referential UPSERT SELECT touch landing across the TTL boundary. GREEN when - * the data table and the index agree on left_id / left_id_space (fixed); RED on the pre-fix - * divergence (data NULL, index non-null). - */ - @Test - public void testUpsertSelectTouchNearTtlBoundaryKeepsDataAndIndexConsistent() throws Exception { - runScenario(true); - } - - /** - * Control: a plain partial UPSERT exercises the identical path and must keep the data table and - * the index consistent, isolating the cause as the partiality of the write, not the SELECT read. - */ - @Test - public void testPlainPartialUpsertTouchNearTtlBoundaryKeepsDataAndIndexConsistent() - throws Exception { - runScenario(false); - } -} diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/MetaDataEndPointIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/MetaDataEndPointIT.java index 66fdc1540bb..dfd21bf91c7 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/MetaDataEndPointIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/MetaDataEndPointIT.java @@ -44,9 +44,9 @@ * {@link MetricsMetadataSourceFactory#getMetadataMetricsSource()} singleton, whose counters * (CREATE_TABLE_COUNT, METADATA_CACHE_ESTIMATED_USED_SIZE, METADATA_CACHE_ADD_COUNT, ...) are * incremented by every server-side metadata operation in the same JVM. Running this test class in - * the {@link ParallelStatsDisabledTest} group lets parallel tests in other classes in the same - * fork mutate those counters between this test's "capture baseline" and "verify after CREATE - * TABLE" calls, producing intermittent strict-equality assertion failures such as: + * the {@link ParallelStatsDisabledTest} group lets parallel tests in other classes in the same fork + * mutate those counters between this test's "capture baseline" and "verify after CREATE TABLE" + * calls, producing intermittent strict-equality assertion failures such as: * * 

          *   testMetadataMetricsOfCreateTable
@@ -54,8 +54,8 @@
  *
        * * Categorize as {@link NeedsOwnMiniClusterTest} so failsafe runs this class in its own forked JVM - * (reuseForks=false), guaranteeing exclusive ownership of the metric singleton for the duration - * of the suite. Tests within this class still run sequentially in that fork, which is sufficient + * (reuseForks=false), guaranteeing exclusive ownership of the metric singleton for the duration of + * the suite. Tests within this class still run sequentially in that fork, which is sufficient * because each individual test captures and verifies its own counter deltas in a single thread. */ @Category(NeedsOwnMiniClusterTest.class) diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/MetadataGetTableReadLockIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/MetadataGetTableReadLockIT.java index f2344fc8c93..58c4b8fbc86 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/MetadataGetTableReadLockIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/MetadataGetTableReadLockIT.java @@ -97,7 +97,8 @@ public void testBlockedReadDoesNotBlockAnotherRead() throws Exception { } } - private static void waitForCoprocessorOnSystemCatalog(Class coprocessorClass) throws Exception { + private static void waitForCoprocessorOnSystemCatalog(Class coprocessorClass) + throws Exception { final TableName sysCatalog = TableName.valueOf("SYSTEM.CATALOG"); utility.waitFor(10000, 100, () -> { List regions = utility.getHBaseCluster().getRegions(sysCatalog); @@ -105,8 +106,7 @@ private static void waitForCoprocessorOnSystemCatalog(Class coprocessorClass) return false; } for (HRegion region : regions) { - if (region.getCoprocessorHost() - .findCoprocessor(coprocessorClass.getName()) == null) { + if (region.getCoprocessorHost().findCoprocessor(coprocessorClass.getName()) == null) { return false; } } diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableSyncIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableSyncIT.java new file mode 100644 index 00000000000..30298b24834 --- /dev/null +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableSyncIT.java @@ -0,0 +1,796 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.end2end.index; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.Arrays; +import java.util.Collection; +import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Admin; +import org.apache.hadoop.hbase.client.Mutation; +import org.apache.hadoop.hbase.coprocessor.ObserverContext; +import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment; +import org.apache.hadoop.hbase.coprocessor.SimpleRegionObserver; +import org.apache.hadoop.hbase.regionserver.MiniBatchOperationInProgress; +import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants; +import org.apache.phoenix.end2end.NeedsOwnMiniClusterTest; +import org.apache.phoenix.jdbc.PhoenixConnection; +import org.apache.phoenix.query.BaseTest; +import org.apache.phoenix.query.QueryServices; +import org.apache.phoenix.schema.PTable; +import org.apache.phoenix.util.EnvironmentEdgeManager; +import org.apache.phoenix.util.ManualEnvironmentEdge; +import org.apache.phoenix.util.ReadOnlyProps; +import org.apache.phoenix.util.TestUtil; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import org.apache.phoenix.thirdparty.com.google.common.collect.Maps; + +/** + * Verifies that internal server-side index-maintenance current-row reads honor Phoenix TTL exactly + * like a client read, and that the read is anchored at the mutation timestamp so the data table and + * a global index stay aligned under compaction. + *

        + * The scenario is the divergence being fixed: a covered column (covcol) is written once, then never + * re-written by later partial "touch" upserts. On the index side covcol is rebuilt at every touch's + * mutationTimestamp from the masked current-row read; on the data side covcol keeps its original + * timestamp. The fix anchors that internal read's masking clock at mutationTimestamp (the same + * instant the index is built at) via scan.setTimeRange(0, mutationTimestamp), so both sides always + * agree on whether covcol is still alive: whatever the read masks the index rebuild omits, and + * whatever the read keeps a later major compaction keeps on the data side too. The row and its + * index then expire as a unit rather than covcol being dropped on only one side. + * {@link #testNoIndexAtomicUpsertMasksExpiredRow} covers the no-index masking path: an atomic ON + * DUPLICATE KEY UPDATE on a TTL table with no secondary index still triggers an internal + * current-row read. That scan is masked using the empty-column CF/CQ the client threads on the + * mutation — the same single source used for the secondary-index case — so an expired row is + * treated as absent rather than resurrected. + */ +@Category(NeedsOwnMiniClusterTest.class) +@RunWith(Parameterized.class) +public class IndexDataTableSyncIT extends BaseTest { + static final int MAX_LOOKBACK_AGE = 10; + static final int TTL = 60; + static final String COVCOL_VALUE = "cov-original"; + static final String IDXCOL_VALUE = "idx-original"; + + private final boolean columnEncoded; + private ManualEnvironmentEdge injectEdge; + + public IndexDataTableSyncIT(boolean columnEncoded) { + this.columnEncoded = columnEncoded; + } + + @Parameterized.Parameters(name = "columnEncoded={0}") + public static synchronized Collection data() { + return Arrays.asList(new Object[][] { { false }, { true } }); + } + + @BeforeClass + public static synchronized void doSetup() throws Exception { + setUpTestDriver(new ReadOnlyProps(baseServerProps().entrySet().iterator())); + } + + /** + * Common server props for the TTL-sync IT: max-lookback and immediate global-index row aging so a + * major compaction deterministically exercises the covered-column timestamp-skew divergence. + */ + static Map baseServerProps() { + Map props = Maps.newHashMapWithExpectedSize(4); + props.put(QueryServices.GLOBAL_INDEX_ROW_AGE_THRESHOLD_TO_DELETE_MS_ATTRIB, Long.toString(0)); + props.put(BaseScannerRegionObserverConstants.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, + Integer.toString(MAX_LOOKBACK_AGE)); + props.put("hbase.procedure.remote.dispatcher.delay.msec", "0"); + // The view case threads the view's literal TTL as the per-mutation _TTL attribute. + props.put(QueryServices.PHOENIX_VIEW_TTL_ENABLED, Boolean.toString(true)); + return props; + } + + @Before + public void beforeTest() { + EnvironmentEdgeManager.reset(); + injectEdge = new ManualEnvironmentEdge(); + injectEdge.setValue(EnvironmentEdgeManager.currentTimeMillis()); + } + + @After + public synchronized void afterTest() throws Exception { + boolean refCountLeaked = isAnyStoreRefCountLeaked(); + EnvironmentEdgeManager.reset(); + Assert.assertFalse("refCount leaked", refCountLeaked); + } + + /** + * Builds the comma-separated table-property clause. When ttlSeconds >= 0 a TTL is emitted as the + * first property; COLUMN_ENCODED_BYTES always follows, and IS_STRICT_TTL=false is appended for a + * non-strict table. + */ + private String withClause(int ttlSeconds, boolean strict) { + StringBuilder sb = new StringBuilder(" "); + if (ttlSeconds >= 0) { + sb.append("TTL=").append(ttlSeconds).append(", "); + } + sb.append("COLUMN_ENCODED_BYTES=").append(columnEncoded ? 2 : 0); + if (!strict) { + sb.append(", IS_STRICT_TTL=false"); + } + return sb.toString(); + } + + /** + * Base table with a literal TTL and a covered global index. After a partial touch that never + * writes covcol, the data-side covcol keeps its original timestamp; the masked internal read + * anchored at mutationTimestamp keeps the data and index agreeing on covcol while the row is + * alive, and a full expiry after that shows the data row and the index expiring as a unit. + */ + @Test + public void testBaseTableCoveredColumnResync() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.createStatement() + .execute("CREATE TABLE " + tableName + " (id VARCHAR NOT NULL PRIMARY KEY, " + + "idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR)" + withClause(TTL, true)); + conn.createStatement() + .execute("CREATE INDEX " + indexName + " ON " + tableName + " (idxcol) INCLUDE (covcol)"); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + // Full initial upsert: covcol is written exactly once here and never again. + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); + conn.commit(); + + // Partial touch that does not write covcol, well within the TTL so the row is alive. + injectEdge.incrementValue(1000); + conn.createStatement() + .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); + conn.commit(); + assertTouchColConsistent(conn, tableName, indexName, "r1", "x1"); + // Data and index agree on covcol + assertCovColConsistent(conn, tableName, indexName, "r1", "k1", COVCOL_VALUE); + + // Advancing past TTL expires the row in data table for internal current row read. + injectEdge.incrementValue((TTL + 1) * 1000L); + conn.createStatement() + .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x2')"); + conn.commit(); + assertTouchColConsistent(conn, tableName, indexName, "r1", "x2"); + // Data and index agree on covcol + assertCovColAbsent(conn, tableName, indexName, "r1"); + } + } + + /** + * Uncovered global index on a base table with a literal TTL. An uncovered index stores its + * indexed column (idxcol) in the index row key, rebuilt at every touch's mutationTimestamp, while + * a partial touch that omits idxcol would otherwise leave the data-side idxcol cell at its + * original timestamp. + */ + @Test + public void testUncoveredIndexIndexedColumnResync() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.createStatement() + .execute("CREATE TABLE " + tableName + " (id VARCHAR NOT NULL PRIMARY KEY, " + + "idxcol VARCHAR, touchcol VARCHAR)" + withClause(TTL, true)); + conn.createStatement() + .execute("CREATE UNCOVERED INDEX " + indexName + " ON " + tableName + " (idxcol)"); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + // Full initial upsert: idxcol is written exactly once here and never again. + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, idxcol, touchcol) VALUES ('r1', '" + IDXCOL_VALUE + "', 'x0')"); + conn.commit(); + + // Partial touch that does not write idxcol, well within the TTL so the row is alive and the + // internal scan anchored at batchTimestamp still returns idxcol for the index rebuild. + injectEdge.incrementValue(1000); + conn.createStatement() + .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); + conn.commit(); + + // An uncovered index carries no data columns, so touchcol is only readable data-side; the + // index side agrees on idxcol below. The row is alive well within the TTL. + assertDataTouchColAlive(conn, tableName, "r1", "x1"); + assertUncoveredIdxColConsistent(conn, tableName, indexName, "r1", IDXCOL_VALUE); + + // Advancing past TTL expires the row in data table for internal current row read. + injectEdge.incrementValue((TTL + 1) * 1000L); + conn.createStatement() + .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x2')"); + conn.commit(); + // Data side: row still alive by PK (touchcol keeps it); the masked idxcol@T0 is trimmed, so + // the uncovered index no longer carries a usable entry keyed by idxcol (asserted below). + assertDataTouchColAlive(conn, tableName, "r1", "x2"); + assertUncoveredIdxColAbsent(conn, tableName, indexName, IDXCOL_VALUE); + } + } + + /** + * View with a view-level literal TTL (not on the shared CF descriptor) and a covered index on the + * view. Exercises the literal-_TTL per-mutation threading: the internal scan masks with the + * view's TTL, anchored at mutationTimestamp, and covcol keeps its original data-side timestamp + * just like the base-table case while the data and index reads stay consistent. A full expiry + * after that shows the view's data row and its index expiring as a unit. + */ + @Test + public void testViewCoveredColumnResync() throws Exception { + String baseTableName = generateUniqueName(); + String viewName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + // Base table carries NO TTL; the TTL lives only on the view. + conn.createStatement() + .execute("CREATE TABLE " + baseTableName + " (id VARCHAR NOT NULL PRIMARY KEY, " + + "idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR)" + withClause(-1, true)); + conn.createStatement() + .execute("CREATE VIEW " + viewName + " AS SELECT * FROM " + baseTableName + " TTL=" + TTL); + conn.createStatement() + .execute("CREATE INDEX " + indexName + " ON " + viewName + " (idxcol) INCLUDE (covcol)"); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + conn.createStatement().execute("UPSERT INTO " + viewName + + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); + conn.commit(); + + injectEdge.incrementValue(1000); + conn.createStatement() + .execute("UPSERT INTO " + viewName + " (id, touchcol) VALUES ('r1', 'x1')"); + conn.commit(); + assertTouchColConsistent(conn, viewName, indexName, "r1", "x1"); + // View and index agree on covcol + assertCovColConsistent(conn, viewName, indexName, "r1", "k1", COVCOL_VALUE); + + // Advancing past the view TTL expires the row in data table for internal current row read. + injectEdge.incrementValue((TTL + 1) * 1000L); + conn.createStatement() + .execute("UPSERT INTO " + viewName + " (id, touchcol) VALUES ('r1', 'x2')"); + conn.commit(); + assertTouchColConsistent(conn, viewName, indexName, "r1", "x2"); + assertCovColAbsent(conn, viewName, indexName, "r1"); + } + } + + /** + * A table with IS_STRICT_TTL=false must not be masked away: the internal scan anchored at + * mutationTimestamp must not mask covcol away. + */ + @Test + public void testNonStrictTableNotOverMasked() throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.createStatement() + .execute("CREATE TABLE " + tableName + " (id VARCHAR NOT NULL PRIMARY KEY, " + + "idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR)" + withClause(TTL, false)); + conn.createStatement() + .execute("CREATE INDEX " + indexName + " ON " + tableName + " (idxcol) INCLUDE (covcol)"); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); + conn.commit(); + + // Advance beyond TTL, then touch. A strict table would mask covcol at the internal scan; + // a non-strict table must not. + injectEdge.incrementValue((TTL + 5) * 1000L); + conn.createStatement() + .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); + conn.commit(); + assertTouchColConsistent(conn, tableName, indexName, "r1", "x1"); + // The decisive check: a non-strict internal read is not masked away, so the index rebuilds + // with covcol and both the data read and the index read still return it. + assertCovColConsistent(conn, tableName, indexName, "r1", "k1", COVCOL_VALUE); + + injectEdge.incrementValue(MAX_LOOKBACK_AGE * 1000L); + flushAndMajorCompact(conn, tableName); + flushAndMajorCompact(conn, indexName); + + // Data side: the row is still alive (touchcol keeps it) but the stale covcol was physically + // collected by compaction, so a forced data-table read returns covcol = NULL. + try (ResultSet rs = conn.createStatement() + .executeQuery("SELECT /*+ NO_INDEX */ covcol FROM " + tableName + " WHERE id = 'r1'")) { + assertTrue("data-table row should still be alive (touchcol keeps it)", rs.next()); + assertNull("stale data-side covcol should be collected by compaction", rs.getString(1)); + assertFalse("exactly one data-table row", rs.next()); + } + + // Index side: the covered index copy survives compaction, so the index read still returns it. + try (ResultSet rs = conn.createStatement() + .executeQuery("SELECT covcol FROM " + tableName + " WHERE idxcol = 'k1'")) { + assertTrue("index-visible row should still exist", rs.next()); + assertEquals("index covcol should survive compaction", COVCOL_VALUE, rs.getString(1)); + assertFalse("exactly one index-visible row", rs.next()); + } + } + } + + /** + * The no-index masking path: an atomic ON DUPLICATE KEY UPDATE on a base table with a literal TTL + * and NO secondary index. + */ + @Test + public void testNoIndexAtomicUpsertMasksExpiredRow() throws Exception { + String tableName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.createStatement().execute("CREATE TABLE " + tableName + + " (id VARCHAR NOT NULL PRIMARY KEY, counter INTEGER)" + withClause(TTL, true)); + conn.commit(); + + EnvironmentEdgeManager.injectEdge(injectEdge); + + // First atomic upsert: the row does not exist, so the ON DUPLICATE clause is skipped and the + // UPSERT VALUES insert counter = 0. + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, counter) VALUES ('r1', 0) ON DUPLICATE KEY UPDATE counter = counter + 1"); + conn.commit(); + assertEquals("fresh insert of a new row", 0, atomicCounter(conn, tableName)); + + // Advance past the TTL. No flush/compact: the row's cells are still physically present, so + // only read masking - not compaction - can hide the now-expired row from the internal scan. + injectEdge.incrementValue((TTL + 1) * 1000L); + + // Second atomic upsert on the now-expired row. Post-fix the masked internal scan returns no + // current row, so this inserts counter = 0 again; pre-fix the unmasked scan resurrects the + // row and the ON DUPLICATE clause increments counter to 1. + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, counter) VALUES ('r1', 0) ON DUPLICATE KEY UPDATE counter = counter + 1"); + conn.commit(); + + assertEquals("expired row must be treated as absent, not resurrected and incremented", 0, + atomicCounter(conn, tableName)); + } + } + + @Test + public void testUpsertSelectTouchNearTtlBoundaryKeepsDataAndIndexConsistent() throws Exception { + runTtlBoundaryScenario(true); + } + + @Test + public void testUpsertValuesTouchNearTtlBoundaryKeepsDataAndIndexConsistent() throws Exception { + runTtlBoundaryScenario(false); + } + + @Test + public void testImmutableDifferentStorageSchemeIndexKeepsDataAndIndexConsistent() + throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + // The scheme mismatch (independent of the class columnEncoded parameter, which this test does + // not use) is the point: it forces server-side index maintenance and the masked current-row + // read. Direction ONE_CELL data -> SINGLE_CELL index is the only allowed one. + conn.createStatement().execute("CREATE IMMUTABLE TABLE " + tableName + + " (id VARCHAR NOT NULL PRIMARY KEY, idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR) " + + "TTL=" + TTL + ", IMMUTABLE_STORAGE_SCHEME=ONE_CELL_PER_COLUMN, COLUMN_ENCODED_BYTES=0"); + conn.createStatement() + .execute("CREATE INDEX " + indexName + " ON " + tableName + " (idxcol) INCLUDE (covcol) " + + "IMMUTABLE_STORAGE_SCHEME=SINGLE_CELL_ARRAY_WITH_OFFSETS, COLUMN_ENCODED_BYTES=2"); + conn.commit(); + + // Confirm the two sides really do use different storage schemes. + assertMetadata(conn, PTable.ImmutableStorageScheme.ONE_CELL_PER_COLUMN, + PTable.QualifierEncodingScheme.NON_ENCODED_QUALIFIERS, tableName); + assertMetadata(conn, PTable.ImmutableStorageScheme.SINGLE_CELL_ARRAY_WITH_OFFSETS, + PTable.QualifierEncodingScheme.TWO_BYTE_QUALIFIERS, indexName); + + // Freeze the clock at t0; every cell of the full initial row lands at t0. + long t0 = EnvironmentEdgeManager.currentTimeMillis(); + injectEdge.setValue(t0); + EnvironmentEdgeManager.injectEdge(injectEdge); + + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); + conn.commit(); + + // Push the t0 cells onto an HFile so the later major compaction merges two files. + flushTable(conn, tableName); + flushTable(conn, indexName); + + // Phase 1: a partial touch that omits covcol, committed WITHIN the TTL so the row is alive. + // The masked current-row read still returns covcol@t0, so the index is rebuilt WITH covcol + // and + // both sides agree covcol is present (non-null). + injectEdge.incrementValue(1000); + conn.createStatement() + .executeUpdate("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); + conn.commit(); + assertCovColConsistent(conn, tableName, indexName, "r1", "k1", COVCOL_VALUE); + + // Phase 2: a second partial touch that also omits covcol. It is READ while the row is still + // alive (one tick before the boundary) but COMMITTED past a > TTL gap, so its + // mutationTimestamp is well past covcol@t0's expiry. The masked read now drops covcol@t0, so + // the index is rebuilt WITHOUT covcol. + injectEdge.incrementValue(TTL * 1000L - 1); + conn.createStatement() + .executeUpdate("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x2')"); + injectEdge.incrementValue(2); + conn.commit(); + + assertDataTouchColAlive(conn, tableName, "r1", "x2"); + // Past the TTL boundary covcol reads NULL on both the data table and the index - agreement + // AND + // the specific expired value: the index was rebuilt without the stale covcol. + assertCovColAbsent(conn, tableName, indexName, "r1"); + } + } + + /** + * A major compaction that races an in-flight partial touch. This reproduces the narrow window the + * design notes call out: {@code preBatchMutateWithExceptions} releases the data-table row locks + * before the (potentially slow) first-phase index write and re-acquires them afterwards + * (IndexRegionObserver.java unlockRows before doPre, lockRows after), so a concurrent major + * compaction on the data table can run between the masked current-row read and the moment the + * touch's data-side cells are persisted. + *

        + * The interleaving is made deterministic by a {@link BlockingIndexWriteObserver} installed on the + * index table: the touch's first-phase index write (doPre -> table.batch to the index) blocks in + * the index region's preBatchMutate, which is strictly after the data-table current-row read and + * strictly before doPre completes. While it is parked we fire the major compaction on the data + * table, then release the index write. + *

        + * When the current row is read the clock is well within the TTL, so the masked read returns + * covcol@t0 and the index is rebuilt with covcol. When the compaction runs the clock has advanced + * past the TTL but not past TTL + max-lookback, so covcol@t0 is older than the row's freshest + * cell by more than the TTL yet is retained physically: CompactionScanner keeps the last row + * version unless {@code compactionTime - maxTimestamp > maxLookbackInMillis + ttl} + * (CompactionScanner.retainCellsOfLastRowVersion), which is false here (65s <= 60s + 10s). The + * non-zero max-lookback is exactly what makes the race harmless: the pre-existing covcol@t0 + * cannot be collected out from under the index rebuild, so the data table and the index still + * agree covcol is present after the touch commits. + */ + @Test + public void testConcurrentMajorCompactionDuringIndexWriteKeepsDataAndIndexConsistent() + throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement() + .execute("CREATE TABLE " + tableName + " (id VARCHAR NOT NULL PRIMARY KEY, " + + "idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR)" + withClause(TTL, true)); + conn.createStatement() + .execute("CREATE INDEX " + indexName + " ON " + tableName + " (idxcol) INCLUDE (covcol)"); + conn.commit(); + + // Freeze the clock at t0; every cell of the full initial row lands at t0. + long t0 = EnvironmentEdgeManager.currentTimeMillis(); + injectEdge.setValue(t0); + EnvironmentEdgeManager.injectEdge(injectEdge); + + // Full initial row: covcol is written once here and never again. Flush so covcol@t0 is on an + // HFile and the later major compaction has a file to rewrite. + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); + conn.commit(); + flushTable(conn, tableName); + flushTable(conn, indexName); + + // Arm the index-table observer so the NEXT index write (the touch's doPre) parks mid-flight, + // then advance to a time still comfortably within the TTL: the touch's current-row read sees + // covcol@t0 as alive and rebuilds the index with covcol. + CountDownLatch indexWriteReached = new CountDownLatch(1); + CountDownLatch indexWriteProceed = new CountDownLatch(1); + BlockingIndexWriteObserver.arm(indexName, indexWriteReached, indexWriteProceed); + TestUtil.addCoprocessor(conn, indexName, BlockingIndexWriteObserver.class); + injectEdge.setValue(t0 + 30_000L); + + // Run the touch on its own thread/connection: it parks inside the index region's + // preBatchMutate (i.e. inside doPre) after the data-table read and after the data locks were + // released, letting us drive the compaction while it is parked. + AtomicReference touchError = new AtomicReference<>(); + Thread toucher = new Thread(() -> { + try (Connection tc = DriverManager.getConnection(getUrl())) { + tc.setAutoCommit(false); + tc.createStatement() + .executeUpdate("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); + tc.commit(); + } catch (Exception e) { + touchError.set(e); + } + }, "ttl-sync-toucher"); + toucher.start(); + + // Wait until the touch's index write has parked (read done, doPre not yet complete). + assertTrue("index write did not reach the observer in time", + indexWriteReached.await(60, TimeUnit.SECONDS)); + + // The row is now past the TTL (65s) but not past TTL + max-lookback (70s), so the concurrent + // major compaction on the data table must NOT physically collect covcol@t0. + injectEdge.setValue(t0 + 65_000L); + TestUtil.majorCompact(getUtility(), TableName.valueOf(tableName)); + + // Release the parked index write and let the touch finish its data-side persist. + indexWriteProceed.countDown(); + toucher.join(TimeUnit.SECONDS.toMillis(60)); + assertFalse("toucher thread did not finish", toucher.isAlive()); + if (touchError.get() != null) { + throw touchError.get(); + } + + // The touch's fresh heartbeat (@t0+30s) keeps the row alive at the current clock (t0+65s, gap + // 35s < TTL). covcol@t0 survived the concurrent compaction, so the data table and the index + // still agree covcol is present. + assertCovColConsistent(conn, tableName, indexName, "r1", "k1", COVCOL_VALUE); + } + } + + private void runTtlBoundaryScenario(boolean useUpsertSelect) throws Exception { + String tableName = generateUniqueName(); + String indexName = generateUniqueName(); + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement() + .execute("CREATE TABLE " + tableName + " (id VARCHAR NOT NULL PRIMARY KEY, " + + "idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR)" + withClause(TTL, true)); + conn.createStatement() + .execute("CREATE INDEX " + indexName + " ON " + tableName + " (idxcol) INCLUDE (covcol)"); + conn.commit(); + + // Freeze the clock at t0; every cell of the full initial row lands at t0. + long t0 = EnvironmentEdgeManager.currentTimeMillis(); + injectEdge.setValue(t0); + EnvironmentEdgeManager.injectEdge(injectEdge); + + conn.createStatement().execute("UPSERT INTO " + tableName + + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); + conn.commit(); + + // Push the t0 cells onto an HFile so the later major compaction merges two files. + flushTable(conn, tableName); + flushTable(conn, indexName); + + // Read the partial touch while the row is still alive (one tick before the boundary) but do + // NOT commit yet. For the UPSERT SELECT variant the inner SELECT reads the live row here. + injectEdge.incrementValue(TTL * 1000L - 1); + String expectedTouch; + if (useUpsertSelect) { + int affected = conn.createStatement().executeUpdate("UPSERT INTO " + tableName + + " (id, touchcol) SELECT id, touchcol FROM " + tableName + " WHERE id = 'r1'"); + assertEquals("UPSERT SELECT must read the still-alive row and buffer one touch", 1, + affected); + expectedTouch = "x0"; + } else { + conn.createStatement() + .executeUpdate("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); + expectedTouch = "x1"; + } + + // Advance two ticks past the boundary and commit: the touch's mutationTimestamp is now + // t0 + TTL + 1 (gap > TTL). idxcol@t0 / covcol@t0 are masked at the internal read anchored at + // mutationTimestamp, so the index is rebuilt without them - matching the data side. + injectEdge.incrementValue(2); + conn.commit(); + + // Settle each side's physical state; the major compaction merges the t0 HFile with this one. + flushAndMajorCompact(conn, tableName); + flushAndMajorCompact(conn, indexName); + + // Data path (forced data-table read): the row is alive by PK, covcol read past the boundary. + String dataCov; + String dataTouch; + try (ResultSet rs = conn.createStatement().executeQuery( + "SELECT /*+ NO_INDEX */ covcol, touchcol FROM " + tableName + " WHERE id = 'r1'")) { + assertTrue("data-table row must still exist (touchcol keeps it alive)", rs.next()); + dataCov = rs.getString(1); + dataTouch = rs.getString(2); + assertFalse("exactly one data-table row", rs.next()); + } + + // Index path (forced index read by PK): idxcol@t0 is masked past the boundary, so the row is + // reachable through the covered index only by its PK suffix, not by idxcol='k1'. + String indexCov; + try (ResultSet rs = conn.createStatement().executeQuery("SELECT /*+ INDEX(" + tableName + " " + + indexName + ") */ covcol FROM " + tableName + " WHERE id = 'r1'")) { + assertTrue("index-visible row must exist", rs.next()); + indexCov = rs.getString(1); + assertFalse("exactly one index-visible row", rs.next()); + } + + assertEquals("touchcol must survive (row alive)", expectedTouch, dataTouch); + assertEquals("covcol must agree between data and index", dataCov, indexCov); + } + } + + // ---- helpers ---- + + /** + * Reads the single {@code counter} value for row {@code r1}, asserting exactly one live row. Used + * by {@link #testNoIndexAtomicUpsertMasksExpiredRow} to observe the atomic upsert's result. + */ + private static int atomicCounter(Connection conn, String tableName) throws SQLException { + try (ResultSet rs = conn.createStatement() + .executeQuery("SELECT counter FROM " + tableName + " WHERE id = 'r1'")) { + assertTrue("row should be present", rs.next()); + int value = rs.getInt(1); + assertFalse("expected exactly one row", rs.next()); + return value; + } + } + + private void assertCovColConsistent(Connection conn, String queryTableName, String indexName, + String id, String idxColValue, String expectedCovVal) throws SQLException { + // Force the data-table read. + try (ResultSet rs = conn.createStatement().executeQuery( + "SELECT /*+ NO_INDEX */ covcol FROM " + queryTableName + " WHERE id = '" + id + "'")) { + assertTrue("data-table row should exist", rs.next()); + assertEquals("data-table covcol", expectedCovVal, rs.getString(1)); + assertFalse(rs.next()); + } + // Read via the index (idxcol is the leading index column). + try (ResultSet rs = + conn.createStatement().executeQuery("SELECT /*+ INDEX(" + queryTableName + " " + indexName + + ") */ id, covcol FROM " + queryTableName + " WHERE idxcol = '" + idxColValue + "'")) { + assertTrue("index-visible row should exist", rs.next()); + assertEquals("index id", id, rs.getString(1)); + assertEquals("index covcol", expectedCovVal, rs.getString(2)); + assertFalse("exactly one index-visible row", rs.next()); + } + } + + private void assertCovColAbsent(Connection conn, String queryTableName, String indexName, + String id) throws SQLException { + try (ResultSet rs = conn.createStatement().executeQuery( + "SELECT /*+ NO_INDEX */ covcol FROM " + queryTableName + " WHERE id = '" + id + "'")) { + assertTrue("data-table row should exist", rs.next()); + assertNull("data-table row should not have covcol", rs.getString(1)); + assertFalse("exactly one data-table row", rs.next()); + } + try (ResultSet rs = conn.createStatement().executeQuery("SELECT /*+ INDEX(" + queryTableName + + " " + indexName + ") */ covcol FROM " + queryTableName + " WHERE id = '" + id + "'")) { + assertTrue("index-visible row should exist", rs.next()); + assertNull("index-visible row should not have covcol", rs.getString(1)); + assertFalse("exactly one index-visible row", rs.next()); + } + } + + private void assertTouchColConsistent(Connection conn, String queryTableName, String indexName, + String id, String expectedTouchVal) throws SQLException { + try (ResultSet rs = conn.createStatement().executeQuery( + "SELECT /*+ NO_INDEX */ touchcol FROM " + queryTableName + " WHERE id = '" + id + "'")) { + assertTrue("data-table row should exist", rs.next()); + assertEquals("data-table touchcol", expectedTouchVal, rs.getString(1)); + assertFalse(rs.next()); + } + try (ResultSet rs = conn.createStatement().executeQuery("SELECT /*+ INDEX(" + queryTableName + + " " + indexName + ") */ touchcol FROM " + queryTableName + " WHERE id = '" + id + "'")) { + assertTrue("index-visible row should exist", rs.next()); + assertEquals("index touchcol", expectedTouchVal, rs.getString(1)); + assertFalse("exactly one index-visible row", rs.next()); + } + } + + /** + * Data-side-only liveness check for the uncovered-index case, where touchcol cannot be read + * through the index (an uncovered index stores no data columns and is reachable only by its + * indexed column). Asserts the data row is alive and carries the expected touchcol. + */ + private void assertDataTouchColAlive(Connection conn, String queryTableName, String id, + String expectedTouchVal) throws SQLException { + try (ResultSet rs = conn.createStatement().executeQuery( + "SELECT /*+ NO_INDEX */ touchcol FROM " + queryTableName + " WHERE id = '" + id + "'")) { + assertTrue("data-table row should exist", rs.next()); + assertEquals("data-table touchcol", expectedTouchVal, rs.getString(1)); + assertFalse(rs.next()); + } + } + + private void assertUncoveredIdxColConsistent(Connection conn, String queryTableName, + String indexName, String id, String idxColValue) throws SQLException { + try (ResultSet rs = + conn.createStatement().executeQuery("SELECT /*+ INDEX(" + queryTableName + " " + indexName + + ") */ id, idxcol FROM " + queryTableName + " WHERE idxcol = '" + idxColValue + "'")) { + assertTrue("index-visible row should exist", rs.next()); + assertEquals("index id", id, rs.getString(1)); + assertEquals("index idxcol", idxColValue, rs.getString(2)); + assertFalse("exactly one index-visible row", rs.next()); + } + } + + private void assertUncoveredIdxColAbsent(Connection conn, String queryTableName, String indexName, + String idxColValue) throws SQLException { + try (ResultSet rs = + conn.createStatement().executeQuery("SELECT /*+ INDEX(" + queryTableName + " " + indexName + + ") */ idxcol FROM " + queryTableName + " WHERE idxcol = '" + idxColValue + "'")) { + assertFalse("no index-visible row should exist", rs.next()); + } + } + + static void flushTable(Connection conn, String tableName) throws Exception { + try (Admin admin = conn.unwrap(PhoenixConnection.class).getQueryServices().getAdmin()) { + admin.flush(TableName.valueOf(tableName)); + } + } + + static void flushAndMajorCompact(Connection conn, String tableName) throws Exception { + TableName tn = TableName.valueOf(tableName); + try (Admin admin = conn.unwrap(PhoenixConnection.class).getQueryServices().getAdmin()) { + admin.flush(tn); + } + TestUtil.majorCompact(getUtility(), tn); + } + + /** + * A region observer installed on the index table that parks the first write to a named index in + * {@code preBatchMutate}, letting a test run a concurrent major compaction on the data table + * while the secondary-index write is in flight. This is the "slow index write" seam: the index + * write is driven by {@code IndexRegionObserver.doPre -> table.batch(...)}, which reaches the + * index region's {@code preBatchMutate} strictly after the data-table current-row read and + * strictly before doPre returns. It is a one-shot per arming so only the touch's write blocks, + * not later maintenance. + */ + public static class BlockingIndexWriteObserver extends SimpleRegionObserver { + private static volatile String targetIndexName; + private static volatile CountDownLatch reached; + private static volatile CountDownLatch proceed; + private static final AtomicBoolean fired = new AtomicBoolean(false); + + static void arm(String indexName, CountDownLatch reachedLatch, CountDownLatch proceedLatch) { + targetIndexName = indexName; + reached = reachedLatch; + proceed = proceedLatch; + fired.set(false); + } + + @Override + public void preBatchMutate(ObserverContext c, + MiniBatchOperationInProgress miniBatchOp) throws IOException { + String tableName = c.getEnvironment().getRegionInfo().getTable().getNameAsString(); + if (tableName.equals(targetIndexName) && fired.compareAndSet(false, true)) { + reached.countDown(); + try { + proceed.await(60, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("interrupted while parking index write", e); + } + } + } + } +} diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java deleted file mode 100644 index ee491ab464c..00000000000 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexDataTableTTLSyncIT.java +++ /dev/null @@ -1,538 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.phoenix.end2end.index; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; - -import java.io.IOException; -import java.sql.Connection; -import java.sql.DriverManager; -import java.sql.ResultSet; -import java.sql.SQLException; -import java.util.Arrays; -import java.util.Collection; -import java.util.Map; -import org.apache.hadoop.hbase.Cell; -import org.apache.hadoop.hbase.CellScanner; -import org.apache.hadoop.hbase.TableName; -import org.apache.hadoop.hbase.client.Admin; -import org.apache.hadoop.hbase.client.Result; -import org.apache.hadoop.hbase.client.ResultScanner; -import org.apache.hadoop.hbase.client.Scan; -import org.apache.hadoop.hbase.client.Table; -import org.apache.hadoop.hbase.util.Bytes; -import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants; -import org.apache.phoenix.end2end.NeedsOwnMiniClusterTest; -import org.apache.phoenix.jdbc.PhoenixConnection; -import org.apache.phoenix.query.BaseTest; -import org.apache.phoenix.query.ConnectionQueryServices; -import org.apache.phoenix.query.QueryServices; -import org.apache.phoenix.util.EnvironmentEdgeManager; -import org.apache.phoenix.util.ManualEnvironmentEdge; -import org.apache.phoenix.util.ReadOnlyProps; -import org.apache.phoenix.util.TestUtil; -import org.junit.After; -import org.junit.Assert; -import org.junit.Before; -import org.junit.BeforeClass; -import org.junit.Test; -import org.junit.experimental.categories.Category; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; - -import org.apache.phoenix.thirdparty.com.google.common.collect.Maps; - -/** - * Verifies that internal server-side index-maintenance current-row reads honor Phoenix TTL exactly - * like a client read, and that the read is anchored at the batch timestamp so the data table and a - * global index stay aligned under compaction. - *

        - * The scenario is the divergence being fixed: a covered column ({@code covcol}) is written once, - * then never re-written by later partial "touch" upserts. On the index side {@code covcol} is - * rebuilt at every touch's {@code batchTimestamp} from the masked current-row read; on the data side - * {@code covcol} keeps its original timestamp. The fix anchors that internal read's masking clock at - * {@code batchTimestamp} (the same instant the index is built at) via - * {@code scan.setTimeRange(0, batchTimestamp)}, so both sides always agree on whether {@code covcol} - * is still alive: whatever the read masks the index rebuild omits, and whatever the read keeps a - * later major compaction keeps on the data side too. The row and its index then expire as a unit - * rather than {@code covcol} being dropped on only one side. - *

        - * Because the fix no longer re-stamps the data-side cell, the load-bearing, timing-independent - * signal is: after a covcol-omitting touch, a raw scan's maximum {@code covcol} timestamp on the - * data side stays at its original write time (it does NOT advance to the touch time), while the - * data read and the index read still agree on the value. Under a {@link ManualEnvironmentEdge} the - * server's {@code batchTimestamp} equals the touch's wall clock, making this deterministic. - *

        - * {@link #testNoIndexAtomicUpsertMasksExpiredRow} covers the no-index masking path: an atomic - * {@code ON DUPLICATE KEY UPDATE} on a TTL table with no secondary index still triggers an internal - * current-row read, but there is no {@code IndexMaintainer} to supply the empty-column CF/CQ. That - * scan is masked using the CF/CQ the client threads on the mutation, so an expired row is treated as - * absent rather than resurrected. - */ -@Category(NeedsOwnMiniClusterTest.class) -@RunWith(Parameterized.class) -public class IndexDataTableTTLSyncIT extends BaseTest { - static final int MAX_LOOKBACK_AGE = 10; - static final int TTL = 60; - static final String COVCOL_VALUE = "cov-original"; - static final String IDXCOL_VALUE = "idx-original"; - - private final boolean columnEncoded; - private ManualEnvironmentEdge injectEdge; - - public IndexDataTableTTLSyncIT(boolean columnEncoded) { - this.columnEncoded = columnEncoded; - } - - @Parameterized.Parameters(name = "columnEncoded={0}") - public static synchronized Collection data() { - return Arrays.asList(new Object[][] { { false }, { true } }); - } - - @BeforeClass - public static synchronized void doSetup() throws Exception { - setUpTestDriver(new ReadOnlyProps(baseServerProps().entrySet().iterator())); - } - - /** - * Common server props for the TTL-sync IT: max-lookback and immediate global-index row aging so a - * major compaction deterministically exercises the covered-column timestamp-skew divergence. - */ - static Map baseServerProps() { - Map props = Maps.newHashMapWithExpectedSize(4); - props.put(QueryServices.GLOBAL_INDEX_ROW_AGE_THRESHOLD_TO_DELETE_MS_ATTRIB, Long.toString(0)); - props.put(BaseScannerRegionObserverConstants.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, - Integer.toString(MAX_LOOKBACK_AGE)); - props.put("hbase.procedure.remote.dispatcher.delay.msec", "0"); - // The view case threads the view's literal TTL as the per-mutation _TTL attribute. - props.put(QueryServices.PHOENIX_VIEW_TTL_ENABLED, Boolean.toString(true)); - return props; - } - - @Before - public void beforeTest() { - EnvironmentEdgeManager.reset(); - injectEdge = new ManualEnvironmentEdge(); - injectEdge.setValue(EnvironmentEdgeManager.currentTimeMillis()); - } - - @After - public synchronized void afterTest() throws Exception { - boolean refCountLeaked = isAnyStoreRefCountLeaked(); - EnvironmentEdgeManager.reset(); - Assert.assertFalse("refCount leaked", refCountLeaked); - } - - /** - * Builds the comma-separated table-property clause. When {@code ttlSeconds >= 0} a {@code TTL} is - * emitted as the first property; {@code COLUMN_ENCODED_BYTES} always follows, and - * {@code IS_STRICT_TTL=false} is appended for a non-strict table. Properties are comma-separated - * (a bare space between properties is a Phoenix parser error). - */ - private String withClause(int ttlSeconds, boolean strict) { - StringBuilder sb = new StringBuilder(" "); - if (ttlSeconds >= 0) { - sb.append("TTL=").append(ttlSeconds).append(", "); - } - sb.append("COLUMN_ENCODED_BYTES=").append(columnEncoded ? 2 : 0); - if (!strict) { - sb.append(", IS_STRICT_TTL=false"); - } - return sb.toString(); - } - - /** - * Base table with a literal TTL and a covered global index. After a partial touch that never - * writes covcol, the data-side covcol keeps its original timestamp (the fix does not re-stamp it); - * the masked internal read anchored at batchTimestamp keeps the data and index agreeing on covcol - * while the row is alive, and a full expiry after that shows the data row and the index expiring as - * a unit. - */ - @Test - public void testBaseTableCoveredColumnResync() throws Exception { - String tableName = generateUniqueName(); - String indexName = generateUniqueName(); - try (Connection conn = DriverManager.getConnection(getUrl())) { - conn.createStatement() - .execute("CREATE TABLE " + tableName + " (id VARCHAR NOT NULL PRIMARY KEY, " - + "idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR)" + withClause(TTL, true)); - conn.createStatement().execute( - "CREATE INDEX " + indexName + " ON " + tableName + " (idxcol) INCLUDE (covcol)"); - conn.commit(); - - EnvironmentEdgeManager.injectEdge(injectEdge); - - // Full initial upsert: covcol is written exactly once here and never again. - conn.createStatement().execute("UPSERT INTO " + tableName - + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); - conn.commit(); - - // Partial touch that does not write covcol, well within the TTL so the row is alive. - injectEdge.incrementValue(1000); - long touchTime = injectEdge.currentTime(); - conn.createStatement() - .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); - conn.commit(); - - // Load-bearing, deterministic signal: the touch does NOT write covcol and the fix does not - // re-stamp it, so the data-side covcol keeps its original write timestamp - it stays strictly - // below the touch time. (Pre-fix the re-sync advanced it to batchTimestamp == touchTime.) - long dataCovTs = - maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), COVCOL_VALUE); - assertTrue("covcol should keep its original write timestamp on the data side; touchTime=" - + touchTime + " dataCovTs=" + dataCovTs, dataCovTs < touchTime); - - // Data and index agree on covcol: the internal scan anchored at batchTimestamp masks covcol - // identically to how the index is rebuilt, so both sides see the same live value. - assertCovColConsistent(conn, tableName, "r1", "k1", COVCOL_VALUE); - - // Advancing past TTL + max-lookback and compacting expires the row on BOTH sides together: the - // whole row version is older than maxLookback + ttl, so compaction purges the data row and the - // index row as a unit. - injectEdge.incrementValue((TTL + MAX_LOOKBACK_AGE + 1) * 1000L); - flushAndMajorCompact(conn, tableName); - flushAndMajorCompact(conn, indexName); - assertCovColAbsent(conn, tableName, "r1", "k1"); - } - } - - /** - * Uncovered global index on a base table with a literal TTL. An uncovered index stores its - * indexed column ({@code idxcol}) positionally in the index row key, rebuilt at every touch's - * {@code batchTimestamp}, while a partial touch that omits {@code idxcol} would otherwise leave - * the data-side {@code idxcol} cell at its original timestamp. A later major compaction can then - * trim the stale data-side {@code idxcol} while the row stays alive, so the index key encodes a - * value the data row no longer has — and because the uncovered read path re-verifies the rebuilt - * key against the data row, the live row silently drops out of an {@code idxcol} predicate rather - * than surfacing the stale value. Anchoring the internal read at {@code batchTimestamp} covers - * uncovered indexes too: the index key is rebuilt from the same masked current-row state, so it - * only ever encodes a value the anchored read still sees alive, and compaction converges the data - * side to the same keep/drop decision. - *

        - * Load-bearing, timing-independent signal (mirrors {@link #testBaseTableCoveredColumnResync}): - * after an {@code idxcol}-omitting touch of a still-alive row, a raw scan's maximum {@code idxcol} - * timestamp on the data side stays at its original write time (the fix does not re-stamp it), while - * the row is still retrievable through the uncovered index because both sides agree the value is - * alive. - */ - @Test - public void testUncoveredIndexIndexedColumnResync() throws Exception { - String tableName = generateUniqueName(); - String indexName = generateUniqueName(); - try (Connection conn = DriverManager.getConnection(getUrl())) { - conn.createStatement() - .execute("CREATE TABLE " + tableName + " (id VARCHAR NOT NULL PRIMARY KEY, " - + "idxcol VARCHAR, touchcol VARCHAR)" + withClause(TTL, true)); - conn.createStatement() - .execute("CREATE UNCOVERED INDEX " + indexName + " ON " + tableName + " (idxcol)"); - conn.commit(); - - EnvironmentEdgeManager.injectEdge(injectEdge); - - // Full initial upsert: idxcol is written exactly once here and never again. - conn.createStatement().execute("UPSERT INTO " + tableName - + " (id, idxcol, touchcol) VALUES ('r1', '" + IDXCOL_VALUE + "', 'x0')"); - conn.commit(); - - // Partial touch that does not write idxcol, well within the TTL so the row is alive and the - // internal scan anchored at batchTimestamp still returns idxcol for the index rebuild. - injectEdge.incrementValue(1000); - long touchTime = injectEdge.currentTime(); - conn.createStatement() - .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); - conn.commit(); - - // The touch does not write idxcol and the fix does not re-stamp it, so the data-side idxcol - // keeps its original write timestamp - strictly below the touch time. (Pre-fix the re-sync - // advanced it to batchTimestamp == touchTime.) - long dataIdxTs = - maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), IDXCOL_VALUE); - assertTrue("uncovered idxcol should keep its original write timestamp on the data side; " - + "touchTime=" + touchTime + " dataIdxTs=" + dataIdxTs, dataIdxTs < touchTime); - - // The row is retrievable through the uncovered index (join-back + key re-verification passes - // because the data-side idxcol still encodes the same key) and touchcol reflects the touch. - assertEquals("touchcol via uncovered index", "x1", - touchColViaUncoveredIndex(conn, tableName, indexName, IDXCOL_VALUE)); - - // Advancing past TTL + max-lookback and compacting expires the row on BOTH sides together. - injectEdge.incrementValue((TTL + MAX_LOOKBACK_AGE + 1) * 1000L); - flushAndMajorCompact(conn, tableName); - flushAndMajorCompact(conn, indexName); - try (ResultSet rs = conn.createStatement().executeQuery("SELECT /*+ NO_INDEX */ touchcol FROM " - + tableName + " WHERE id = 'r1'")) { - assertFalse("data-table row should be expired", rs.next()); - } - assertNull("row should be expired via the uncovered index too", - touchColViaUncoveredIndex(conn, tableName, indexName, IDXCOL_VALUE)); - } - } - - /** - * View with a view-level literal TTL (not on the shared CF descriptor) and a covered index on the - * view. Exercises the literal-_TTL per-mutation threading: the internal scan masks with the view's - * TTL, anchored at batchTimestamp, and covcol keeps its original data-side timestamp just like the - * base-table case while the data and index reads stay consistent. - */ - @Test - public void testViewCoveredColumnResync() throws Exception { - String baseTableName = generateUniqueName(); - String viewName = generateUniqueName(); - String indexName = generateUniqueName(); - try (Connection conn = DriverManager.getConnection(getUrl())) { - // Base table carries NO TTL; the TTL lives only on the view. - conn.createStatement() - .execute("CREATE TABLE " + baseTableName + " (id VARCHAR NOT NULL PRIMARY KEY, " - + "idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR)" + withClause(-1, true)); - conn.createStatement().execute( - "CREATE VIEW " + viewName + " AS SELECT * FROM " + baseTableName + " TTL=" + TTL); - conn.createStatement() - .execute("CREATE INDEX " + indexName + " ON " + viewName + " (idxcol) INCLUDE (covcol)"); - conn.commit(); - - EnvironmentEdgeManager.injectEdge(injectEdge); - - conn.createStatement().execute("UPSERT INTO " + viewName - + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); - conn.commit(); - - injectEdge.incrementValue(1000); - long touchTime = injectEdge.currentTime(); - conn.createStatement() - .execute("UPSERT INTO " + viewName + " (id, touchcol) VALUES ('r1', 'x1')"); - conn.commit(); - - long dataCovTs = maxTimestampForValue(conn, TableName.valueOf(baseTableName), - Bytes.toBytes("r1"), COVCOL_VALUE); - assertTrue("covcol should keep its original write timestamp on the view's data side; touchTime=" - + touchTime + " dataCovTs=" + dataCovTs, dataCovTs < touchTime); - - assertCovColConsistent(conn, viewName, "r1", "k1", COVCOL_VALUE); - } - } - - /** - * A non-strict table must not be over-masked: the strictness flag is threaded to the internal - * scan, so a value that a non-strict client read still returns is retained rather than masked out - * of the index rebuild. If strictness were NOT ported, the internal scan would default to strict - * and mask covcol away past the TTL, so the index would be rebuilt without covcol and the - * data/index consistency check would fail. The load-bearing check here is therefore - * {@link #assertCovColConsistent}; the raw-scan timestamp probe only confirms the covcol cell is - * still physically present at its original write time (raw scans see cells regardless of masking). - */ - @Test - public void testNonStrictTableNotOverMasked() throws Exception { - String tableName = generateUniqueName(); - String indexName = generateUniqueName(); - try (Connection conn = DriverManager.getConnection(getUrl())) { - conn.createStatement() - .execute("CREATE TABLE " + tableName + " (id VARCHAR NOT NULL PRIMARY KEY, " - + "idxcol VARCHAR, covcol VARCHAR, touchcol VARCHAR)" + withClause(TTL, false)); - conn.createStatement().execute( - "CREATE INDEX " + indexName + " ON " + tableName + " (idxcol) INCLUDE (covcol)"); - conn.commit(); - - EnvironmentEdgeManager.injectEdge(injectEdge); - - conn.createStatement().execute("UPSERT INTO " + tableName - + " (id, idxcol, covcol, touchcol) VALUES ('r1', 'k1', '" + COVCOL_VALUE + "', 'x0')"); - conn.commit(); - - // Advance beyond TTL, then touch. A strict table would mask covcol at the internal scan; - // a non-strict table must not. - injectEdge.incrementValue((TTL + 5) * 1000L); - long touchTime = injectEdge.currentTime(); - conn.createStatement() - .execute("UPSERT INTO " + tableName + " (id, touchcol) VALUES ('r1', 'x1')"); - conn.commit(); - - long dataCovTs = - maxTimestampForValue(conn, TableName.valueOf(tableName), Bytes.toBytes("r1"), COVCOL_VALUE); - assertTrue("non-strict covcol should be retained at its original write timestamp, not masked " - + "away; touchTime=" + touchTime + " dataCovTs=" + dataCovTs, dataCovTs < touchTime); - - // The decisive check: a non-strict internal read is not over-masked, so the index is rebuilt - // with covcol and both the data read and the index read still return it. - assertCovColConsistent(conn, tableName, "r1", "k1", COVCOL_VALUE); - } - } - - /** - * The no-index masking path: an atomic {@code ON DUPLICATE KEY UPDATE} on a base table with a - * literal TTL and NO secondary index. The atomic path still opens an internal current-row read - * ({@code getCurrentRowStates} fires for {@code hasAtomic}), but there is no - * {@code IndexMaintainer} to supply the empty-column CF/CQ that let {@code TTLRegionScanner} mask - * the scan. Those are instead threaded on the mutation by the client - * ({@code ScanUtil.annotateMutationWithLiteralTTL}) and captured server-side, so the internal scan - * masks an expired row exactly like a client read — treating it as absent rather than resurrecting - * it. - *

        - * {@code counter = counter + 1} reads the current {@code counter} from the masked current-row - * state, so it is the sharpest probe of masking. After the row TTL-expires, a masked scan returns - * no current row, the {@code ON DUPLICATE KEY} clause is skipped, and the {@code UPSERT VALUES} - * ({@code counter = 0}) are inserted fresh. Pre-fix the unmasked scan resurrects the expired row - * and increments {@code counter} to 1. No flush/compaction is involved: the expired cells are - * still physically present, so read masking alone — not compaction — governs the outcome. - */ - @Test - public void testNoIndexAtomicUpsertMasksExpiredRow() throws Exception { - String tableName = generateUniqueName(); - try (Connection conn = DriverManager.getConnection(getUrl())) { - conn.createStatement().execute("CREATE TABLE " + tableName - + " (id VARCHAR NOT NULL PRIMARY KEY, counter INTEGER)" + withClause(TTL, true)); - conn.commit(); - - EnvironmentEdgeManager.injectEdge(injectEdge); - - // First atomic upsert: the row does not exist, so the ON DUPLICATE clause is skipped and the - // UPSERT VALUES insert counter = 0. - conn.createStatement().execute("UPSERT INTO " + tableName - + " (id, counter) VALUES ('r1', 0) ON DUPLICATE KEY UPDATE counter = counter + 1"); - conn.commit(); - assertEquals("fresh insert of a new row", 0, atomicCounter(conn, tableName)); - - // Advance past the TTL. No flush/compact: the row's cells are still physically present, so - // only read masking - not compaction - can hide the now-expired row from the internal scan. - injectEdge.incrementValue((TTL + 1) * 1000L); - - // Second atomic upsert on the now-expired row. Post-fix the masked internal scan returns no - // current row, so this inserts counter = 0 again; pre-fix the unmasked scan resurrects the - // row and the ON DUPLICATE clause increments counter to 1. - conn.createStatement().execute("UPSERT INTO " + tableName - + " (id, counter) VALUES ('r1', 0) ON DUPLICATE KEY UPDATE counter = counter + 1"); - conn.commit(); - - assertEquals("expired row must be treated as absent, not resurrected and incremented", 0, - atomicCounter(conn, tableName)); - } - } - - // ---- helpers ---- - - /** - * Reads the single {@code counter} value for row {@code r1}, asserting exactly one live row. Used - * by {@link #testNoIndexAtomicUpsertMasksExpiredRow} to observe the atomic upsert's result. - */ - private static int atomicCounter(Connection conn, String tableName) throws SQLException { - try (ResultSet rs = conn.createStatement() - .executeQuery("SELECT counter FROM " + tableName + " WHERE id = 'r1'")) { - assertTrue("row should be present", rs.next()); - int value = rs.getInt(1); - assertFalse("expected exactly one row", rs.next()); - return value; - } - } - - private void assertCovColConsistent(Connection conn, String queryTableName, String id, - String idxColValue, String expectedCovVal) throws SQLException { - // Force the data-table read. - try (ResultSet rs = conn.createStatement().executeQuery("SELECT /*+ NO_INDEX */ covcol FROM " - + queryTableName + " WHERE id = '" + id + "'")) { - assertTrue("data-table row should exist", rs.next()); - assertEquals("data-table covcol", expectedCovVal, rs.getString(1)); - assertFalse(rs.next()); - } - // Read via the index (idxcol is the leading index column). - try (ResultSet rs = conn.createStatement().executeQuery( - "SELECT covcol FROM " + queryTableName + " WHERE idxcol = '" + idxColValue + "'")) { - assertTrue("index-visible row should exist", rs.next()); - assertEquals("index covcol", expectedCovVal, rs.getString(1)); - assertFalse(rs.next()); - } - } - - private void assertCovColAbsent(Connection conn, String queryTableName, String id, - String idxColValue) throws SQLException { - try (ResultSet rs = conn.createStatement().executeQuery("SELECT /*+ NO_INDEX */ covcol FROM " - + queryTableName + " WHERE id = '" + id + "'")) { - assertFalse("data-table row should be expired", rs.next()); - } - try (ResultSet rs = conn.createStatement().executeQuery( - "SELECT covcol FROM " + queryTableName + " WHERE idxcol = '" + idxColValue + "'")) { - assertFalse("index-visible row should be expired", rs.next()); - } - } - - /** - * Raw-scans a single data-table row and returns the maximum timestamp among cells whose value - * equals {@code value}. Matching by value bytes is column-encoding independent (encoding rewrites - * qualifiers, not VARCHAR values), so this works for both COLUMN_ENCODED_BYTES=0 and 2. - */ - static long maxTimestampForValue(Connection conn, TableName tableName, byte[] rowKey, String value) - throws SQLException, IOException { - byte[] valueBytes = Bytes.toBytes(value); - ConnectionQueryServices cqs = conn.unwrap(PhoenixConnection.class).getQueryServices(); - long maxTs = -1L; - try (Table table = cqs.getTable(tableName.getName())) { - Scan scan = new Scan(); - scan.withStartRow(rowKey, true); - scan.withStopRow(rowKey, true); - scan.setRaw(true); - scan.readAllVersions(); - try (ResultScanner scanner = table.getScanner(scan)) { - Result result; - while ((result = scanner.next()) != null) { - CellScanner cellScanner = result.cellScanner(); - while (cellScanner.advance()) { - Cell cell = cellScanner.current(); - if ( - Bytes.equals(valueBytes, 0, valueBytes.length, cell.getValueArray(), - cell.getValueOffset(), cell.getValueLength()) - ) { - maxTs = Math.max(maxTs, cell.getTimestamp()); - } - } - } - } - } - return maxTs; - } - - static void flushAndMajorCompact(Connection conn, String tableName) throws Exception { - TableName tn = TableName.valueOf(tableName); - try (Admin admin = conn.unwrap(PhoenixConnection.class).getQueryServices().getAdmin()) { - admin.flush(tn); - } - TestUtil.majorCompact(getUtility(), tn); - } - - /** - * Reads {@code touchcol} for the given indexed value through the uncovered index, forcing the - * join-back-and-verify read path ({@code touchcol} is neither an index key nor a covered column, - * so the query must join back to the data table and the uncovered scanner re-verifies the index - * key against the data row). Returns the {@code touchcol} value, or {@code null} if the uncovered - * read excludes the row — which happens when the data-side indexed column was trimmed by - * compaction so the rebuilt index key no longer matches the stored one. - */ - static String touchColViaUncoveredIndex(Connection conn, String tableName, String indexName, - String idxColValue) throws SQLException { - String sql = "SELECT /*+ INDEX(" + tableName + " " + indexName + ") */ touchcol FROM " - + tableName + " WHERE idxcol = '" + idxColValue + "'"; - try (ResultSet rs = conn.createStatement().executeQuery(sql)) { - if (!rs.next()) { - return null; - } - String value = rs.getString(1); - assertFalse("expected at most one row via the uncovered index", rs.next()); - return value; - } - } -} diff --git a/phoenix-core/src/it/java/org/apache/phoenix/iterate/PhoenixQueryTimeoutIT.java b/phoenix-core/src/it/java/org/apache/phoenix/iterate/PhoenixQueryTimeoutIT.java index 98724c8dc64..8dd5ea5d7f4 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/iterate/PhoenixQueryTimeoutIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/iterate/PhoenixQueryTimeoutIT.java @@ -201,8 +201,10 @@ public void testScanningResultIteratorQueryTimeoutForPagingWithVeryLowTimeout() return false; } for (HRegion region : regions) { - if (region.getCoprocessorHost().findCoprocessor( - DelayedRegionObserver.class.getName()) == null) { + if ( + region.getCoprocessorHost().findCoprocessor(DelayedRegionObserver.class.getName()) + == null + ) { return false; } }