Skip to content

feat: support multiple PostgreSQL transaction options #1949

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jul 22, 2022
Merged

feat: support multiple PostgreSQL transaction options #1949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1d8e6c0
feat: support multiple PostgreSQL transaction options
olavloite Jul 16, 2022
8868723
fix: remove duplicated statement
olavloite Jul 16, 2022
2cc7ac2
chore: run code formatter
olavloite Jul 16, 2022
6ff3a60
fix: support 'to' in set statements
olavloite Jul 20, 2022
9e0581d
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Jul 20, 2022
5521db4
Merge branch 'main' into pg-transaction-options
olavloite Jul 20, 2022
e27c841
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Jul 20, 2022
93f6f5a
fix: support on/off yes/no 1/0
olavloite Jul 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@ implementation 'com.google.cloud:google-cloud-spanner'
If you are using Gradle without BOM, add this to your dependencies:

```Groovy
implementation 'com.google.cloud:google-cloud-spanner:6.26.0'
implementation 'com.google.cloud:google-cloud-spanner:6.27.0'
```

If you are using SBT, add this to your dependencies:

```Scala
libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.26.0"
libraryDependencies += "com.google.cloud" % "google-cloud-spanner" % "6.27.0"
```

## Authentication
Expand Down
125 changes: 119 additions & 6 deletions ...src/main/java/com/google/cloud/spanner/connection/ClientSideStatementValueConverters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,16 @@
import com.google.cloud.spanner.SpannerExceptionFactory;
import com.google.cloud.spanner.TimestampBound;
import com.google.cloud.spanner.TimestampBound.Mode;
import com.google.cloud.spanner.connection.PgTransactionMode.AccessMode;
import com.google.cloud.spanner.connection.PgTransactionMode.IsolationLevel;
import com.google.common.base.Function;
import com.google.common.base.Preconditions;
import com.google.protobuf.Duration;
import com.google.protobuf.util.Durations;
import com.google.spanner.v1.RequestOptions.Priority;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import java.util.regex.Matcher;
Expand Down Expand Up @@ -85,6 +88,53 @@ public Boolean convert(String value) {
}
}

/** Converter from string to {@link Boolean} */
static class PgBooleanConverter implements ClientSideStatementValueConverter<Boolean> {

public PgBooleanConverter(String allowedValues) {}

@Override
public Class<Boolean> getParameterClass() {
return Boolean.class;
}

@Override
public Boolean convert(String value) {
if (value == null) {
return null;
}
if (value.length() > 1
&& ((value.startsWith("'") && value.endsWith("'"))
|| (value.startsWith("\"") && value.endsWith("\"")))) {
Copy link
Contributor

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

Copy link
Collaborator Author

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)

value = value.substring(1, value.length() - 1);
}
if ("true".equalsIgnoreCase(value)
Copy link
Contributor

@gauravsnj gauravsnj Jul 21, 2022 •
edited
Loading

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 '

Copy link
Collaborator Author

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.

|| "tru".equalsIgnoreCase(value)
|| "tr".equalsIgnoreCase(value)
|| "t".equalsIgnoreCase(value)
|| "on".equalsIgnoreCase(value)
|| "1".equalsIgnoreCase(value)
|| "yes".equalsIgnoreCase(value)
|| "ye".equalsIgnoreCase(value)
|| "y".equalsIgnoreCase(value)) {
return Boolean.TRUE;
}
if ("false".equalsIgnoreCase(value)
|| "fals".equalsIgnoreCase(value)
Copy link
Contributor

@gauravsnj gauravsnj Jul 21, 2022 •
edited
Loading

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

Copy link
Collaborator Author

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 and n are allowed both with and without quotes.
  • Spaces inside quotes are not allowed, so ' true ' is not a valid value.

Copy link
Contributor

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

|| "fal".equalsIgnoreCase(value)
Copy link
Contributor

@gauravsnj gauravsnj Jul 21, 2022 •
edited
Loading

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.

Copy link
Collaborator Author

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).

|| "fa".equalsIgnoreCase(value)
|| "f".equalsIgnoreCase(value)
|| "off".equalsIgnoreCase(value)
|| "of".equalsIgnoreCase(value)
|| "0".equalsIgnoreCase(value)
|| "no".equalsIgnoreCase(value)
|| "n".equalsIgnoreCase(value)) {
return Boolean.FALSE;
}
return null;
}
}

