fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630 by davecramer · Pull Request #3631 · pgjdbc/pgjdbc

davecramer · 2025-05-12T20:35:03Z

No description provided.

vlsi · 2025-05-13T08:55:41Z

Can we somehow test the change?

davecramer · 2025-05-13T09:30:57Z

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

vlsi · 2025-05-13T12:10:58Z

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:

    /**
     * ...
     * @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 google/error-prone#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.

vlsi · 2025-05-13T12:15:52Z

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.

vlsi · 2025-05-13T14:48:52Z

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.

vlsi · 2025-05-16T11:50:45Z

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

vlsi · 2025-06-02T08:29:59Z

The PR did not change much though as QueryExecutorImpl#updateQueryMode resets AS_SINGLE flag anyway:

pgjdbc/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java

Lines 317 to 322 in 0a88ea4

    
           private int updateQueryMode(int flags) { 
        
             switch (getPreferQueryMode()) { 
        
               case SIMPLE: 
        
                 return flags | QUERY_EXECUTE_AS_SIMPLE; 
        
               case EXTENDED: 
        
                 return flags & ~QUERY_EXECUTE_AS_SIMPLE;

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)

vlsi · 2025-06-02T08:33:58Z

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

…ents deallocate Previously, the code for .isValid() was using executeWithFlags("", QueryExecutor.QUERY_EXECUTE_AS_SIMPLE) However, the execution still went with extended query protocol since QueryExecutorImpl#updateQueryMode trimmed the flag in preferQueryMode=extended (default) mode. It does not feel right to trim the flag there since the intention of the option was to "prefer" query mode unless specified explicitly. The case is identified and tested in AutoRollbackTest: isValid should return true after statements like DEALLOCATE, DISCARD, and so on. See pgjdbc#3631

davecramer · 2025-06-02T10:37:56Z

I ran that test and it did not fail locally ?

…ents deallocate Previously, the code for .isValid() was using executeWithFlags("", QueryExecutor.QUERY_EXECUTE_AS_SIMPLE) However, the execution still went with extended query protocol since QueryExecutorImpl#updateQueryMode trimmed the flag in preferQueryMode=extended (default) mode. It does not feel right to trim the flag there since the intention of the option was to "prefer" query mode unless specified explicitly. The case is identified and tested in AutoRollbackTest: isValid should return true after statements like DEALLOCATE, DISCARD, and so on. See pgjdbc#3631

vlsi · 2025-06-02T10:47:53Z

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

davecramer · 2025-06-02T10:51:12Z

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 ?

vlsi · 2025-06-02T11:42:30Z

The full sequence is as follows:

Rename the test to AutoRollbackTest
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.

davecramer · 2025-06-02T21:28:58Z

I guess I need your PR to test this

vlsi · 2025-06-03T05:45:50Z

AutoRollbackTest reproduces with the base code.
You could set a breakpoint in

pgjdbc/pgjdbc/src/test/java/org/postgresql/test/jdbc2/AutoRollbackTestSuite.java

Lines 313 to 315 in 10e3546

    
           Assert.assertFalse("Connection.isValid should return false since failMode=" + failMode 
        
               + ", flushCacheOnDeallocate=false, and autosave=NEVER", 
        
               con.isValid(4));

, and there you'll see that .isValid(4) executes an extended query.

An alternative option is to add e.printStackTrace() here:

pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java

Line 1568 in 10e3546

LOGGER.log(Level.FINE, GT.tr("Validating connection."), e);

so you could see there are "prepared statement does not exist" errors coming from .isValid.

…ents deallocate Previously, the code for .isValid() was using executeWithFlags("", QueryExecutor.QUERY_EXECUTE_AS_SIMPLE) However, the execution still went with extended query protocol since QueryExecutorImpl#updateQueryMode trimmed the flag in preferQueryMode=extended (default) mode. It does not feel right to trim the flag there since the intention of the option was to "prefer" query mode unless specified explicitly. The case is identified and tested in AutoRollbackTest: isValid should return true after statements like DEALLOCATE, DISCARD, and so on. See pgjdbc#3631

…ents deallocate Previously, the code for .isValid() was using executeWithFlags("", QueryExecutor.QUERY_EXECUTE_AS_SIMPLE) However, the execution still went with extended query protocol since QueryExecutorImpl#updateQueryMode trimmed the flag in preferQueryMode=extended (default) mode. It does not feel right to trim the flag there since the intention of the option was to "prefer" query mode unless specified explicitly. The case is identified and tested in AutoRollbackTest: isValid should return true after statements like DEALLOCATE, DISCARD, and so on. See pgjdbc#3631 Fixes pgjdbc#3630

…ents deallocate Previously, the code for .isValid() was using executeWithFlags("", QueryExecutor.QUERY_EXECUTE_AS_SIMPLE) However, the execution still went with extended query protocol since QueryExecutorImpl#updateQueryMode trimmed the flag in preferQueryMode=extended (default) mode. It does not feel right to trim the flag there since the intention of the option was to "prefer" query mode unless specified explicitly. The case is identified and tested in AutoRollbackTest: isValid should return true after statements like DEALLOCATE, DISCARD, and so on. See #3631 Fixes #3630

vlsi mentioned this pull request May 13, 2025

Rule request: redundant type cast google/error-prone#3684

Open

davecramer changed the title ~~use executeWithFlags fixes Issue #3630~~ fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630 May 16, 2025

fix isValid to correctly use executeWithFlags fixes Issue pgjdbc#3630

bfaa669

davecramer force-pushed the fix_validate branch from 0fa551c to bfaa669 Compare May 16, 2025 18:20

davecramer merged commit 2de9b94 into pgjdbc:master May 19, 2025
19 checks passed

vlsi mentioned this pull request Jun 6, 2025

fix: ensure Connection.isValid() returns true even if prepared statements deallocate #3655

Merged

Conversation

davecramer commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlsi commented May 13, 2025

Uh oh!

davecramer commented May 13, 2025

Uh oh!

vlsi commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlsi commented May 13, 2025

Uh oh!

vlsi commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlsi commented May 16, 2025

Uh oh!

Uh oh!

vlsi commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlsi commented Jun 2, 2025

Uh oh!

davecramer commented Jun 2, 2025

Uh oh!

vlsi commented Jun 2, 2025

Uh oh!

davecramer commented Jun 2, 2025

Uh oh!

vlsi commented Jun 2, 2025

Uh oh!

davecramer commented Jun 2, 2025

Uh oh!

vlsi commented Jun 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

davecramer commented May 12, 2025 •

edited

Loading

vlsi commented May 13, 2025 •

edited

Loading

vlsi commented May 13, 2025 •

edited

Loading

vlsi commented Jun 2, 2025 •

edited

Loading