PHOENIX-7905: address review feedback (alterIndex correctness, addColumn freshTable, tenantId-null lock contract, default interface methods, ops log demote, new tests)

- alterIndex: hoist transform-lock acquire + under-lock re-check to BEFORE the
  index-state UPSERT (was AFTER), so a contending caller cannot leave a
  half-committed BUILDING state behind on lock-acquire failure. Thread the
  post-re-check freshTable into evaluateStmtProperties, incrementTableSeqNum,
  updateIndexState, and addTransform.
- addColumn: hoist freshTable to acquiredTransformLock scope and thread it into
  incrementTableSeqNum + addTransform so transform-decision sites see the
  post-re-check view rather than the stale snapshot (parallel to alterIndex
  fix).
- ConnectionQueryServices: convert acquireTransformLock / releaseTransformLock
  to default methods so older binary-compatible delegates keep compiling;
  Javadoc clarifies tenantId is accepted for API symmetry but is NOT part of
  the lock rowkey, and that callers MUST release in a finally block.
- ConnectionQueryServicesImpl: pass null for tenantId when constructing the
  SYSTEM.MUTEX rowkey to make tenant-scope independence explicit; mark
  TRANSFORM_LOCK_MARKER @VisibleForTesting; demote contended-mutex log line
  from ERROR to INFO (lock contention is expected steady-state, not an
  operator-actionable event).
- TransformLockIT: rename testReleaseIsIdempotent to testReleaseDoesNotThrow
  to match the actual contract; assert schema/table render in the
  transform-in-progress exception message; add testGlobalTransformBlocksTenantTransform,
  testTenantBlocksGlobalAndOtherTenants, testNonTransformAlterIndexDoesNotAcquireLock,
  testConcurrentAlterIndexSerializesViaTransformLock.

Generated-by: Claude Code (Opus 4.7)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: lokiore

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

@@ -303,24 +303,37 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
    * physical table) with concurrent transform lifecycle operations. Non-transform-triggering ALTERs
    * (e.g., SET TTL, ADD COLUMN, SET IMMUTABLE_ROWS) do not contend on this lock.
    * <p>
