Subject: Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 13 May 2025 08:55:41 +0000

Can we somehow test the change?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 13 May 2025 09:30:57 +0000

What's bothersome to me is that the compiler can't detect the problem either. I'll think about how to test the change

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 13 May 2025 12:10:58 +0000

How about adding a check to `execute(String sql, int autoGeneratedKeys)` so it rejects unknown flags at least during pgjdbc test execution?

Here's the current code:

```java
    /**
     * ...
     * @param autoGeneratedKeys a flag indicating whether auto-generated keys
     *        should be made available for retrieval;
     *         one of the following constants:
     *         {@code Statement.RETURN_GENERATED_KEYS}
     *         {@code Statement.NO_GENERATED_KEYS}
     */
  @Override
  public boolean execute(String sql, int autoGeneratedKeys) throws SQLException {
    if (autoGeneratedKeys == Statement.NO_GENERATED_KEYS) {
      return execute(sql);
    }
    return execute(sql, (String[]) null);
  }
```

WDYT of adding a check there so it ensures `autoGeneratedKeys` must be either `RETURN_GENERATED_KEYS` or `NO_GENERATED_KEYS`?
Of course it is not backward-compatible, so it should be under `pgjdbc.asserts.statements=true` flag so we could enable it for our tests and the users could enable it to detect wrong API usage in their systems.

----

The test could be located in error-prone (see https://github.com/google/error-prone/issues/3684), or in `javac` itself. I guess it could be a valid `javac` warning if the code casts the object, and then calls the method which was avaliable without a cast.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 13 May 2025 12:15:52 +0000

For the reference, I've executed "remove redundant cast(s)" inspection in IDEA, and it does not reveal the other problems like this one. There are 13 extra redundant casts, however, they are harmless.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 13 May 2025 14:48:52 +0000

`javac` does have `-Xlint:cast` to detect redundant casts, however, it does not understand the cast in `((PgStatement)checkConnectionQuery).execute("", QueryExecutor.QUERY_EXECUTE_AS_SIMPLE);` is redundant.
I will file an enhancement request to `javac`.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Fri, 16 May 2025 11:50:45 +0000

Could you update the PR title (and the commit message) so it conveys the change to the end-users?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Mon, 02 Jun 2025 08:29:59 +0000

The PR did not change much though as `QueryExecutorImpl#updateQueryMode` resets `AS_SINGLE` flag anyway: https://github.com/pgjdbc/pgjdbc/blob/0a88ea425e86dce691a96d6aa7023c20ac887b98/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java#L317-L322

We'd better write explicit tests rather than rely on the code change alone.

So it still ends up with the unwanted error when calling `isValid`:

```
org.postgresql.util.PSQLException: ERROR: prepared statement "S_3" does not exist
	at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2736)
	at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2423)
	at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:374)
	at org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:518)
	at org.postgresql.jdbc.PgStatement.execute(PgStatement.java:435)
	at org.postgresql.jdbc.PgStatement.executeWithFlags(PgStatement.java:357)
	at org.postgresql.jdbc.PgStatement.executeCachedSql(PgStatement.java:342)
	at org.postgresql.jdbc.PgStatement.executeWithFlags(PgStatement.java:318)
	at org.postgresql.jdbc.PgConnection.isValid(PgConnection.java:1554)
	at org.postgresql.test.jdbc2.AutoRollbackTest.run(AutoRollbackTest.java:314)
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Mon, 02 Jun 2025 08:33:58 +0000

I think `updateQueryMode` should not trim `flags & ~QUERY_EXECUTE_AS_SIMPLE` when running `preferQueryMode=extended`.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 02 Jun 2025 10:37:56 +0000

I ran that test and it did not fail locally ?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Mon, 02 Jun 2025 10:47:53 +0000

The test did not fail because it was not named properly I believe it was just ignored.
Have you tried renaming the test after `AutoRollbackTest`?

I discovered the failure when working on https://github.com/pgjdbc/pgjdbc/pull/3652

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 02 Jun 2025 10:51:12 +0000

> The test did not fail because it was not named properly I believe it was just ignored. Have you tried renaming the test after `AutoRollbackTest`?
> 
> I discovered the failure when working on #3652

Yes, I changed the name and ran the test by itself. I wonder if it makes a difference if I run all of the tests ?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Mon, 02 Jun 2025 11:42:30 +0000

The full sequence is as follows:
1) Rename the test to `AutoRollbackTest`
2) Add `preferQueryMode=extendedForPrepared`

Then the test fails:

```
java.lang.AssertionError: Connection.isValid should return false since failMode=DEALLOCATE, flushCacheOnDeallocate=false, and autosave=NEVER
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertFalse(Assert.java:65)
	at org.postgresql.test.jdbc2.AutoRollbackTest.run(AutoRollbackTest.java:315)
```

As I analyzed the failure, I put `e.printStackTrace()` into `isValid` to check the actual errors, and I noticed it attempts using prepared statements.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Mon, 02 Jun 2025 21:28:58 +0000

I guess I need your PR to test this

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 03 Jun 2025 05:45:50 +0000

`AutoRollbackTest` reproduces with the base code.
You could set a breakpoint in https://github.com/pgjdbc/pgjdbc/blob/10e3546750888767191df90f188651306b3bafa7/pgjdbc/src/test/java/org/postgresql/test/jdbc2/AutoRollbackTestSuite.java#L313-L315, and there you'll see that `.isValid(4)` executes an **extended** query.

An alternative option is to add `e.printStackTrace()` here: https://github.com/pgjdbc/pgjdbc/blob/10e3546750888767191df90f188651306b3bafa7/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java#L1568 so you could see there are "prepared statement does not exist" errors coming from `.isValid`.