feat: support PG connection properties in driver - #886
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces PostgreSQL compatibility connection properties (GUCs) and corresponding mock server tests for the Spanner PostgreSQL dialect. The feedback suggests optimizing performance by pre-computing the merged PostgreSQL connection properties map during package initialization, which avoids map allocation and copying overhead on every connection creation.
65c870d to
cadca3f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces PostgreSQL compatibility connection properties (GUCs) such as application_name, client_min_messages, client_encoding, timezone, default_transaction_isolation, server_version_num, and server_version for the PostgreSQL dialect, along with corresponding tests. Feedback recommends using Go subtests (t.Run) to isolate distinct test cases in accordance with the repository style guide, and adding missing verifyShowFails assertions to align with the test's documented behavior.
cadca3f to
6c724cf
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds PostgreSQL compatibility connection properties (GUCs) such as application_name, client_min_messages, client_encoding, timezone, default_transaction_isolation, server_version_num, and server_version to support client library setup queries when using the PostgreSQL dialect. The review feedback suggests making default_transaction_isolation writable (validating that it is set to serializable) instead of strictly read-only, as standard PostgreSQL drivers or ORMs may attempt to set this parameter during initialization. Additionally, the feedback recommends adding a test case to verify that setting it to 'serializable' succeeds and updating the corresponding subtest name.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces PostgreSQL compatibility connection properties (GUCs) such as application_name, client_min_messages, client_encoding, timezone, default_transaction_isolation, server_version_num, and server_version to support client library setup queries when using the PostgreSQL dialect. It also adds corresponding unit tests. The review feedback correctly identifies that the newly acquired database connections in the tests are not closed, which could lead to connection leaks, and suggests using defer silentClose(conn) to resolve this.
6c724cf to
c9bd260
Compare
2b84fe9 to
0c170f4
Compare
0c170f4 to
d315b3f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces PostgreSQL compatibility connection properties (GUCs) for the Spanner PostgreSQL dialect, including properties like application_name, client_min_messages, client_encoding, timezone, default_transaction_isolation, server_version_num, and server_version. It updates connection state initialization to load these properties when using the PostgreSQL dialect and adds support for formatting time.Duration values in SHOW statements. Comprehensive mock server tests are also added to verify the behavior of these properties under both PostgreSQL and GoogleSQL dialects. There are no review comments, so no additional feedback is provided.
Summary
Adds support for PostgreSQL-specific GUC configuration properties (
application_name,timezone,client_min_messages,client_encoding,default_transaction_isolation,server_version, andserver_version_num). These defaults and no-op properties enable standard PostgreSQL drivers to connect and execute initial setup queries seamlessly.Changes
postgresConnectionPropertiesduring packageinit()to avoid map allocations and copying on every connection setup.conn_with_mockserver_test.goverifying read/write behavior, read-only validation, and dialect restriction.