PHOENIX-7996 Correctly maintain immutable global indexes on the server side for partial upserts

[lokiore]

What changes were proposed in this pull request?

This hardens the server-side maintenance path for immutable, global, non-transactional secondary indexes and fixes two correctness gaps that server-side maintenance exposes. The client default DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED stays false — this change is a pure correctness fix for the opt-in path (phoenix.server.side.immutable.indexes.enabled=true). Flipping the default on is deferred to a separate follow-up so it can be adopted independently once the path is proven.

When the flag is enabled, immutable global indexes are maintained server-side by IndexRegionObserver (added in PHOENIX-7426) rather than by the client: the client ships the serialized IndexMaintainer and the region server builds the index updates exactly once.

To keep the opt-in path safe, immutable data tables that declare a ROW_TIMESTAMP column continue to be maintained client-side regardless of the flag. The decision is centralized in a new helper, IndexUtil.isServerSideImmutableIndexMaintenanceEnabled(...), and applied at every data-table gate that reads the flag:

• IndexUtil.getClientMaintainedIndexes
• IndexMaintainer.maintainedLocalOrGlobalIndexesWithoutMatchingStorageScheme (the INDEX_UUID gate)
• MutationState.filterIndexCheckerMutations
• DeleteCompiler.isMaintainedOnClient (signature extended to take the data table so the guard resolves ROW_TIMESTAMP against the data table, not a projected or index table)
• IndexMetaDataCacheClient.setMetaDataOnMutations (the send-metadata gate)
• UpsertCompiler's flag read

Routing all gates through the same helper keeps the client and server in agreement on which side maintains a given table; a disagreement would cause a ROW_TIMESTAMP table to be maintained on both sides.

The two correctness fixes, both required before server-side maintenance can be relied on:

• Partial upserts on immutable global indexes could lose or misbuild an index column. IndexRegionObserver skips the current-row read-back for immutable batches. A partial upsert that omits an indexed, covered, or index-WHERE column then builds the index entry from the partial mutation alone. For a covered index this drops the column, so it is lost from the index while it survives in the data table (silent divergence, the same class of hazard as PHOENIX-7961). For an uncovered index the omitted indexed column is materialized as null, so a spurious null-keyed index entry is written while the data row keeps the earlier value: a point lookup self-heals through the index read-repair join-back, but a server-side aggregate over the index rows returns the wrong result. The read-back gate now forces a read-back for immutable batches carrying a covered or uncovered global index when any enabled mutation omits one of that index's on-disk columns. Columns are resolved to their on-disk qualifiers per the data table storage scheme, so full-row upserts and single-cell tables keep the fast path with no read-back. The gate trades a bounded in-memory column scan (only on the immutable global-index write path) for avoiding a disk read-back on the common full-row-upsert case.
• Uncovered global immutable indexes with a matching storage scheme were maintained by nobody. The immutable server-serialize filter in IndexMaintainer matched IndexType.GLOBAL only. An uncovered global immutable index whose storage scheme matched the data table was therefore serialized by neither the client (which returns no client-maintained indexes for these tables when the flag is on) nor the server (whose filter dropped it), so it went unmaintained. The filter now matches IndexUtil.isGlobalIndex, which covers both GLOBAL and UNCOVERED_GLOBAL.

Why are the changes needed?

PHOENIX-7426 added server-side maintenance of immutable-table indexes behind a flag that defaults to off. Before that path can be enabled anywhere, two correctness gaps it exposes must be closed and the maintenance-side decision must be centralized so the client and server never disagree. This PR does both without changing the product default:

• Without the read-back, a partial immutable upsert would serve stale/missing covered values or a wrong-keyed uncovered entry from the index once the flag is on.
• Without the broadened serialize filter, an uncovered global immutable index would receive no updates at all once the flag is on.
• The ROW_TIMESTAMP carve-out is required for correctness whenever the flag is on. Server-side maintenance re-stamps every data cell — including the ROW_TIMESTAMP column — with the server batch timestamp, overwriting the user-supplied ROW_TIMESTAMP value. ROW_TIMESTAMP range predicates push an HBase scan TimeRange, so re-stamped cells fall outside it and rows are silently dropped on range reads (SCN-based visibility breaks for the same reason). This is the same hazard behind CANNOT_CREATE_INDEX_ON_MUTABLE_TABLE_WITH_ROWTIMESTAMP, which already forbids the mutable variant; the immutable variant is safe only because it stays client-maintained.

Does this PR introduce any user-facing change?

No default behavior change: phoenix.server.side.immutable.indexes.enabled stays false, so immutable global indexes remain client-maintained by default.

When the flag is explicitly enabled:

• Immutable global indexes are now maintained correctly on partial upserts, and uncovered global immutable indexes (matching storage scheme) are maintained at all.
• Immutable tables with a ROW_TIMESTAMP column stay client-maintained.
• Upgrade region servers before clients. Server-side maintenance rides the generic IndexRegionObserver path, which is enabled by default; on a default cluster a new client talking to an already-upgraded (or default-configured) server loses no index data. Silent index-data loss is only reachable on the deprecated legacy-Indexer configuration (phoenix.index.region.observer.enabled=false); clusters still on that configuration must move off it before enabling this flag.

