NotificationsYou must be signed in to change notification settings
Fork904
Star1.6k

fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630#3631

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

davecramer merged 1 commit intopgjdbc:masterfromdavecramer:fix_validate

May 19, 2025

Merged

fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630#3631

davecramer merged 1 commit intopgjdbc:masterfromdavecramer:fix_validate

May 19, 2025

Conversation

Copy link

Member

davecramer commentedMay 12, 2025•
edited
Loading

No description provided.

Copy link

Member

vlsi commentedMay 13, 2025

Can we somehow test the change?

Copy link

MemberAuthor

davecramer commentedMay 13, 2025

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 mentioned this pull request

May 13, 2025

Rule request: redundant type castgoogle/error-prone#3684

Open

Copy link

Member

vlsi commentedMay 13, 2025•
edited
Loading

How about adding a check toexecute(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}     */@Overridepublicbooleanexecute(Stringsql,intautoGeneratedKeys)throwsSQLException {if (autoGeneratedKeys ==Statement.NO_GENERATED_KEYS) {returnexecute(sql);    }returnexecute(sql, (String[])null);  }

WDYT of adding a check there so it ensuresautoGeneratedKeys must be eitherRETURN_GENERATED_KEYS orNO_GENERATED_KEYS?
Of course it is not backward-compatible, so it should be underpgjdbc.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 (seegoogle/error-prone#3684), or injavac itself. I guess it could be a validjavac warning if the code casts the object, and then calls the method which was avaliable without a cast.

Copy link

Member

vlsi commentedMay 13, 2025

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.

Copy link

Member

vlsi commentedMay 13, 2025•
edited
Loading

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

Copy link

Member

vlsi commentedMay 16, 2025

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

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 Issuepgjdbc#3630

bfaa669

davecramer force-pushed thefix_validate branch from0fa551c tobfaa669Compare

May 16, 2025 18:20

davecramer merged commit2de9b94 intopgjdbc:master

May 19, 2025

19 checks passed

Copy link

Member

vlsi commentedJun 2, 2025•
edited
Loading

The PR did not change much though asQueryExecutorImpl#updateQueryMode resetsAS_SINGLE flag anyway:

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

Lines 317 to 322 in0a88ea4

	privateintupdateQueryMode(intflags) {
	switch (getPreferQueryMode()) {
	caseSIMPLE:
	returnflags \|QUERY_EXECUTE_AS_SIMPLE;
	caseEXTENDED:
	returnflags & ~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 callingisValid:

org.postgresql.util.PSQLException: ERROR: prepared statement "S_3" does not existat 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)

Copy link

Member

vlsi commentedJun 2, 2025

I thinkupdateQueryMode should not trimflags & ~QUERY_EXECUTE_AS_SIMPLE when runningpreferQueryMode=extended.

vlsi added a commit to vlsi/pgjdbc that referenced this pull request

Jun 2, 2025

fix: ensure Connection.isValid() returns true even if prepared statem…

c1e2552

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

Copy link

MemberAuthor

davecramer commentedJun 2, 2025

I ran that test and it did not fail locally ?

vlsi added a commit to vlsi/pgjdbc that referenced this pull request

Jun 2, 2025

fix: ensure Connection.isValid() returns true even if prepared statem…

7d62428

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

Copy link

Member

vlsi commentedJun 2, 2025

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

I discovered the failure when working on#3652

Copy link

MemberAuthor

davecramer commentedJun 2, 2025

The test did not fail because it was not named properly I believe it was just ignored. Have you tried renaming the test afterAutoRollbackTest?
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 ?

Copy link

Member

vlsi commentedJun 2, 2025

The full sequence is as follows:

Rename the test toAutoRollbackTest
AddpreferQueryMode=extendedForPrepared

Then the test fails:

java.lang.AssertionError: Connection.isValid should return false since failMode=DEALLOCATE, flushCacheOnDeallocate=false, and autosave=NEVERat 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 pute.printStackTrace() intoisValid to check the actual errors, and I noticed it attempts using prepared statements.

Copy link

MemberAuthor

davecramer commentedJun 2, 2025

I guess I need your PR to test this

Copy link

Member

vlsi commentedJun 3, 2025

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 in10e3546

	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 anextended query.

An alternative option is to adde.printStackTrace() here:

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

Line 1568 in10e3546

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

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

vlsi added a commit to vlsi/pgjdbc that referenced this pull request

Jun 4, 2025

fix: ensure Connection.isValid() returns true even if prepared statem…

07fcbb7

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

vlsi added a commit to vlsi/pgjdbc that referenced this pull request

Jun 6, 2025

fix: ensure Connection.isValid() returns true even if prepared statem…

0996ffc

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

vlsi mentioned this pull request

Jun 6, 2025

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

Merged

vlsi added a commit that referenced this pull request

Jun 6, 2025

fix: ensure Connection.isValid() returns true even if prepared statem…

cb10dce

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

Labels

None yet

2 participants

Movatterモバイル変換

fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630#3631

fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630#3631

Uh oh!

Conversation

davecramer commentedMay 12, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

vlsi commentedMay 13, 2025

Uh oh!

davecramer commentedMay 13, 2025

Uh oh!

vlsi commentedMay 13, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

vlsi commentedMay 13, 2025

Uh oh!

vlsi commentedMay 13, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

vlsi commentedMay 16, 2025

Uh oh!

Uh oh!

vlsi commentedJun 2, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

vlsi commentedJun 2, 2025

Uh oh!

davecramer commentedJun 2, 2025

Uh oh!

vlsi commentedJun 2, 2025

Uh oh!

davecramer commentedJun 2, 2025

Uh oh!

vlsi commentedJun 2, 2025

Uh oh!

davecramer commentedJun 2, 2025

Uh oh!

vlsi commentedJun 3, 2025

Uh oh!

Uh oh!

davecramer commentedMay 12, 2025•
edited
Loading

vlsi commentedMay 13, 2025•
edited
Loading

vlsi commentedMay 13, 2025•
edited
Loading

vlsi commentedJun 2, 2025•
edited
Loading