pgjdbc/pgjdbc GitHub issues and pull requests (mirror)
help / color / mirror / Atom feed[pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
15+ messages / 2 participants
[nested] [flat]
* [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-05-12 20:35 "davecramer (@davecramer)" <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: davecramer (@davecramer) @ 2025-05-12 20:35 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-05-13 08:55 ` "vlsi (@vlsi)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: vlsi (@vlsi) @ 2025-05-13 08:55 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Can we somehow test the change?
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-05-13 09:30 ` "davecramer (@davecramer)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: davecramer (@davecramer) @ 2025-05-13 09:30 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
What's bothersome to me is that the compiler can't detect the problem either. I'll think about how to test the change
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-05-13 12:10 ` "vlsi (@vlsi)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: vlsi (@vlsi) @ 2025-05-13 12:10 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
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.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-05-13 12:15 ` "vlsi (@vlsi)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: vlsi (@vlsi) @ 2025-05-13 12:15 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
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.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-05-13 14:48 ` "vlsi (@vlsi)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: vlsi (@vlsi) @ 2025-05-13 14:48 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
`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`.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-05-16 11:50 ` "vlsi (@vlsi)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: vlsi (@vlsi) @ 2025-05-16 11:50 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Could you update the PR title (and the commit message) so it conveys the change to the end-users?
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-06-02 08:29 ` "vlsi (@vlsi)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: vlsi (@vlsi) @ 2025-06-02 08:29 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
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/...
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)
```
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-06-02 08:33 ` "vlsi (@vlsi)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: vlsi (@vlsi) @ 2025-06-02 08:33 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
I think `updateQueryMode` should not trim `flags & ~QUERY_EXECUTE_AS_SIMPLE` when running `preferQueryMode=extended`.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-06-02 10:37 ` "davecramer (@davecramer)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: davecramer (@davecramer) @ 2025-06-02 10:37 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
I ran that test and it did not fail locally ?
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-06-02 10:47 ` "vlsi (@vlsi)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: vlsi (@vlsi) @ 2025-06-02 10:47 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
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
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-06-02 10:51 ` "davecramer (@davecramer)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: davecramer (@davecramer) @ 2025-06-02 10:51 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
> 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 ?
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-06-02 11:42 ` "vlsi (@vlsi)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: vlsi (@vlsi) @ 2025-06-02 11:42 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
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.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-06-02 21:28 ` "davecramer (@davecramer)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: davecramer (@davecramer) @ 2025-06-02 21:28 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
I guess I need your PR to test this
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630
@ 2025-06-03 05:45 ` "vlsi (@vlsi)" <[email protected]>
13 siblings, 0 replies; 15+ messages in thread
From: vlsi (@vlsi) @ 2025-06-03 05:45 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
`AutoRollbackTest` reproduces with the base code.
You could set a breakpoint in https://github.com/pgjdbc/pgjdbc/blob/10e3546750888767191df90f188651306b3bafa7/pgjdbc/src/test/java/..., 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/... so you could see there are "prepared statement does not exist" errors coming from `.isValid`.
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2025-06-03 05:45 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-05-12 20:35 [pgjdbc/pgjdbc] PR #3631: fix: isValid incorrectly called execute, instead of executeWithFlags fixes Issue #3630 "davecramer (@davecramer)" <[email protected]>
2025-05-13 08:55 ` "vlsi (@vlsi)" <[email protected]>
2025-05-13 09:30 ` "davecramer (@davecramer)" <[email protected]>
2025-05-13 12:10 ` "vlsi (@vlsi)" <[email protected]>
2025-05-13 12:15 ` "vlsi (@vlsi)" <[email protected]>
2025-05-13 14:48 ` "vlsi (@vlsi)" <[email protected]>
2025-05-16 11:50 ` "vlsi (@vlsi)" <[email protected]>
2025-06-02 08:29 ` "vlsi (@vlsi)" <[email protected]>
2025-06-02 08:33 ` "vlsi (@vlsi)" <[email protected]>
2025-06-02 10:37 ` "davecramer (@davecramer)" <[email protected]>
2025-06-02 10:47 ` "vlsi (@vlsi)" <[email protected]>
2025-06-02 10:51 ` "davecramer (@davecramer)" <[email protected]>
2025-06-02 11:42 ` "vlsi (@vlsi)" <[email protected]>
2025-06-02 21:28 ` "davecramer (@davecramer)" <[email protected]>
2025-06-03 05:45 ` "vlsi (@vlsi)" <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox