PHOENIX-7996 Enable server-side maintenance of immutable indexes by default

phoenix-core-client/src/main/java/org/apache/phoenix/compile/DeleteCompiler.java

@@ -18,8 +18,6 @@
 package org.apache.phoenix.compile;
 
 import static org.apache.phoenix.execute.MutationState.RowTimestampColInfo.NULL_ROWTIMESTAMP_INFO;
-import static org.apache.phoenix.query.QueryServices.SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB;
-import static org.apache.phoenix.query.QueryServicesOptions.DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED;
 import static org.apache.phoenix.util.NumberUtil.add;
 
 import edu.umd.cs.findbugs.annotations.NonNull;
@@ -214,7 +212,7 @@ public byte[] getRowKey() {
           // The data table is always the last one in the list if it's
           // not chosen as the best of the possible plans.
           dataTable = otherTableRefs.get(otherTableRefs.size() - 1).getTable();
-          if (!isMaintainedOnClient(table, connection)) {
+          if (!isMaintainedOnClient(table, dataTable, connection)) {
             // dataTable is a projected table and may not include all the indexed columns and so we
             // need to get
             // the actual data table
@@ -259,7 +257,7 @@ public byte[] getRowKey() {
         // row timestamp column, then the
         // row key will already have its value.
         // Check for otherTableRefs being empty required when deleting directly from the index
-        if (otherTableRefs.isEmpty() || isMaintainedOnClient(table, connection)) {
+        if (otherTableRefs.isEmpty() || isMaintainedOnClient(table, dataTable, connection)) {
           mutations.put(rowKeyPtr,
             new RowMutationState(PRow.DELETE_MARKER, 0,
               statement.getConnection().getStatementExecutionCounter(), NULL_ROWTIMESTAMP_INFO,
@@ -385,7 +383,7 @@ private List<PTable> getClientSideMaintainedIndexes(TableRef tableRef) {
       for (PTable index : table.getIndexes()) {
         if (
           !index.getIndexState().isDisabled()
-            && isMaintainedOnClient(index, statement.getConnection())
+            && isMaintainedOnClient(index, table, statement.getConnection())
         ) {
           nonDisabledIndexes.add(index);
         }
@@ -569,7 +567,7 @@ public MutationPlan compile(DeleteStatement delete, MutationState.ReturnResult r
     // mutations are generated on the client side. Indexed columns are needed to identify index rows
     // to be deleted
     for (PTable index : table.getIndexes()) {
-      if (isMaintainedOnClient(index, connection)) {
+      if (isMaintainedOnClient(index, table, connection)) {
         IndexMaintainer maintainer = index.getIndexMaintainer(table, connection);
         // Go through maintainer as it handles functional indexes correctly
         for (Pair<String, String> columnInfo : maintainer.getIndexedColumnInfo()) {
@@ -1135,15 +1133,16 @@ public QueryPlan getQueryPlan() {
     }
   }
 
-  private static boolean isMaintainedOnClient(PTable table, PhoenixConnection connection) {
+  private static boolean isMaintainedOnClient(PTable table, PTable dataTable,
+    PhoenixConnection connection) {
     if (CDCUtil.isCDCIndex(table)) {
       return false;
     }
+    // The server-side-maintenance flag is read against the data table so a ROW_TIMESTAMP data
+    // table keeps its indexes client-maintained (see IndexUtil).
     if (
       !table.isTransactional() && table.getIndexType() != IndexType.LOCAL
-        && connection.getQueryServices().getConfiguration().getBoolean(
-          SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB,
-          DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED)
+        && IndexUtil.isServerSideImmutableIndexMaintenanceEnabled(dataTable, connection)
     ) {
       return false;
     }

phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java

@@ -1927,7 +1927,9 @@ private void filterIndexCheckerMutations(Map<TableInfo, List<Mutation>> mutation
       }
       PTable logicalTable = tableInfo.getPTable();
       if (
-        !this.serverSideImmutableIndexes && tableInfo.getOrigTableRef().getTable().isImmutableRows()
+        !IndexUtil.isServerSideImmutableIndexMaintenanceEnabled(
+          tableInfo.getOrigTableRef().getTable(), this.serverSideImmutableIndexes)
+          && tableInfo.getOrigTableRef().getTable().isImmutableRows()
           && (this.indexRegionObserverEnabledAllTables
             || IndexUtil.isGlobalIndexCheckerEnabled(connection, tableInfo.getHTableName()))
       ) {

phoenix-core-client/src/main/java/org/apache/phoenix/index/IndexMaintainer.java

@@ -17,8 +17,6 @@
  */
 package org.apache.phoenix.index;
 
-import static org.apache.phoenix.query.QueryServices.SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB;
-import static org.apache.phoenix.query.QueryServicesOptions.DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED;
 import static org.apache.phoenix.schema.PTable.QualifierEncodingScheme.NON_ENCODED_QUALIFIERS;
 
 import com.google.protobuf.InvalidProtocolBufferException;
@@ -198,11 +196,9 @@ public static Iterator<PTable> maintainedLocalOrGlobalIndexesWithoutMatchingStor
     return Iterators.filter(indexes, new Predicate<PTable>() {
       @Override
       public boolean apply(PTable index) {
-        return sendIndexMaintainer(index) && ((index.getIndexType() == IndexType.GLOBAL
+        return sendIndexMaintainer(index) && ((IndexUtil.isGlobalIndex(index)
           && (dataTable.getImmutableStorageScheme() != index.getImmutableStorageScheme()
-            || connection.getQueryServices().getConfiguration().getBoolean(
-              SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB,
-              DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED)))
+            || IndexUtil.isServerSideImmutableIndexMaintenanceEnabled(dataTable, connection)))
           || index.getIndexType() == IndexType.LOCAL || CDCUtil.isCDCIndex(index));
       }
     });

phoenix-core-client/src/main/java/org/apache/phoenix/index/IndexMetaDataCacheClient.java

@@ -40,6 +40,7 @@
 import org.apache.phoenix.schema.PTable;
 import org.apache.phoenix.schema.PTableType;
 import org.apache.phoenix.util.ByteUtil;
+import org.apache.phoenix.util.IndexUtil;
 import org.apache.phoenix.util.PhoenixRuntime;
 import org.apache.phoenix.util.ReadOnlyProps;
 import org.apache.phoenix.util.ScanUtil;
@@ -145,9 +146,11 @@ public static ServerCache setMetaDataOnMutations(PhoenixConnection connection, P
         QueryServicesOptions.DEFAULT_INDEX_USE_SERVER_METADATA)
         && props.getBoolean(QueryServices.INDEX_REGION_OBSERVER_ENABLED_ATTRIB,
           QueryServicesOptions.DEFAULT_INDEX_REGION_OBSERVER_ENABLED);
-      boolean serverSideImmutableIndexes =
-        props.getBoolean(SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB,
-          DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED);
+      // Keep ROW_TIMESTAMP tables client-maintained so the send-metadata decision matches the
+      // client-vs-server maintenance decision made for the same table elsewhere.
+      boolean serverSideImmutableIndexes = IndexUtil.isServerSideImmutableIndexMaintenanceEnabled(
+        table, props.getBoolean(SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB,
+          DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED));
       boolean useServerCacheRpc =
         useIndexMetadataCache(connection, mutations, indexMetaDataPtr.getLength() + txState.length)
           && sendIndexMaintainers;

phoenix-core-client/src/main/java/org/apache/phoenix/query/QueryServicesOptions.java

@@ -433,7 +433,7 @@ public class QueryServicesOptions {
   public static final long DEFAULT_GLOBAL_INDEX_ROW_AGE_THRESHOLD_TO_DELETE_MS =
     7 * 24 * 60 * 60 * 1000; /* 7 days */
   public static final boolean DEFAULT_INDEX_REGION_OBSERVER_ENABLED = true;
-  public static final boolean DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED = false;
+  public static final boolean DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED = true;
 
   public static final String DEFAULT_INDEX_REGION_OBSERVER_ENABLED_ALL_TABLES =
     Boolean.toString(true);

phoenix-core-client/src/main/java/org/apache/phoenix/util/IndexUtil.java

@@ -761,14 +761,35 @@ public static void updateIndexState(PhoenixConnection conn, String indexTableNam
     }
   }
 
+  /**
+   * Whether server-side maintenance of an immutable data table's global indexes applies. A table
+   * with a ROW_TIMESTAMP column stays client-maintained even when the flag is set: server-side
+   * maintenance stamps every cell with the server batch timestamp, which would overwrite the
+   * user-supplied ROW_TIMESTAMP value and silently drop rows on ROW_TIMESTAMP range scans.
+   */
+  public static boolean isServerSideImmutableIndexMaintenanceEnabled(PTable dataTable,
+    boolean configured) {
+    return configured && dataTable.getRowTimestampColPos() == -1;
+  }
+
+  public static boolean isServerSideImmutableIndexMaintenanceEnabled(PTable dataTable,
+    PhoenixConnection connection) {
+    return isServerSideImmutableIndexMaintenanceEnabled(dataTable,
+      connection.getQueryServices().getConfiguration().getBoolean(
+        QueryServices.SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB,
+        QueryServicesOptions.DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED));
+  }
+
   public static List<PTable> getClientMaintainedIndexes(PTable table,
     boolean serverSideImmutableIndex) {
     Iterator<PTable> indexIterator = // Only maintain tables with immutable rows through this
                                      // client-side mechanism
       (table.isTransactional() && table.getTransactionProvider().getTransactionProvider()
         .isUnsupported(Feature.MAINTAIN_LOCAL_INDEX_ON_SERVER))
         ? IndexMaintainer.maintainedIndexes(table.getIndexes().iterator())
-        : (table.isImmutableRows() && !serverSideImmutableIndex || table.isTransactional())
+        : (table.isImmutableRows()
+          && !isServerSideImmutableIndexMaintenanceEnabled(table, serverSideImmutableIndex)
+          || table.isTransactional())
           // If the data table has a different storage scheme than index table, don't maintain
           // this on the client. For example, if the index is single cell but the data table is
           // one_cell, and there is a partial update on the data table, index can't be built

phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java

@@ -1189,6 +1189,44 @@ private boolean isPartialUncoveredIndexMutation(PhoenixIndexMetaData indexMetaDa
     return false;
   }
 
+  /**
+   * Determines whether any data table mutation in the batch omits an on-disk column that a global
+   * index materializes (an indexed, covered, or index-WHERE column). For an immutable table the
+   * region-side build skips the current-row read-back, so such a partial upsert would drop or
+   * misbuild that column in the index while it survives in the data table. Columns are resolved to
+   * their on-disk qualifiers per the data table storage scheme, so a single-cell table (whose
+   * upserts rewrite the whole cell) is never treated as partial. Both covered and uncovered global
+   * maintainers are considered; transform and local index maintainers are skipped.
+   */
+  private boolean isPartialGlobalIndexMutation(PhoenixIndexMetaData indexMetaData,
+    MiniBatchOperationInProgress<Mutation> miniBatchOp) {
+    Set<ColumnReference> columns = new HashSet<ColumnReference>();
+    for (IndexMaintainer indexMaintainer : indexMetaData.getIndexMaintainers()) {
+      if (indexMaintainer instanceof TransformMaintainer || indexMaintainer.isLocalIndex()) {
+        continue;
+      }
+      columns.addAll(indexMaintainer.getAllColumnsForDataTable());
+    }
+    if (columns.isEmpty()) {
+      return false;
+    }
+    for (int i = 0; i < miniBatchOp.size(); i++) {
+      if (isAtomicOperationComplete(miniBatchOp.getOperationStatus(i))) {
+        continue;
+      }
+      Mutation m = miniBatchOp.getOperation(i);
+      if (!this.builder.isEnabled(m)) {
+        continue;
+      }
+      for (ColumnReference column : columns) {
+        if (m.get(column.getFamily(), column.getQualifier()).isEmpty()) {
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+
   /**
    * Retrieve the data row state either from memory or disk. The rows are locked by the caller.
    * <p>
@@ -1985,6 +2023,8 @@ public void preBatchMutateWithExceptions(ObserverContext<RegionCoprocessorEnviro
           || context.hasStrictConditionalTTL()
           || !context.immutableRows && context.hasUncoveredIndex
             && isPartialUncoveredIndexMutation(indexMetaData, miniBatchOp)
+          || context.immutableRows && (context.hasGlobalIndex || context.hasUncoveredIndex)
+            && isPartialGlobalIndexMutation(indexMetaData, miniBatchOp)
       ) {
         getCurrentRowStates(c, context, batchTimestamp);
       }

phoenix-core/src/it/java/org/apache/phoenix/end2end/DeleteIT.java

@@ -60,6 +60,7 @@
 import org.apache.phoenix.parse.SQLParser;
 import org.apache.phoenix.query.ConnectionQueryServices;
 import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesOptions;
 import org.apache.phoenix.util.PropertiesUtil;
 import org.apache.phoenix.util.QueryUtil;
 import org.junit.Test;
@@ -505,7 +506,14 @@ public void testPointDeleteRowFromTableWithImmutableIndex(boolean localIndex,
       psDelete.setString(3, "CC");
       psDelete.setDate(4, date);
       String explainPlan = QueryUtil.getExplainPlan(psDelete.executeQuery());
-      if (addNonPKIndex) {
+      // A non-PK index needs the row's non-PK values to maintain its entries. When immutable
+      // indexes are maintained client side the client must read the row first, so the delete is
+      // not a single-row plan. When they are maintained server side the region server reads and
+      // maintains the index, so the point delete stays a single-row plan.
+      boolean serverSideImmutableIndexes = con.unwrap(PhoenixConnection.class).getQueryServices()
+        .getConfiguration().getBoolean(QueryServices.SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB,
+          QueryServicesOptions.DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED);
+      if (addNonPKIndex && !serverSideImmutableIndexes) {
         assertNotEquals("DELETE SINGLE ROW", explainPlan);
       } else {
         assertEquals("DELETE SINGLE ROW", explainPlan);

phoenix-core/src/it/java/org/apache/phoenix/end2end/IndexToolIT.java

@@ -888,8 +888,13 @@ private void testIndexToDataVerificationHelper(boolean caseSensitive) throws Exc
         0, IndexTool.IndexVerifyType.ONLY, "-fi");
 
       CounterGroup mrJobCounters = getMRJobCounters(indexTool);
-      // Extra index rows should be detected
-      if (mutable) {
+      boolean serverSideImmutableIndexes = conn.unwrap(PhoenixConnection.class).getQueryServices()
+        .getConfiguration().getBoolean(QueryServices.SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB,
+          QueryServicesOptions.DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED);
+      // Extra index rows should be detected. Immutable indexes maintained server-side adopt the
+      // same two-phase verified write protocol as mutable indexes, so the directly-written extra
+      // rows are counted as UNVERIFIED rather than VERIFIED.
+      if (mutable || serverSideImmutableIndexes) {
         assertEquals(3, mrJobCounters
           .findCounter(BEFORE_REPAIR_EXTRA_UNVERIFIED_INDEX_ROW_COUNT.name()).getValue());
       } else {

phoenix-core/src/it/java/org/apache/phoenix/end2end/index/BaseIndexIT.java

@@ -69,6 +69,7 @@
 import org.apache.phoenix.query.BaseTest;
 import org.apache.phoenix.query.QueryConstants;
 import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesOptions;
 import org.apache.phoenix.schema.PIndexState;
 import org.apache.phoenix.schema.PTable;
 import org.apache.phoenix.schema.PTableImpl;
@@ -271,15 +272,21 @@ private void assertNoClientSideIndexMutations(Connection conn) throws SQLExcepti
     Iterator<Pair<byte[], List<Cell>>> iterator = PhoenixRuntime.getUncommittedDataIterator(conn);
     if (iterator.hasNext()) {
       byte[] tableName = iterator.next().getFirst(); // skip data table mutations
-      PTable table = conn.unwrap(PhoenixConnection.class).getTable(Bytes.toString(tableName));
-      boolean clientSideUpdate =
-        (!localIndex || (transactional && table.getTransactionProvider().getTransactionProvider()
-          .isUnsupported(Feature.MAINTAIN_LOCAL_INDEX_ON_SERVER))) && (!mutable || transactional);
+      PhoenixConnection pconn = conn.unwrap(PhoenixConnection.class);
+      PTable table = pconn.getTable(Bytes.toString(tableName));
+      // Immutable indexes are maintained server side unless client-side maintenance is enabled.
+      boolean serverSideImmutableIndexes = pconn.getQueryServices().getConfiguration().getBoolean(
+        QueryServices.SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB,
+        QueryServicesOptions.DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED);
+      boolean clientSideUpdate = (!localIndex || (transactional && table.getTransactionProvider()
+        .getTransactionProvider().isUnsupported(Feature.MAINTAIN_LOCAL_INDEX_ON_SERVER)))
+        && ((!mutable && !serverSideImmutableIndexes) || transactional);
       if (!clientSideUpdate) {
         assertTrue(table.getType() == PTableType.TABLE); // should be data table
       }
       boolean hasIndexData = iterator.hasNext();
-      // global immutable and global transactional tables are processed client side
+      // global transactional (and immutable when maintained client side) tables are processed
+      // client side
       assertEquals(clientSideUpdate, hasIndexData);
     }
   }

phoenix-core/src/it/java/org/apache/phoenix/end2end/index/BaseIndexWithRegionMovesIT.java

@@ -69,6 +69,7 @@
 import org.apache.phoenix.query.BaseTest;
 import org.apache.phoenix.query.QueryConstants;
 import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesOptions;
 import org.apache.phoenix.schema.PIndexState;
 import org.apache.phoenix.schema.PTable;
 import org.apache.phoenix.schema.PTableImpl;
@@ -300,14 +301,19 @@ private void assertNoClientSideIndexMutations(Connection conn) throws SQLExcepti
     if (iterator.hasNext()) {
       byte[] tableName = iterator.next().getFirst(); // skip data table mutations
       PTable table = PhoenixRuntime.getTable(conn, Bytes.toString(tableName));
-      boolean clientSideUpdate =
-        (!localIndex || (transactional && table.getTransactionProvider().getTransactionProvider()
-          .isUnsupported(Feature.MAINTAIN_LOCAL_INDEX_ON_SERVER))) && (!mutable || transactional);
+      // Immutable indexes are maintained server side unless client-side maintenance is enabled.
+      boolean serverSideImmutableIndexes = conn.unwrap(PhoenixConnection.class).getQueryServices()
+        .getConfiguration().getBoolean(QueryServices.SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED_ATTRIB,
+          QueryServicesOptions.DEFAULT_SERVER_SIDE_IMMUTABLE_INDEXES_ENABLED);
+      boolean clientSideUpdate = (!localIndex || (transactional && table.getTransactionProvider()
+        .getTransactionProvider().isUnsupported(Feature.MAINTAIN_LOCAL_INDEX_ON_SERVER)))
+        && ((!mutable && !serverSideImmutableIndexes) || transactional);
       if (!clientSideUpdate) {
         assertTrue(table.getType() == PTableType.TABLE); // should be data table
       }
       boolean hasIndexData = iterator.hasNext();
-      // global immutable and global transactional tables are processed client side
+      // global transactional (and immutable when maintained client side) tables are processed
+      // client side
       assertEquals(clientSideUpdate, hasIndexData);
     }
   }