How was this patch tested?

• RowTimestampIT — regression lock; asserts raw-scan cell timestamps equal the user ROW_TIMESTAMP on both data and index tables for the immutable case. Passes with the guard.
• GlobalIndexCheckerIT#testPartialRowUpdateForImmutable — regression lock for the read-back fix on a ONE_CELL_PER_COLUMN immutable covered index; a full upsert is followed by a partial upsert that omits a covered column, and the test asserts the index read still returns the earlier value. Fails without the read-back fix (expected:<abcd> but was:<null>) and passes with it.
• GlobalIndexCheckerIT#testPartialRowUpdateForImmutableUncovered — regression lock for the read-back fix on a ONE_CELL_PER_COLUMN immutable uncovered index; a full upsert is followed by a partial upsert that omits the indexed column, and the test asserts (via the index path) that the row still resolves under its original key and that IS NULL returns nothing under both maintenance modes. The index-COUNT invariant is asserted only when server-side maintenance is enabled (read from the effective flag), because the scan path self-heals unverified index rows at read time but the aggregate path agrees only when the server rebuilds a verified entry from the read-back row — mirroring BaseImmutableIndexIT.
• UncoveredGlobalImmutableNonTxIndexIT and UncoveredGlobalImmutableNonTxIndex2IT — pass with the broadened serialize filter (uncovered global immutable indexes are now maintained under the flag). GlobalImmutableNonTxIndexIT (covered global) stays green, confirming no regression to the covered path.
• ServerSideImmutableIndexIT and ClientSideImmutableIndexIT — pass. These extend BaseImmutableIndexIT, which gained partial-upsert/delete coverage that runs under both drivers (flag on and off) and is parameterized over columnEncoded, so each case executes against both ONE_CELL_PER_COLUMN and SINGLE_CELL_ARRAY_WITH_OFFSETS: partial upsert over a covered index, over an uncovered index (COUNT path, which does not self-heal), across multiple global indexes, across multiple column families, with an index WHERE clause, a delete over an immutable ROW_TIMESTAMP table with an index (the branch the DeleteCompiler signature change was added for), and a mixed matching/mismatched storage-scheme index case.
• PhoenixMetricsIT, PhoenixLoggingMetricsIT, and PhoenixTableLevelMetricsIT#testMetricsWithIndexUsage — pass; the flag is pinned off at the driver level for the assertions that account for client-side index mutations.
• IndexToolIT#testIndexToDataVerification* — when the flag is on, the immutable case expects the extra directly-written index rows to be counted as EXTRA_UNVERIFIED rather than EXTRA_VERIFIED, because server-side maintenance applies the same two-phase verified write protocol as mutable indexes; the assertion reads the flag so it stays correct under either default. Passes across all parameter combinations.
• PhoenixClientRpcIT#testIndexQos — pins the flag off so it keeps exercising the client-maintained path it was written to verify (that immutable-index writes do not use the region-server index RPC queue).

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8)

[Copilot]

🟡 Changes recommended

Differing storage schemes bypass the ROW_TIMESTAMP safeguard and can still trigger unsafe server-side maintenance.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Enables server-side maintenance for immutable indexes by default while preserving client maintenance for ROW_TIMESTAMP tables and correcting partial-upsert handling.

Changes:

• Centralizes immutable-index routing and flips the default.
• Adds server read-back for partial immutable global-index mutations.
• Updates integration tests and metrics expectations.

File summaries

File	Description
PhoenixTableLevelMetricsIT.java	Preserves client-side mutation metrics.
BasePhoenixMetricsIT.java	Pins client-side maintenance for metrics tests.
GlobalIndexCheckerIT.java	Tests partial uncovered-index updates.
BaseIndexWithRegionMovesIT.java	Updates mutation-routing assertions.
BaseIndexIT.java	Updates mutation-routing assertions.
DeleteIT.java	Adjusts delete-plan expectations.
IndexRegionObserver.java	Detects partial global-index mutations.
IndexUtil.java	Adds centralized ROW_TIMESTAMP guard.
QueryServicesOptions.java	Enables server maintenance by default.
IndexMetaDataCacheClient.java	Applies guarded metadata routing.
IndexMaintainer.java	Broadens server serialization to uncovered indexes.
MutationState.java	Applies guarded index-checker filtering.
DeleteCompiler.java	Routes delete maintenance using the data table.

Review details

• Files reviewed: 13/13 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[lokiore]

Confirmed — the analysis is correct. The storage-scheme-mismatch term is the left operand of the inner ||, so for a mismatched-scheme index the predicate short-circuits to true there and the ROW_TIMESTAMP carve-out on the right operand (isServerSideImmutableIndexMaintenanceEnabled) is never consulted. There is also no client-side rescue: getClientMaintainedIndexes routes an immutable ROW_TIMESTAMP table through maintainedGlobalIndexesWithMatchingStorageScheme, which excludes a cross-scheme index, so it cannot safely be client-maintained without a deeper change.

