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
c9bd260 to
d5efdbe
Compare
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.