Return correct default from PgDatabaseMetaData.getDefaultTransactionIsolation

[nvanbenschoten]

Fixes: #2991

This commit updates PgDatabaseMetaData.getDefaultTransactionIsolation to query the database for the default transaction isolation level, instead of assuming that the default isolation level is READ COMMITTED.

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Does your submission pass tests?
2. Does ./gradlew styleCheck pass ?

Changes to Existing Features:

• Does this break existing behaviour? If so please explain.
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?
• Have you successfully run tests with your changes locally?

[davecramer]

First off thanks for the PR!
We have been slowly migrating code over to using try with resources it would be great if getDefaultTransactionIsolation() used them
Also the test doesn't actually test the database something like

 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()) {
          int 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;
          }
        }
      }
    } finally {
      con.createStatement().execute("alter database test set default_transaction_isolation to DEFAULT");
    }

would

[nvanbenschoten]

We have been slowly migrating code over to using try with resources it would be great if getDefaultTransactionIsolation() used them

Done. Much cleaner!

Also the test doesn't actually test the database something like ... would

Good idea. Added that in and confirmed that it passes the tests.

[davecramer]

What does this have to do with what we are currently testing ?

[nvanbenschoten]

Just that it's related to transaction support and wasn't already tested. testCapabilities felt like the right place for it, but I removed it.

[davecramer]

I'd prefer this was in it's own test. We really don't need it, but if we have to have it, can we create a different test for it?

[nvanbenschoten]

I remove it and renamed the test to reflect the smaller scope.

[davecramer]

/home/runner/work/pgjdbc/pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java:980: error: [dereference.of.nullable] dereference of possibly-null reference level
level = level.toUpperCase(Locale.US);

[nvanbenschoten]

/home/runner/work/pgjdbc/pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java:980: error: [dereference.of.nullable] dereference of possibly-null reference level
level = level.toUpperCase(Locale.US);

Fixed.

[vlsi]

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

[davecramer]

agreed

[vlsi]

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

[ecki]

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

[davecramer]

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