+   * Lock scope: keyed on {@code (schemaName, tableName)} only — the {@code tenantId} arg is
+   * accepted for API symmetry but does NOT participate in the lock rowkey. A transform-triggering
+   * change on a (schema, table) blocks any new transform attempt on the same table regardless of
+   * tenant, and vice versa.
+   * <p>
    * Implemented on top of SYSTEM.MUTEX, so the lock auto-expires after the column-family TTL
    * ({@link org.apache.phoenix.jdbc.PhoenixDatabaseMetaData#TTL_FOR_MUTEX} = 15 min) if the holder
    * dies without releasing.
+   * <p>
+   * Callers MUST invoke {@link #releaseTransformLock} from a {@code finally} block on every path
+   * where this method returned {@code true}.
    * @return true if the caller acquired the lock; false if it is currently held by another caller
    *         <p>
    *         Caller is responsible for completing the operation within the column-family TTL; this
    *         is a coarse advisory lock with no fencing token. Holders that pause past the TTL may
    *         both observe lock-acquired simultaneously.
    */
-  boolean acquireTransformLock(String tenantId, String schemaName, String tableName)
-    throws SQLException;
+  default boolean acquireTransformLock(String tenantId, String schemaName, String tableName)
+    throws SQLException {
+    return true;
+  }
 
   /**
-   * Release the transform lock on the given logical table. Idempotent — no-op if the lock is not
-   * held (already released, expired via TTL, or never acquired by this caller).
+   * Release the transform lock on the given logical table. Caller MUST call this from a
+   * {@code finally} block whenever {@link #acquireTransformLock} returned {@code true}. The release
+   * deletes the lock cell unconditionally; callers MUST NOT call release after the column-family
+   * TTL expiry, otherwise a different caller's lock cell may be deleted.
    */
-  void releaseTransformLock(String tenantId, String schemaName, String tableName)
-    throws SQLException;
+  default void releaseTransformLock(String tenantId, String schemaName, String tableName)
+    throws SQLException {
+  }
 
   /**
    * Truncate a phoenix table

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

@@ -423,7 +423,8 @@ public ConnectionInfo getConnectionInfo() {
   private final boolean shouldThrottleNumConnections;
   public static final byte[] MUTEX_LOCKED = "MUTEX_LOCKED".getBytes(StandardCharsets.UTF_8);
   // Disambiguate the transform-lock row in SYSTEM.MUTEX from other lock purposes
-  public static final String TRANSFORM_LOCK_MARKER = "TRANSFORM_LOCK";
+  @VisibleForTesting
+  static final String TRANSFORM_LOCK_MARKER = "TRANSFORM_LOCK";
   private ServerSideRPCControllerFactory serverSideRPCControllerFactory;
   private boolean localIndexUpgradeRequired;
 
@@ -5614,7 +5615,7 @@ public boolean writeMutexCell(String tenantId, String schemaName, String tableNa
         String msg = " tenantId : " + tenantId + " schemaName : " + schemaName + " tableName : "
           + tableName + " columnName : " + columnName + " familyName : " + familyName;
         if (!checkAndPut) {
-          LOGGER.error(processName + " failed to acquire mutex for " + msg);
+          LOGGER.info(processName + " mutex acquire contended for " + msg);
         } else {
           LOGGER.debug(processName + " acquired mutex for " + msg);
         }
@@ -5659,13 +5660,16 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
   @Override
   public boolean acquireTransformLock(String tenantId, String schemaName, String tableName)
     throws SQLException {
-    return writeMutexCell(tenantId, schemaName, tableName, TRANSFORM_LOCK_MARKER, null);
+    // Lock is keyed on (schemaName, tableName) only — tenantId is intentionally not part of the
+    // SYSTEM.MUTEX rowkey so that a transform on a (schema, table) blocks any concurrent
+    // transform on the same table regardless of tenant scope.
+    return writeMutexCell(null, schemaName, tableName, TRANSFORM_LOCK_MARKER, null);
   }
 
   @Override
   public void releaseTransformLock(String tenantId, String schemaName, String tableName)
     throws SQLException {
-    deleteMutexCell(tenantId, schemaName, tableName, TRANSFORM_LOCK_MARKER, null);
+    deleteMutexCell(null, schemaName, tableName, TRANSFORM_LOCK_MARKER, null);
   }
 
   @VisibleForTesting

phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java

@@ -4818,6 +4818,10 @@ public MutationState addColumn(PTable table, List<ColumnDef> origColumnDefs,
     Set<String> acquiredColumnMutexSet = Sets.newHashSetWithExpectedSize(3);
     boolean acquiredBaseTableMutex = false;
     boolean acquiredTransformLock = false;
+    // Tracks the post-re-check fresh table view; used as the source-of-truth for transform-decision
+    // sites (incrementTableSeqNum + addTransform). Defaults to {@code table} when no transform is
+    // needed or when the lock was already held by a parent frame (no second re-check happens).
+    PTable freshTable = table;
     try {
       connection.setAutoCommit(false);
       List<ColumnDef> columnDefs;
@@ -4959,8 +4963,9 @@ public MutationState addColumn(PTable table, List<ColumnDef> origColumnDefs,
             // Re-check under lock with fresh metadata: a concurrent transform-triggering ALTER
             // may have committed between our first checkIsTransformNeeded and the lock acquire.
             // If so, the transform we observed is stale — drop through and treat as no-op.
-            PTable freshTable =
-              connection.getTableNoCache(SchemaUtil.getTableName(schemaName, tableName));
+            // Capture freshTable so downstream transform-decision sites (incrementTableSeqNum,
+            // addTransform) see the post-re-check view rather than the stale {@code table}.
+            freshTable = connection.getTableNoCache(SchemaUtil.getTableName(schemaName, tableName));
             isTransformNeeded = TransformClient.checkIsTransformNeeded(metaProperties, schemaName,
               freshTable, tableName, null, tenantIdToUse, connection);
           }
@@ -5201,7 +5206,7 @@ public MutationState addColumn(PTable table, List<ColumnDef> origColumnDefs,
         long seqNum = 0;
         if (changingPhoenixTableProperty || columnDefs.size() > 0) {
           seqNum =
-            incrementTableSeqNum(table, tableType, columnDefs.size(), metaPropertiesEvaluated);
+            incrementTableSeqNum(freshTable, tableType, columnDefs.size(), metaPropertiesEvaluated);
 
           tableMetaData
             .addAll(connection.getMutationState().toMutations(timeStamp).next().getSecond());
@@ -5211,8 +5216,8 @@ public MutationState addColumn(PTable table, List<ColumnDef> origColumnDefs,
         PTable transformingNewTable = null;
         if (isTransformNeeded) {
           try {
-            transformingNewTable = TransformClient.addTransform(connection, tenantIdToUse, table,
-              metaProperties, seqNum, PTable.TransformType.METADATA_TRANSFORM);
+            transformingNewTable = TransformClient.addTransform(connection, tenantIdToUse,
+              freshTable, metaProperties, seqNum, PTable.TransformType.METADATA_TRANSFORM);
           } catch (SQLException ex) {
             connection.rollback();
             throw ex;
@@ -6001,9 +6006,6 @@ public MutationState alterIndex(AlterIndexStatement statement) throws SQLExcepti
 
       boolean isTransformNeeded = TransformClient.checkIsTransformNeeded(metaProperties, schemaName,
         table, indexName, dataTableName, tenantId, connection);
-      MetaPropertiesEvaluated metaPropertiesEvaluated = new MetaPropertiesEvaluated();
-      boolean changingPhoenixTableProperty = evaluateStmtProperties(metaProperties,
-        metaPropertiesEvaluated, table, schemaName, tableName, new MutableBoolean(false));
 
       PIndexState newIndexState = statement.getIndexState();
       IndexConsistency newIndexConsistency = statement.getIndexConsistency();
@@ -6050,6 +6052,37 @@ public MutationState alterIndex(AlterIndexStatement statement) throws SQLExcepti
       connection.setAutoCommit(false);
       // Confirm index table is valid and up-to-date
       TableRef indexRef = FromCompiler.getResolver(statement, connection).getTables().get(0);
+
+      // If the statement is a transform-triggering ALTER INDEX, acquire the transform lock and
+      // re-check freshness under the lock BEFORE we commit any index-state UPSERT. Otherwise a
+      // contending caller would see (or leave behind) a half-committed BUILDING state if the
+      // lock acquire failed below.
+      PTable freshTable = table;
+      boolean stillTransformNeeded = false;
+      if (isTransformNeeded) {
+        if (indexRef.getTable().getViewIndexId() != null) {
+          throw new SQLExceptionInfo.Builder(SQLExceptionCode.CANNOT_TRANSFORM_LOCAL_OR_VIEW_INDEX)
+            .setSchemaName(schemaName).setTableName(indexName).build().buildException();
+        }
+        acquiredTransformLock =
+          connection.getQueryServices().acquireTransformLock(tenantId, schemaName, tableName);
+        if (!acquiredTransformLock) {
+          throw new SQLExceptionInfo.Builder(
+            SQLExceptionCode.CANNOT_MODIFY_TABLE_WITH_TRANSFORM_IN_PROGRESS)
+              .setSchemaName(schemaName).setTableName(tableName).build().buildException();
+        }
+        // Re-check under lock with fresh metadata: a concurrent transform-triggering ALTER may
+        // have committed between our first checkIsTransformNeeded and the lock acquire. If the
+        // observed transform is no longer needed, drop through and treat as no-op.
+        freshTable = connection.getTableNoCache(SchemaUtil.getTableName(schemaName, tableName));
+        stillTransformNeeded = TransformClient.checkIsTransformNeeded(metaProperties, schemaName,
+          freshTable, indexName, dataTableName, tenantId, connection);
+      }
+
+      MetaPropertiesEvaluated metaPropertiesEvaluated = new MetaPropertiesEvaluated();
+      boolean changingPhoenixTableProperty = evaluateStmtProperties(metaProperties,
+        metaPropertiesEvaluated, freshTable, schemaName, tableName, new MutableBoolean(false));
+
       try (PreparedStatement tableUpsert = connection.prepareStatement(
         newIndexState == PIndexState.ACTIVE ? UPDATE_INDEX_STATE_TO_ACTIVE : UPDATE_INDEX_STATE)) {
         tableUpsert.setString(1,
@@ -6069,14 +6102,15 @@ public MutationState alterIndex(AlterIndexStatement statement) throws SQLExcepti
       connection.rollback();
 
       if (changingPhoenixTableProperty) {
-        seqNum = incrementTableSeqNum(table, statement.getTableType(), 0, metaPropertiesEvaluated);
+        seqNum =
+          incrementTableSeqNum(freshTable, statement.getTableType(), 0, metaPropertiesEvaluated);
         tableMetadata
           .addAll(connection.getMutationState().toMutations(timeStamp).next().getSecond());
         connection.rollback();
       }
 
       MetaDataMutationResult result = connection.getQueryServices().updateIndexState(tableMetadata,
-        dataTableName, properties, table);
+        dataTableName, properties, freshTable);
 
       try {
         MutationCode code = result.getMutationCode();
@@ -6090,34 +6124,13 @@ public MutationState alterIndex(AlterIndexStatement statement) throws SQLExcepti
             .setSchemaName(schemaName).setTableName(indexName).build().buildException();
         }
 
-        if (isTransformNeeded) {
-          if (indexRef.getTable().getViewIndexId() != null) {
-            throw new SQLExceptionInfo.Builder(
-              SQLExceptionCode.CANNOT_TRANSFORM_LOCAL_OR_VIEW_INDEX).setSchemaName(schemaName)
-                .setTableName(indexName).build().buildException();
-          }
-          acquiredTransformLock =
-            connection.getQueryServices().acquireTransformLock(tenantId, schemaName, tableName);
-          if (!acquiredTransformLock) {
-            throw new SQLExceptionInfo.Builder(
-              SQLExceptionCode.CANNOT_MODIFY_TABLE_WITH_TRANSFORM_IN_PROGRESS)
-                .setSchemaName(schemaName).setTableName(tableName).build().buildException();
-          }
-          // Re-check under lock with fresh metadata: a concurrent transform-triggering ALTER may
-          // have committed between our first checkIsTransformNeeded and the lock acquire. If the
-          // observed transform is no longer needed, drop through and treat as no-op.
-          PTable freshTable =
-            connection.getTableNoCache(SchemaUtil.getTableName(schemaName, tableName));
-          boolean stillTransformNeeded = TransformClient.checkIsTransformNeeded(metaProperties,
-            schemaName, freshTable, indexName, dataTableName, tenantId, connection);
-          if (stillTransformNeeded) {
-            try {
-              TransformClient.addTransform(connection, tenantId, table, metaProperties, seqNum,
-                PTable.TransformType.METADATA_TRANSFORM);
-            } catch (SQLException ex) {
-              connection.rollback();
-              throw ex;
-            }
+        if (isTransformNeeded && stillTransformNeeded) {
+          try {
+            TransformClient.addTransform(connection, tenantId, freshTable, metaProperties, seqNum,
+              PTable.TransformType.METADATA_TRANSFORM);
+          } catch (SQLException ex) {
+            connection.rollback();
+            throw ex;
           }
         }