Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new read-only connection property, database_dialect, which is automatically determined at startup, along with unit tests to verify its behavior for PostgreSQL and Google Standard SQL. A critical concurrency issue was identified in driver.go, where updating the initialPropertyValues map could lead to a race condition and potential panic because concurrent reads in other methods are not synchronized with the same lock.
olavloite
force-pushed
the
add-database-dialect-read-only-property
branch
from
191bf4f to
5d2b7c1
Compare
Adds a database_dialect property that shows the dialect of the current database. This property is automatically determined at startup and cannot be manually modified. This is different from the 'dialect' connection property, which can be set manually, and is used to determine the dialect that should be used for new databases that are created from this connection. The latter is primarily used in combination with the auto_config_emulator property. Fixes #784
olavloite
force-pushed
the
add-database-dialect-read-only-property
branch
from
5d2b7c1 to
0b72b6e
Compare
rahul2393
left a comment
rahul2393
left a comment
There was a problem hiding this comment.
LGTM with some nits, feel free to address them separately
| c.connectorConfig.Instance, | ||
| c.connectorConfig.Database) | ||
| if value, ok := c.initialPropertyValues[propertyConnectTimeout.Key()]; ok { | ||
| c.clientMu.Lock() |
There was a problem hiding this comment.
This is unrelated change, is it to fix some existing race condition?
There was a problem hiding this comment.
It is actually related. This change writes to c.initialPropertyValues (it writes the database_dialect value) after the connector has been created, and at the same time there are potentially concurrent reads from this map. The latter happens if the application tries to open multiple connections in parallel using the same connector. These connections all try to read the connect_timeout value from this map.
driver.go
Outdated
| } | ||
|
|
||
| if err := c.increaseConnCount(ctx, databaseName, opts); err != nil { | ||
|
|
driver.go
Outdated
| connectionStateType = connectionstate.TypeNonTransactional | ||
| } | ||
| } | ||
|
|
driver.go
Outdated
| execSingleDMLTransactional: execInNewRWTransaction, | ||
| execSingleDMLPartitioned: execAsPartitionedDML, | ||
| } | ||
|
|
driver.go
Outdated
| // Initialize the session. | ||
| _ = connection.ResetSession(context.Background()) | ||
| return connection, nil | ||
|
|
| verifyConnectionPropertyValue(t, conn, "database_dialect", "POSTGRESQL") | ||
| // Verify that 'dialect' property is NOT updated and remains empty/unspecified. | ||
| verifyConnectionPropertyValue(t, conn, "dialect", "") | ||
| verifySetFails(t, conn, "database_dialect", "'GOOGLE_STANDARD_SQL'") |
There was a problem hiding this comment.
nit: The property defaults to DATABASE_DIALECT_UNSPECIFIED. What happens if database_dialect is queried before the dialect is determined (e.g., if increaseConnCount fails). Can it be ignored?
The property's defaultValue is databasepb.DatabaseDialect_DATABASE_DIALECT_UNSPECIFIED (an enum/int), but the test asserts string values like "POSTGRESQL". This works because of the converter/display logic, but it's worth verifying that the SHOW VARIABLE path consistently formats the enum as its string name. If someone changes the formatting, these tests could silently pass with wrong values.
There was a problem hiding this comment.
Regarding the first: It is impossible to read DATABASE_DIALECT_UNSPECIFIED from the connection, because the query that is used to determine the dialect of the underlying database must successfully succeed before a connection is returned. So there are two possibilities:
- An application requests a connection and the dialect of the database has not yet been determined for this connector: The query is executed, it successfully returns the dialect, and only then is a connection returned to the application.
- An application requests a connection and the dialect of the database has not yet been determined for this connector: The query is executed and it fails and returns an error. The connector does not return a connection to the application, but an error.
Regarding the latter: These tests verify exactly that; The verifyConnectionPropertyValue constructs a SHOW [VARIABLE] <variable name> statement and executes it, and then verifies that the returned value is the expected value. So if anyone changes the format function of those enums, then this test will fail. The show statement is 'hidden' in a separate helper method because the syntax is slightly different for GoogleSQL and PostgreSQL, so it is easier to use a helper method to generate it.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
2 participants
Adds a database_dialect property that shows the dialect of the current database. This property is automatically determined at startup and cannot be manually modified. This is different from the 'dialect' connection property, which can be set manually, and is used to determine the dialect that should be used for new databases that are created from this connection. The latter is primarily used in combination with the auto_config_emulator property.
Fixes #784