Skip to content
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

Jump to bottom

Return correct default from PgDatabaseMetaData.getDefaultTransactionIsolation #2992

Merged
merged 1 commit into from
Nov 15, 2023
Merged

Return correct default from PgDatabaseMetaData.getDefaultTransactionIsolation #2992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 36 additions & 1 deletion pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java
View file
Open in desktop
Expand Up @@ -38,6 +38,7 @@
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.StringTokenizer;

Expand All @@ -53,6 +54,7 @@ public PgDatabaseMetaData(PgConnection conn) {

private int nameDataLength = 0; // length for name datatype
private int indexMaxKeys = 0; // maximum number of keys in an index.
private int defaultTransactionIsolation = 0;

protected int getMaxIndexKeys() throws SQLException {
if (indexMaxKeys == 0) {
Expand Down Expand Up @@ -957,7 +959,40 @@ public int getMaxUserNameLength() throws SQLException {
}

public int getDefaultTransactionIsolation() throws SQLException {
return Connection.TRANSACTION_READ_COMMITTED;
if (defaultTransactionIsolation == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the current PG implementation caches default_transaction_isolation, and it requires client to-reconnect to observe the changed value. I believe it sounds like a bug, and it is not documented: https://www.postgresql.org/docs/current/runtime-config-client.html#GUC-DEFAULT-TRANSACTION-ISOLATION

At the same time, the javadoc for getDefaultTransactionIsolation says Retrieves this database's default transaction isolation level, so we should probably refrain from caching the isolation at the driver level unless we implement cache invalidation properly

String sql;
sql = "SELECT setting FROM pg_catalog.pg_settings WHERE name='default_transaction_isolation'";

try (Statement stmt = connection.createStatement()) {
try (ResultSet rs = stmt.executeQuery(sql)) {
String level = null;
if (rs.next()) {
level = rs.getString(1);
}
if (level == null) {
throw new PSQLException(
GT.tr(
"Unable to determine a value for DefaultTransactionIsolation due to missing "
+ "system catalog data."),
PSQLState.UNEXPECTED_ERROR);
}

level = level.toUpperCase(Locale.US);
if (level.equals("READ COMMITTED")) {
defaultTransactionIsolation = Connection.TRANSACTION_READ_COMMITTED;
} else if (level.equals("READ UNCOMMITTED")) {
defaultTransactionIsolation = Connection.TRANSACTION_READ_UNCOMMITTED;
} else if (level.equals("REPEATABLE READ")) {
defaultTransactionIsolation = Connection.TRANSACTION_REPEATABLE_READ;
} else if (level.equals("SERIALIZABLE")) {
defaultTransactionIsolation = Connection.TRANSACTION_SERIALIZABLE;
} else {
defaultTransactionIsolation = Connection.TRANSACTION_READ_COMMITTED; // Best guess.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it does a buest guess here, it should do one above as well instead of throwing an exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree. The error is being thrown on a relatively straightforward query. If that fails, something has gone wrong.

}
}
}
}
return defaultTransactionIsolation;
}

public boolean supportsTransactions() throws SQLException {
Expand Down
35 changes: 35 additions & 0 deletions pgjdbc/src/test/java/org/postgresql/test/jdbc2/DatabaseMetaDataPropertiesTest.java
View file
Open in desktop
Expand Up @@ -147,6 +147,41 @@ public void testIdentifiers() throws SQLException {

}

@Test
public void testDefaultTransactionIsolation() throws SQLException {
DatabaseMetaData dbmd = con.getMetaData();
assertNotNull(dbmd);

int transactionIsolation = dbmd.getDefaultTransactionIsolation();
assertEquals(Connection.TRANSACTION_READ_COMMITTED, transactionIsolation);

String [] isolationLevels = {"\"read committed\"","\"read uncommitted\"", "\"repeatable read\"","serializable"};
try {
for (int i = 0; i < isolationLevels.length; i++) {
con.createStatement().execute("alter database test set default_transaction_isolation to " + isolationLevels[i]);
try (Connection con1 = TestUtil.openDB()) {
transactionIsolation = con1.getMetaData().getDefaultTransactionIsolation();
switch (i) {
case 0:
assertEquals(Connection.TRANSACTION_READ_COMMITTED, transactionIsolation);
break;
case 1:
assertEquals(Connection.TRANSACTION_READ_UNCOMMITTED, transactionIsolation);
break;
case 2:
assertEquals(Connection.TRANSACTION_REPEATABLE_READ, transactionIsolation);
break;
case 3:
assertEquals(Connection.TRANSACTION_SERIALIZABLE, transactionIsolation);
break;
Comment on lines +165 to +176
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically speaking, Java 8 supports String switch, so case "read committed": ... would be a little bit easier to read.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

}
}
}
} finally {
con.createStatement().execute("alter database test set default_transaction_isolation to DEFAULT");
}
}

@Test
public void testTables() throws SQLException {
DatabaseMetaData dbmd = con.getMetaData();
Expand Down