/** Converter from string to {@link Duration}. */
static class DurationConverter implements ClientSideStatementValueConverter<Duration> {
private final Pattern allowedValues;
Expand Down Expand Up @@ -286,16 +336,39 @@ public TransactionMode convert(String value) {
}
}

static class PgTransactionIsolationConverter
implements ClientSideStatementValueConverter<IsolationLevel> {
private final CaseInsensitiveEnumMap<IsolationLevel> values =
new CaseInsensitiveEnumMap<>(IsolationLevel.class, IsolationLevel::getShortStatementString);

public PgTransactionIsolationConverter(String allowedValues) {}

@Override
public Class<IsolationLevel> getParameterClass() {
return IsolationLevel.class;
}

@Override
public IsolationLevel convert(String value) {
// Isolation level may contain multiple spaces.
String valueWithSingleSpaces = value.replaceAll("\\s+", " ");
if (valueWithSingleSpaces.length() > 1
&& ((valueWithSingleSpaces.startsWith("'") && valueWithSingleSpaces.endsWith("'"))
|| (valueWithSingleSpaces.startsWith("\"")
&& valueWithSingleSpaces.endsWith("\"")))) {
valueWithSingleSpaces =
valueWithSingleSpaces.substring(1, valueWithSingleSpaces.length() - 1);
}
return values.get(valueWithSingleSpaces);
}
}

