Repository: GoogleCloudPlatform/pgadapter
Pull request #2949 (2025-02-27 14:45:41)

fix: remove 'for update of' clauses
Spanner now supports FOR UPDATE clauses in SELECT statements. However, Spanner does not support FOR UPDATE OF tbl statements. This change re-introduces automatic replacement of those statements with a simple FOR UPDATE clause.

This fixes the build error in https://github.com/GoogleCloudPlatform/pgadapter/actions/runs/13567564731/job/37924031938?pr=2947 (once the emulator supports `for update` clauses).


Patch:
diff --git a/samples/golang/gorm/create_data_model.sql b/samples/golang/gorm/create_data_model.sql
index 2c9ba6c5..dce8ff3c 100644
--- a/samples/golang/gorm/create_data_model.sql
+++ b/samples/golang/gorm/create_data_model.sql
@@ -63,18 +63,10 @@ create table if not exists concerts (
     constraint chk_end_time_after_start_time check (end_time > start_time)
 );
 
--- Create a bit-reversed sequence that will be used to generate identifiers for the ticket_sales table.
--- See also https://cloud.google.com/spanner/docs/reference/postgresql/data-definition-language#create_sequence
--- Note that the 'bit_reversed_positive' keyword is required for Spanner,
--- and is automatically skipped for open-source PostgreSQL.
-create sequence if not exists ticket_sale_seq
-    /* skip_on_open_source_pg */ bit_reversed_positive
-    /* skip_on_open_source_pg */ skip range 1 1000
-    /* skip_on_open_source_pg */ start counter with 50000
-;
+/* skip_on_open_source_pg */ alter database db set spanner.default_sequence_kind='bit_reversed_positive';
 
 create table if not exists ticket_sales (
-    id bigint not null primary key default nextval('ticket_sale_seq'),
+    id               serial primary key,
     concert_id       varchar not null,
     customer_name    varchar not null,
     price            decimal not null,
diff --git a/samples/golang/gorm/sample.go b/samples/golang/gorm/sample.go
index e3a10cc6..7a2265e9 100644
--- a/samples/golang/gorm/sample.go
+++ b/samples/golang/gorm/sample.go
@@ -108,7 +108,8 @@ type Concert struct {
 }
 
 // TicketSale uses the standard gorm.Model as its base model. This model uses an auto-generated `uint` primary key
-// value. The value is generated by a bit-reversed sequence in the database.
+// value. The value is generated using the serial data type. This uses a backing bit-reversed sequence that
+// automatically generates unique, non-monotonically increasing values.
 type TicketSale struct {
 	gorm.Model
 	Concert      Concert
@@ -414,9 +415,10 @@ func CreateVenueAndConcertInTransaction(db *gorm.DB) error {
 			return res.Error
 		}
 		// Create a ticket sale for the concert.
-		// TicketSale uses `gorm.Model` as its base Model. This model uses a bit-reversed sequence
+		// TicketSale uses `gorm.Model` as its base Model. This model uses the serial data type
 		// to generate primary key values, meaning that we do not have to supply a value for it in
-		// the application.
+		// the application. The serial data type is backed by a bit-reversed sequence, meaning that
+		// the primary key values that are generated are not monotonically increasing.
 		price, _ := decimal.NewFromString("30.99")
 		ticketSale := TicketSale{
 			ConcertId:    concert.ID,
diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/ConnectionHandler.java b/src/main/java/com/google/cloud/spanner/pgadapter/ConnectionHandler.java
index 46c9d2fc..98f2b8c1 100644
--- a/src/main/java/com/google/cloud/spanner/pgadapter/ConnectionHandler.java
+++ b/src/main/java/com/google/cloud/spanner/pgadapter/ConnectionHandler.java
@@ -273,6 +273,13 @@ public class ConnectionHandler implements Runnable {
     this.spannerConnection = spannerConnection;
     this.databaseId = connectionOptions.getDatabaseId();
     this.extendedQueryProtocolHandler = new ExtendedQueryProtocolHandler(this);
+    // TODO: Remove when the emulator supports FOR UPDATE clauses.
+    if (Boolean.parseBoolean(server.getProperties().getProperty("autoConfigEmulator", "false"))) {
+      this.extendedQueryProtocolHandler
+          .getBackendConnection()
+          .getSessionState()
+          .setConnectionStartupValue("spanner", "replace_for_update", "true");
+    }
   }
 
   @VisibleForTesting
diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/statements/BackendConnection.java b/src/main/java/com/google/cloud/spanner/pgadapter/statements/BackendConnection.java
index 7afc6dc8..cd477f08 100644
--- a/src/main/java/com/google/cloud/spanner/pgadapter/statements/BackendConnection.java
+++ b/src/main/java/com/google/cloud/spanner/pgadapter/statements/BackendConnection.java
@@ -364,7 +364,11 @@ public class BackendConnection {
           //       feature is able to detect FOR UPDATE clauses as a write.
           if (sessionState.isReplaceForUpdateClause()
               && !spannerConnection.isDelayTransactionStartUntilFirstWrite()) {
-            updatedStatement = replaceForUpdate(updatedStatement, sqlLowerCase);
+            updatedStatement =
+                replaceForUpdate(updatedStatement, sqlLowerCase, /*replaceWithHint=*/ true);
+          } else {
+            updatedStatement =
+                replaceForUpdate(updatedStatement, sqlLowerCase, /*replaceWithHint=*/ false);
           }
           updatedStatement = bindStatement(updatedStatement, sqlLowerCase);
           result.set(analyzeOrExecute(updatedStatement));
diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/statements/SimpleParser.java b/src/main/java/com/google/cloud/spanner/pgadapter/statements/SimpleParser.java
index f926e086..63cc3dc5 100644
--- a/src/main/java/com/google/cloud/spanner/pgadapter/statements/SimpleParser.java
+++ b/src/main/java/com/google/cloud/spanner/pgadapter/statements/SimpleParser.java
@@ -226,8 +226,9 @@ public class SimpleParser {
   }
 
   /**
-   * Replaces any 'for update' clause with a LOCK_SCANNED_RANGES=exclusive hint. This method only
-   * replaces the 'for update' clause if it fulfills these criteria:
+   * Replaces any 'for update' clause with a LOCK_SCANNED_RANGES=exclusive hint and/or removes any
+   * 'of table1[, table2[,...]]' clauses. This method only replaces the 'for update' clause if it
+   * fulfills these criteria:
    *
    * <ol>
    *   <li>The 'for update' clause is on the outermost expression
@@ -237,7 +238,8 @@ public class SimpleParser {
    *
    * Any 'OF table_name[, ...]' clauses in the expression are ignored.
    */
-  static Statement replaceForUpdate(Statement statement, String lowerCaseSql) {
+  static Statement replaceForUpdate(
+      Statement statement, String lowerCaseSql, boolean replaceWithHint) {
     // If there is no 'for' clause, then we know that we don't have to analyze any further.
     if (!lowerCaseSql.contains("for")) {
       return statement;
@@ -249,6 +251,9 @@ public class SimpleParser {
     }
     int startPos = parser.pos;
     if (parser.eatKeyword("for") && parser.eatKeyword("update")) {
+      if (!replaceWithHint) {
+        startPos = parser.pos;
+      }
       int endPos = parser.pos;
       // Skip 'of table1[, table2[, ...]] clauses
       if (parser.eatKeyword("of")) {
@@ -257,6 +262,9 @@ public class SimpleParser {
           return statement;
         }
         endPos = parser.pos;
+      } else if (!replaceWithHint) {
+        // Stop here if the statement does not contain an 'of table' clause.
+        return statement;
       }
       // 'nowait' and 'skip locked' clauses are not supported.
       if (parser.eatKeyword("nowait") || parser.eatKeyword("skip")) {
@@ -266,11 +274,16 @@ public class SimpleParser {
         return statement;
       }
       // This is a simple 'for update' clause. Replace it with a 'LOCK_SCANNED_RANGES=exclusive'
-      // hint.
-      return Statement.of(
-          "/*@ LOCK_SCANNED_RANGES=exclusive */"
-              + statement.getSql().substring(0, startPos)
-              + statement.getSql().substring(endPos));
+      // hint and/or remove the 'of table' clause.
+      if (replaceWithHint) {
+        return Statement.of(
+            "/*@ LOCK_SCANNED_RANGES=exclusive */"
+                + statement.getSql().substring(0, startPos)
+                + statement.getSql().substring(endPos));
+      } else {
+        return Statement.of(
+            statement.getSql().substring(0, startPos) + statement.getSql().substring(endPos));
+      }
     }
     return statement;
   }


Test patch:
diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/statements/SelectForUpdateTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/statements/SelectForUpdateTest.java
index 7352b4bd..dd463417 100644
--- a/src/test/java/com/google/cloud/spanner/pgadapter/statements/SelectForUpdateTest.java
+++ b/src/test/java/com/google/cloud/spanner/pgadapter/statements/SelectForUpdateTest.java
@@ -67,14 +67,35 @@ public class SelectForUpdateTest {
             "select col1 from my_table for update of my_table, /*comment*/ my_other_table"));
   }
 
+  @Test
+  public void testReplaceForUpdateOf() {
+    assertEquals(
+        Statement.of("select col1 from foo for update"),
+        internalReplaceForUpdate("select col1 from foo for update", false));
+    assertEquals(
+        Statement.of("select col1 from foo for update"),
+        internalReplaceForUpdate("select col1 from foo for update of foo", false));
+    assertEquals(
+        Statement.of("select col1 from foo for update"),
+        internalReplaceForUpdate("select col1 from foo for update of foo, bar", false));
+    assertEquals(
+        Statement.of("select col1 from foo for update"),
+        internalReplaceForUpdate("select col1 from foo for update of foo, bar, \"baz\"", false));
+  }
+
   private void assertSameAfterRemoveForUpdate(String sql) {
     Statement statement = Statement.of(sql);
     assertSame(
-        statement, replaceForUpdate(statement, statement.getSql().toLowerCase(Locale.ENGLISH)));
+        statement,
+        replaceForUpdate(statement, statement.getSql().toLowerCase(Locale.ENGLISH), true));
   }
 
   private Statement internalReplaceForUpdate(String sql) {
-    return replaceForUpdate(Statement.of(sql), sql.toLowerCase(Locale.ENGLISH));
+    return internalReplaceForUpdate(sql, true);
+  }
+
+  private Statement internalReplaceForUpdate(String sql, boolean replaceWithHint) {
+    return replaceForUpdate(Statement.of(sql), sql.toLowerCase(Locale.ENGLISH), replaceWithHint);
   }
 
   @Test