Repository navigation
fix: use database default isolation level when none is specified - #5041
rayudu3745 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements support for setting, resetting, and showing the default_transaction_isolation session setting in PGAdapter, aligning it with PostgreSQL behavior. It introduces tracking of initial, default, and transaction-specific isolation levels in SessionState, and synchronizes these levels with the underlying Spanner connection during transaction boundaries and client-side statements. Additionally, the SessionStatementParser is updated to handle SET and RESET commands for default_transaction_isolation. The feedback suggests refactoring getTransactionIsolationLevelFromConnection in BackendConnection.java to store the result of rs.getString(0) in a local variable to avoid redundant calls and improve readability.
9b09b63 to
8464953
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements proper tracking and synchronization of transaction isolation levels (such as default_transaction_isolation and transaction_isolation) between PGAdapter's session state and the underlying Spanner connection. It removes the previous hardcoded serializable override, adds parser support for isolation level settings, and updates tests to reflect these changes. The review feedback suggests avoiding a potential NullPointerException by checking !rs.isNull(0) before calling rs.getString(0) in BackendConnection, and recommends enhancing PGSetting.unquote to handle both single and double quotes to simplify the unquoting logic in SessionState.
8464953 to
29e24b0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements comprehensive support for managing and tracking transaction isolation levels (such as default_transaction_isolation and transaction_isolation) within PGAdapter's session state, aligning it with PostgreSQL behavior. Instead of unconditionally overriding unspecified isolation levels to SERIALIZABLE at startup, PGAdapter now dynamically tracks the initial, default, and active transaction-level isolation levels. It introduces parsing support for setting and showing these isolation levels (including SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL), synchronizes session state with the underlying Cloud Spanner connection, and updates various mock server and client library tests to reflect these changes. There are no review comments to address, and the changes look solid.
29e24b0 to
8cf809e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors and enhances the handling of transaction isolation levels in PGAdapter, aligning it closer to PostgreSQL behavior. It introduces tracking for database default, initial, default, and active transaction isolation levels within SessionState, and synchronizes these states with client-side statements in BackendConnection. Additionally, it updates SessionStatementParser to support SET and RESET operations for default_transaction_isolation and transaction_isolation, and adds comprehensive tests. The review feedback suggests simplifying the manual unquoting logic in BackendConnection by reusing SessionState.toSpannerIsolationLevel, and refactoring the parsing of SET CHARACTERISTICS AS TRANSACTION in SessionStatementParser to support optional SESSION keywords for better compatibility.
| String unquoted = value != null ? value.trim() : null; | ||
| if (unquoted != null | ||
| && unquoted.length() >= 2 | ||
| && unquoted.startsWith("'") | ||
| && unquoted.endsWith("'")) { | ||
| unquoted = unquoted.substring(1, unquoted.length() - 1); | ||
| } | ||
| IsolationLevel level = SessionState.toSpannerIsolationLevel(unquoted); |
There was a problem hiding this comment.
The manual unquoting and trimming logic here is redundant because SessionState.toSpannerIsolationLevel(value) already internally calls PGSetting.unquote(value.trim()) and handles both single and double quotes, as well as null values. We can simplify this block to a single line.
| String unquoted = value != null ? value.trim() : null; | |
| if (unquoted != null | |
| && unquoted.length() >= 2 | |
| && unquoted.startsWith("'") | |
| && unquoted.endsWith("'")) { | |
| unquoted = unquoted.substring(1, unquoted.length() - 1); | |
| } | |
| IsolationLevel level = SessionState.toSpannerIsolationLevel(unquoted); | |
| IsolationLevel level = SessionState.toSpannerIsolationLevel(value); |
| boolean ateSession = false; | ||
| if (parser.eatKeyword("local")) { | ||
| builder.setLocal(); | ||
| } else { | ||
| // Ignore, this is the default. | ||
| parser.eatKeyword("session"); | ||
| ateSession = parser.eatKeyword("session"); | ||
| } | ||
| if (!builder.local | ||
| && ateSession | ||
| && eatKeywordsAtomically( | ||
| parser, "characteristics", "as", "transaction", "isolation", "level")) { |
There was a problem hiding this comment.
We can simplify the parsing of SET CHARACTERISTICS AS TRANSACTION by removing the ateSession variable and checking for the keywords atomically. This also has the benefit of supporting both SET SESSION CHARACTERISTICS... and SET CHARACTERISTICS... (where SESSION is optional in PostgreSQL), improving compatibility.
if (parser.eatKeyword("local")) {
builder.setLocal();
} else {
parser.eatKeyword("session");
}
if (!builder.local
&& eatKeywordsAtomically(
parser, "characteristics", "as", "transaction", "isolation", "level")) {
Previously,
ConnectionHandlerexplicitly set the connection's default isolation level toIsolationLevel.SERIALIZABLEwhenever the Spanner connection hadISOLATION_LEVEL_UNSPECIFIED. This was originally added so thatSHOW TRANSACTION ISOLATION LEVELandSHOW DEFAULT_TRANSACTION_ISOLATION(when executed directly on the SpannerConnection) would not returnISOLATION_LEVEL_UNSPECIFIEDto PostgreSQL drivers such aspgjdbc.However, forcing
IsolationLevel.SERIALIZABLEon the SpannerConnectioncaused every transaction to sendSERIALIZABLEinTransactionOptionsinstead ofISOLATION_LEVEL_UNSPECIFIED, overriding any database-level default isolation level configured on Cloud Spanner (e.g.,ALTER DATABASE db SET default_transaction_isolation = 'repeatable read'). In addition, isolation level changes and queries were split between the SpannerConnectionAPI and PGAdapter'sSessionState, causing inconsistencies acrossSHOW,SET,RESET, andpg_settings.This change:
IsolationLevel.SERIALIZABLEoverride inConnectionHandler.connectToSpanner()so connections without an explicit isolation level remain atISOLATION_LEVEL_UNSPECIFIEDand use the database's default isolation level on Cloud Spanner.initialIsolationLevel), session (defaultIsolationLevel), and transactional (transactionDefaultIsolationLevel) isolation levels inSessionState, reportingserializablein session settings when the Spanner isolation level isISOLATION_LEVEL_UNSPECIFIEDwhile preservingISOLATION_LEVEL_UNSPECIFIEDon the underlying Spanner connection.SHOW TRANSACTION ISOLATION LEVEL,SHOW DEFAULT_TRANSACTION_ISOLATION,SET [SESSION | LOCAL] default_transaction_isolation,SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL ...,RESET default_transaction_isolation, andRESET ALLthroughSessionStatementParsersoSessionStateand the SpannerConnectionstay synchronized and return standard PostgreSQL isolation level names (serializable,repeatable read).BEGIN ISOLATION LEVEL ...andSET TRANSACTION ISOLATION LEVEL ...) intoSessionState's localtransaction_isolationsetting, and defers applyingdefault_transaction_isolationchanges to the SpannerConnectionuntil active transactions complete.PGSettingso multi-word enum values inpg_settings.txt(such asrepeatable read) pass validation.