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 d7379a6203..5567b23f5f 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 @@ -434,16 +434,25 @@ 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; - invalidateCache(); } /** 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; - invalidateCache(); } /** Returns the PostgreSQL version. */ 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 2f4b3ca455..c8e7dc89eb 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,6 +31,7 @@ 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; @@ -206,6 +207,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("UTC"), 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));