For a covered global index this routing is unchanged by this PR — the mismatch term is pre-existing (relative to the merge-base, the only edits to that operand are IndexType.GLOBAL -> IndexUtil.isGlobalIndex and the direct flag read -> the helper). So a covered mismatched-scheme index on a ROW_TIMESTAMP immutable table was already server-maintained and re-stamped before this flip. The isGlobalIndex broadening does newly bring UNCOVERED_GLOBAL mismatched-scheme indexes onto the server path, extending the same corner to the uncovered variant.

The correct fix is a server-side ROW_TIMESTAMP exemption in the re-stamp path (setTimestamps) rather than routing changes; it is orthogonal to this default flip and best handled separately.

[tkhurana]

Do you know why this check is now needed and how was the client handling partial updates ?

[lokiore]

Before this change, immutable indexes were maintained entirely on the client: each index mutation was built from only the columns present in the current upsert, with no read-back. So a partial upsert produced an index row from just those columns, and correctness was recovered lazily at read time — GlobalIndexChecker's two-phase verified/unverified protocol rebuilds the index row from the full data-table row on read. Uncovered indexes weren't maintained at write time at all.

Now that the server maintains immutable indexes at write time, a partial mutation no longer carries every column the index needs, so we have to read the current row state to build a correct entry. This check gates that read-back to the immutable global/uncovered case on partial mutations (isPartialGlobalIndexMutation), matching what the mutable path already does; full upserts (all index columns present) still skip it.

[tkhurana]

UpsertCompiler.java:629-633 (pre-existing) reads the flag directly, not through the new isServerSideImmutableIndexMaintenanceEnabled helper:

[tkhurana]

Some test gaps identified

• (A) SINGLE_CELL storage-scheme partial upsert is entirely untested. Both new GlobalIndexCheckerIT tests early-return on encoded and hardcode ONE_CELL_PER_COLUMN — and encoded=true (→ SINGLE_CELL_ARRAY_WITH_OFFSETS) is exactly the matrix combination they skip. The single-cell branch of getAllColumnsForDataTable/addColumnRefForScan is the PR's own load-bearing fast-path claim and has zero assertion behind it. This is the one gap I'd most want closed (matching data+index single-cell scheme, full then partial upsert, assert index/data agreement).
• (B) Index-WHERE (partial index) columns — getAllColumnsForDataTable deliberately includes them, but no test creates CREATE INDEX … WHERE … on an immutable table and omits the WHERE column.
• (C) Delete over an immutable ROW_TIMESTAMP table with an index — the exact branch the DeleteCompiler signature change was added for is unexercised (the one DeleteIT change is about single-row plans, not ROW_TIMESTAMP).
• (D) Multiple global indexes on one table — the cross-maintainer column union in isPartialGlobalIndexMutation is untested.

[lokiore]

@tkhurana added coverage in BaseImmutableIndexIT (2ff28fd). It runs under both ServerSideImmutableIndexIT (flag on) and ClientSideImmutableIndexIT (flag off) and is parameterized over columnEncoded, so each test executes against both ONE_CELL_PER_COLUMN and SINGLE_CELL_ARRAY_WITH_OFFSETS — the matrix combination the GlobalIndexCheckerIT tests skipped.

• (A) SINGLE_CELL partial upsert: testPartialUpsertForImmutableCoveredIndex and testPartialUpsertForImmutableUncoveredIndex run under columnEncoded=true (-> SINGLE_CELL_ARRAY_WITH_OFFSETS) with a matching-scheme index — full-then-partial upsert asserting index/data agreement (and, for the uncovered case, a COUNT over the index, which does not self-heal). The single-cell getAllColumnsForDataTable/addColumnRefForScan fast path is now exercised on both maintenance sides.
• (B) Index-WHERE: testPartialUpsertForImmutablePartialIndex creates CREATE INDEX … WHERE … on an immutable table and omits the WHERE column.
• (C) Delete over immutable ROW_TIMESTAMP + index: testDeleteFromImmutableRowTimestampTableWithIndex exercises the exact DeleteCompiler signature-change branch, asserting the surviving ROW_TIMESTAMP rows remain reachable in their bands.
• (D) Multiple global indexes: testPartialUpsertForImmutableMultipleIndexes covers the cross-maintainer column union in isPartialGlobalIndexMutation.

Also added testPartialUpsertForImmutableMultipleColumnFamilies and testPartialUpsertForImmutableMixedStorageSchemeIndexes (a matching + a mismatched-scheme index on the same table, with a non-self-healing COUNT served from the mismatched-scheme index).

[tkhurana]

What kind of test coverage will we have if we keep this false by default ? Do we need to explicitly enable this in few ITs ?

[lokiore]

most of the tests added are in BaseImmutableIndexIT which is being extended by ServerSideImmutableIndexIT and ClientSideImmutableIndexIT (irrespective of default) we are testing both for these ITs atleast. Other general index ITs work on default value.

I'll update the PR to make it false by default and update the PR and JIRA accordingly. Thanks for approval