-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support multiple PostgreSQL transaction options #1949
Conversation
PostgreSQL allows BEGIN and other transaction statements to set multiple transaction options at once (e.g. both 'read only' and 'isolation level serializable'). This was not supported by the Connection API, which only allowed one option at a time. The Python psycopg2 driver generates statements that include multiple transaction options in one statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a couple of queries, but otherwise looks good.
* @return a string representation of this {@link TransactionMode} that can be used in a SQL | ||
* statement to set the transaction mode. | ||
*/ | ||
public String getStatementString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this AccessMode.getStatementString
get called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be used to generate the SQL string that should be used (i.e. if there was a difference between toString()
and this method), but it is not needed for these enums. Removed.
"method": "statementBeginPgTransaction", | ||
"exampleStatements": [ | ||
"begin", "start", "begin transaction", "start transaction", "begin work", "start work", | ||
"begin read only", "start read only", "begin transaction read only", "start transaction read only", "begin work read only", "start work read only", | ||
"begin read write", "start read write", "begin transaction read write", "start transaction read write", "begin work read write", "start work read write", | ||
"begin isolation level default", "start isolation level default", "begin transaction isolation level default", "start transaction isolation level default", "begin work isolation level default", "start work isolation level default", | ||
"begin isolation level serializable", "start isolation level serializable", "begin transaction isolation level serializable", "start transaction isolation level serializable", "begin work isolation level serializable", "start work isolation level serializable" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to be generating a bunch of these permutations on our own for exampleStatements, how do we track if we are not missing any specific permutation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the short answer is 'we don't', or at best; we do so during code reviews. The examples are based on the PostgreSQL documentation here: https://www.postgresql.org/docs/current/sql-start-transaction.html and on the part of that statement that we support. That is; we don't include example statements for read committed
, as we don't support that isolation level.
} | ||
if ("false".equalsIgnoreCase(value) | ||
|| "fals".equalsIgnoreCase(value) | ||
|| "fal".equalsIgnoreCase(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add support for 'ON', 'OFF', 'yes', 'no', 0 and 1 while converting the boolean values? For the Psycopg2, adding only 'ON' and 'OFF' will do but we can add 'yes', 'no', 0 and 1 to make it fully generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, added for:
on/off
1/0
yes/no
Including all unique prefixes (i.e. ye
is also valid as true
).
"set default_transaction_read_only = t", | ||
"set default_transaction_read_only = f", | ||
"set default_transaction_read_only to 't'", | ||
"set default_transaction_read_only to \"f\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add a couple of example statements here related to ON and OFF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return Boolean.TRUE; | ||
} | ||
if ("false".equalsIgnoreCase(value) | ||
|| "fals".equalsIgnoreCase(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the values like "tru", "fal", "tr"
are supported without single quotes. So, tru
or fal
shouldn't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strangely enough, they are. I've tried a multitude of different statements with a real PG instance. See below for the results:
knut-test-db=# set default_transaction_read_only to "on";
SET
knut-test-db=# set default_transaction_read_only to tr;
SET
knut-test-db=# set default_transaction_read_only to ' true ';
ERROR: parameter "default_transaction_read_only" requires a Boolean value
knut-test-db=# set default_transaction_read_only to "true";
SET
knut-test-db=# set default_transaction_read_only to 'true';
SET
knut-test-db=# set default_transaction_read_only to ye;
SET
knut-test-db=# set default_transaction_read_only to 'ye';
SET
So TLDR:
- Both single and double quotes are allowed (which is strange, as double quotes would normally be used to quote identifiers)
- Unique prefixes like
tr
andn
are allowed both with and without quotes. - Spaces inside quotes are not allowed, so
' true '
is not a valid value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's strange because once I tested inserting boolean values in some table and the results were totally opposite :) Btw thanks for clarifying
|| (value.startsWith("\"") && value.endsWith("\"")))) { | ||
value = value.substring(1, value.length() - 1); | ||
} | ||
if ("true".equalsIgnoreCase(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also trim the string to consider the cases like ' true '
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(See also above). PostgreSQL does not consider that to be a valid boolean value. When the value is quoted with either single or double quotes, it may not contain any spaces.
} | ||
if (value.length() > 1 | ||
&& ((value.startsWith("'") && value.endsWith("'")) | ||
|| (value.startsWith("\"") && value.endsWith("\"")))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Double Quotes("") are used to identify objects inside the database like column names, table names etc. So, "true"
shouldn't work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it strange, but it actually allowed (see above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Source-Link: googleapis/synthtool@4b49307 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:25b384ee1674eda3984ec41c15b514a63bbeb5eda4d57c73c7e6f5adef2fd2f1
…3013) Source-Link: googleapis/synthtool@4b49307 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:25b384ee1674eda3984ec41c15b514a63bbeb5eda4d57c73c7e6f5adef2fd2f1 Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
PostgreSQL allows BEGIN and other transaction statements to set multiple transaction
options at once (e.g. both 'read only' and 'isolation level serializable'). This was
not supported by the Connection API, which only allowed one option at a time. The
Python psycopg2 driver generates statements that include multiple transaction options
in one statement.
This PR adds support for:
begin read only isolation level serializable
begin isolation level serializable, read write
set transaction read only, isolation level serializable
set transaction isolation level serializable read write
DEFAULT_TRANSACTION_ISOLATION
andDEFAULT_TRANSACTION_READ_ONLY
properties