From d4baf9857270ab311fc85853ff68f038e1e383ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Fri, 11 Sep 2026 12:09:23 +0200 Subject: [PATCH] fix: cached session settings survived a rollback The cached values for settings like TimeZone were only cleared by set(..), and not by commit() or rollback(). A SET inside a transaction that was rolled back therefore left the cached value in place, and it stayed wrong until the next SET happened to clear it. Clear the cache on commit and rollback, but only when the transaction actually changed a setting. commit() runs for every statement in autocommit mode, so clearing it unconditionally would keep almost nothing in the cache. --- .../pgadapter/session/SessionState.java | 11 +++ .../pgadapter/session/SessionStateTest.java | 68 +++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/src/main/java/com/google/cloud/spanner/pgadapter/session/SessionState.java b/src/main/java/com/google/cloud/spanner/pgadapter/session/SessionState.java index a5418a6578..df756a8f5f 100644 --- a/src/main/java/com/google/cloud/spanner/pgadapter/session/SessionState.java +++ b/src/main/java/com/google/cloud/spanner/pgadapter/session/SessionState.java @@ -415,12 +415,23 @@ public void commit() { settings.put(toKey(setting.getExtension(), setting.getName()), setting); } } + // Dropping the local settings can uncover a different value, so the cache must go. Promoting + // the transaction settings cannot: internalGet(..) returns the same PGSetting instance either + // way. The distinction matters, as commit() runs for every statement in autocommit mode. + if (localSettings != null) { + invalidateCache(); + } this.localSettings = null; this.transactionSettings = null; } /** Rolls back the current transaction and abandons any pending changes to the settings. */ public void rollback() { + // Both maps are dropped here, so the cached values are only stale if the transaction changed + // something. + if (localSettings != null || transactionSettings != null) { + invalidateCache(); + } this.localSettings = null; this.transactionSettings = null; } diff --git a/src/test/java/com/google/cloud/spanner/pgadapter/session/SessionStateTest.java b/src/test/java/com/google/cloud/spanner/pgadapter/session/SessionStateTest.java index 817fc7682f..b022b6c6ca 100644 --- a/src/test/java/com/google/cloud/spanner/pgadapter/session/SessionStateTest.java +++ b/src/test/java/com/google/cloud/spanner/pgadapter/session/SessionStateTest.java @@ -31,9 +31,11 @@ import com.google.cloud.spanner.pgadapter.error.PGException; import com.google.cloud.spanner.pgadapter.metadata.OptionsMetadata; import com.google.cloud.spanner.pgadapter.metadata.OptionsMetadata.DdlTransactionMode; +import com.google.cloud.spanner.pgadapter.session.PGSetting.Context; import com.google.cloud.spanner.pgadapter.statements.PgCatalog; import com.google.cloud.spanner.pgadapter.utils.ClientAutoDetector.WellKnownClient; import com.google.common.collect.ImmutableMap; +import java.time.ZoneId; import java.util.List; import java.util.Locale; import java.util.Map; @@ -158,6 +160,72 @@ public void testOverwriteLocalSettingWithSessionSetting() { assertEquals("my-app", state.get(null, "application_name").getSetting()); } + @Test + public void testRollbackInvalidatesCachedValues() { + SessionState state = new SessionState(mock(OptionsMetadata.class)); + ZoneId zoneIdBeforeTransaction = state.getTimezone(); + int bufferSizeBeforeTransaction = state.getBinaryConversionBufferSize(); + + state.set(null, "timezone", "America/New_York"); + state.set("spanner", "binary_conversion_buffer_size", "1024"); + // Read the values while the transaction is active, so they are cached. + assertEquals(ZoneId.of("America/New_York"), state.getTimezone()); + assertEquals(1024, state.getBinaryConversionBufferSize()); + + state.rollback(); + + assertEquals(zoneIdBeforeTransaction, state.getTimezone()); + assertEquals(bufferSizeBeforeTransaction, state.getBinaryConversionBufferSize()); + } + + @Test + public void testRollbackInvalidatesCachedLocalValues() { + SessionState state = new SessionState(mock(OptionsMetadata.class)); + ZoneId zoneIdBeforeTransaction = state.getTimezone(); + + state.setLocal(null, "timezone", "America/New_York"); + assertEquals(ZoneId.of("America/New_York"), state.getTimezone()); + + state.rollback(); + + assertEquals(zoneIdBeforeTransaction, state.getTimezone()); + } + + @Test + public void testCommitInvalidatesCachedLocalValues() { + SessionState state = new SessionState(mock(OptionsMetadata.class)); + ZoneId zoneIdBeforeTransaction = state.getTimezone(); + + // A local setting is dropped by a commit, so the cached value must be dropped as well. + state.setLocal(null, "timezone", "America/New_York"); + assertEquals(ZoneId.of("America/New_York"), state.getTimezone()); + + state.commit(); + + assertEquals(zoneIdBeforeTransaction, state.getTimezone()); + } + + @Test + public void testCommitAndRollbackKeepCachedValuesIfNothingChanged() { + SessionState state = new SessionState(mock(OptionsMetadata.class)); + ZoneId cachedZoneId = state.getTimezone(); + assertEquals(ZoneId.of("Europe/Berlin"), cachedZoneId); + + // Modify the setting directly instead of through set(..). That bypasses the cache + // invalidation that set(..) does, which makes the cached value deliberately stale. Any read + // that still returns the stale value therefore proves that the cache was not invalidated. + state.get(null, "timezone").setSetting(Context.SUPERUSER, "America/New_York"); + assertEquals(cachedZoneId, state.getTimezone()); + + // Neither of these changes any value that is visible to the session, so they must not throw + // away the cached values. + state.commit(); + assertEquals(cachedZoneId, state.getTimezone()); + + state.rollback(); + assertEquals(cachedZoneId, state.getTimezone()); + } + @Test public void testGetAll() { SessionState state = new SessionState(mock(OptionsMetadata.class));