/**
* Converter for converting string values to {@link PgTransactionMode} values. Includes no-op
* handling of setting the isolation level of the transaction to default or serializable.
*/
static class PgTransactionModeConverter
implements ClientSideStatementValueConverter<PgTransactionMode> {
private final CaseInsensitiveEnumMap<PgTransactionMode> values =
new CaseInsensitiveEnumMap<>(
PgTransactionMode.class, PgTransactionMode::getStatementString);

PgTransactionModeConverter() {}

public PgTransactionModeConverter(String allowedValues) {}
Expand All @@ -307,9 +380,49 @@ public Class<PgTransactionMode> getParameterClass() {

@Override
public PgTransactionMode convert(String value) {
PgTransactionMode mode = new PgTransactionMode();
// Transaction mode may contain multiple spaces.
String valueWithSingleSpaces = value.replaceAll("\\s+", " ");
return values.get(valueWithSingleSpaces);
String valueWithSingleSpaces =
value.replaceAll("\\s+", " ").toLowerCase(Locale.ENGLISH).trim();
int currentIndex = 0;
while (currentIndex < valueWithSingleSpaces.length()) {
// This will use the last access mode and isolation level that is encountered in the string.
// This is consistent with the behavior of PostgreSQL, which also allows multiple modes to
// be specified in one string, and will use the last one that is encountered.
if (valueWithSingleSpaces.substring(currentIndex).startsWith("read only")) {
currentIndex += "read only".length();
mode.setAccessMode(AccessMode.READ_ONLY_TRANSACTION);
} else if (valueWithSingleSpaces.substring(currentIndex).startsWith("read write")) {
currentIndex += "read write".length();
mode.setAccessMode(AccessMode.READ_WRITE_TRANSACTION);
} else if (valueWithSingleSpaces
.substring(currentIndex)
.startsWith("isolation level serializable")) {
currentIndex += "isolation level serializable".length();
mode.setIsolationLevel(IsolationLevel.ISOLATION_LEVEL_SERIALIZABLE);
} else if (valueWithSingleSpaces
.substring(currentIndex)
.startsWith("isolation level default")) {
currentIndex += "isolation level default".length();
mode.setIsolationLevel(IsolationLevel.ISOLATION_LEVEL_DEFAULT);
} else {
return null;
}
// Skip space and/or comma that may separate multiple transaction modes.
if (currentIndex < valueWithSingleSpaces.length()
&& valueWithSingleSpaces.charAt(currentIndex) == ' ') {
currentIndex++;
}
if (currentIndex < valueWithSingleSpaces.length()
&& valueWithSingleSpaces.charAt(currentIndex) == ',') {
currentIndex++;
}
if (currentIndex < valueWithSingleSpaces.length()
&& valueWithSingleSpaces.charAt(currentIndex) == ' ') {
currentIndex++;
}
}
return mode;
}
}

Expand Down
3 changes: 3 additions & 0 deletions ...panner/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.cloud.spanner.connection;

import com.google.cloud.spanner.TimestampBound;
import com.google.cloud.spanner.connection.PgTransactionMode.IsolationLevel;
import com.google.protobuf.Duration;
import com.google.spanner.v1.RequestOptions.Priority;

Expand Down Expand Up @@ -98,6 +99,8 @@ interface ConnectionStatementExecutor {
StatementResult statementSetPgSessionCharacteristicsTransactionMode(
PgTransactionMode transactionMode);

StatementResult statementSetPgDefaultTransactionIsolation(IsolationLevel isolationLevel);

StatementResult statementStartBatchDdl();

StatementResult statementStartBatchDml();
Expand Down
52 changes: 30 additions & 22 deletions ...er/src/main/java/com/google/cloud/spanner/connection/ConnectionStatementExecutorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.RUN_BATCH;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_AUTOCOMMIT;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_AUTOCOMMIT_DML_MODE;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_DEFAULT_TRANSACTION_ISOLATION;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_OPTIMIZER_STATISTICS_PACKAGE;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_OPTIMIZER_VERSION;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_READONLY;
Expand Down Expand Up @@ -70,6 +71,7 @@
import com.google.cloud.spanner.TimestampBound;
import com.google.cloud.spanner.Type;
import com.google.cloud.spanner.Type.StructField;
import com.google.cloud.spanner.connection.PgTransactionMode.IsolationLevel;
import com.google.cloud.spanner.connection.ReadOnlyStalenessUtil.DurationValueGetter;
import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
Expand Down Expand Up @@ -372,39 +374,45 @@ public StatementResult statementSetTransactionMode(TransactionMode mode) {

@Override
public StatementResult statementSetPgTransactionMode(PgTransactionMode transactionMode) {
switch (transactionMode) {
case READ_ONLY_TRANSACTION:
getConnection().setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION);
break;
case READ_WRITE_TRANSACTION:
getConnection().setTransactionMode(TransactionMode.READ_WRITE_TRANSACTION);
break;
case ISOLATION_LEVEL_DEFAULT:
case ISOLATION_LEVEL_SERIALIZABLE:
default:
// no-op
if (transactionMode.getAccessMode() != null) {
switch (transactionMode.getAccessMode()) {
case READ_ONLY_TRANSACTION:
getConnection().setTransactionMode(TransactionMode.READ_ONLY_TRANSACTION);
break;
case READ_WRITE_TRANSACTION:
getConnection().setTransactionMode(TransactionMode.READ_WRITE_TRANSACTION);
break;
default:
// no-op
}
}
return noResult(SET_TRANSACTION_MODE);
}

@Override
public StatementResult statementSetPgSessionCharacteristicsTransactionMode(
PgTransactionMode transactionMode) {
switch (transactionMode) {
case READ_ONLY_TRANSACTION:
getConnection().setReadOnly(true);
break;
case READ_WRITE_TRANSACTION:
getConnection().setReadOnly(false);
break;
case ISOLATION_LEVEL_DEFAULT:
case ISOLATION_LEVEL_SERIALIZABLE:
default:
// no-op
if (transactionMode.getAccessMode() != null) {
switch (transactionMode.getAccessMode()) {
case READ_ONLY_TRANSACTION:
getConnection().setReadOnly(true);
break;
case READ_WRITE_TRANSACTION:
getConnection().setReadOnly(false);
break;
default:
// no-op
}
}
return noResult(SET_TRANSACTION_MODE);
}

@Override
public StatementResult statementSetPgDefaultTransactionIsolation(IsolationLevel isolationLevel) {
// no-op
return noResult(SET_DEFAULT_TRANSACTION_ISOLATION);
}

@Override
public StatementResult statementStartBatchDdl() {
getConnection().startBatchDdl();
Expand Down
Loading