pgjdbc/pgjdbc GitHub issues and pull requests (mirror)
help / color / mirror / Atom feed[pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
31+ messages / 4 participants
[nested] [flat]
* [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-16 06:22 "kneth (@kneth)" <[email protected]>
0 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-02-16 06:22 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
### All Submissions:
* [x] Have you followed the guidelines in our [Contributing](https://github.com/pgjdbc/pgjdbc/blob/master/CONTRIBUTING.md) document?
* [x] Have you checked to ensure there aren't other open [Pull Requests](../../pulls) for the same update/change?
Postgres compatible databases like https://github.com/crate/crate can use PG JDBC as driver. Often the PG JDBC driver is used by a tool or integration (JetBrains DataGrip to name one). As CrateDB isn't fully PG compatible, this patch will make life a bit easier for our users. The issue has been reported as https://github.com/crate/crate/issues/17393.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-18 16:48 "davecramer (@davecramer)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: davecramer (@davecramer) @ 2025-02-18 16:48 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
I think I'd rather change all of the code referring to current_database to not use SQL at all. The connection knows the database. I don't thin it is necessary to use this function
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-18 18:33 "jankohlmann (@jankohlmann)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: jankohlmann (@jankohlmann) @ 2025-02-18 18:33 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@davecramer are you thinking about reverting https://github.com/pgjdbc/pgjdbc/pull/3390 ?
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-18 18:45 "davecramer (@davecramer)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: davecramer (@davecramer) @ 2025-02-18 18:45 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Probably not reverting it but changing the way it is done.
In the one you are changing you could simply do `select 'db' as current_database` where `db` is the database in the connection URL, or whatever is returned in the startup parameters
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-19 05:57 "vlsi (@vlsi)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: vlsi (@vlsi) @ 2025-02-19 05:57 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Frankly, I see no issues leaving it as `current_database() as current_database`. I would probably prefer this one.
It would make query easier to understand and test. It would naturally avoid "concatenate string sql on every execution" as well.
WDYT?
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-19 06:39 "vlsi (@vlsi)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: vlsi (@vlsi) @ 2025-02-19 06:39 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/test/java/org/postgresql/test/jdbc42/DatabaseMetaDataTest.java)
Can we add a verification for the `current_database` column name?
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-19 10:04 "davecramer (@davecramer)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: davecramer (@davecramer) @ 2025-02-19 10:04 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
> Frankly, I see no issues leaving it as `current_database() as current_database`. I would probably prefer this one.
>
> It would make query easier to understand and test. It would naturally avoid "concatenate string sql on every execution" as well. WDYT?
If https://github.com/pgjdbc/pgjdbc/pull/3528 fixes all of the performance regressions then we can leave this one as is.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-19 10:21 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-02-19 10:21 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/test/java/org/postgresql/test/jdbc42/DatabaseMetaDataTest.java)
I have added it to the test. Notice that `getColumns` reports the current database in the `TABLE_CAT` column: https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMeta...
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-19 10:32 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-02-19 10:32 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Thank you for taken the time to review my PR. I have expanded the test as outlined in https://github.com/pgjdbc/pgjdbc/pull/3526#pullrequestreview-2625757302
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-19 11:58 "vlsi (@vlsi)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: vlsi (@vlsi) @ 2025-02-19 11:58 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/test/java/org/postgresql/test/jdbc42/DatabaseMetaDataTest.java)
I thought `current_database` was returned via client=facing APIs, however, now I see it is used only in the internals.
However, it looks like the specific usage could be completely removed from SQL and we could use something like
```java
byte[] catalogName = /* rs.getBytes("current_database") */; // replace with a known literal
...
tuple[0] = catalogName;
```
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-19 12:05 "vlsi (@vlsi)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: vlsi (@vlsi) @ 2025-02-19 12:05 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
>, or whatever is returned in the startup parameters
It looks like the startup parameters do not return the database name.
At the same time, there's `current_catalog` function and `information_schema.information_schema_catalog_name` table returning `catalog_name` which is **different** from `current_database`.
So it looks like we should use those functions rather than `current_database()` if we want to treat catalogs somehow.
Is there a reason to ignore `current_catalog` PostgreSQL function?
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-19 12:45 "davecramer (@davecramer)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: davecramer (@davecramer) @ 2025-02-19 12:45 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/test/java/org/postgresql/test/jdbc42/DatabaseMetaDataTest.java)
Yes, I had this in mind as well.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-19 12:46 "davecramer (@davecramer)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: davecramer (@davecramer) @ 2025-02-19 12:46 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
> > , or whatever is returned in the startup parameters
>
> It looks like the startup parameters do not return the database name.
Interesting, somehow I though it was.
>
> At the same time, there's `current_catalog` function and `information_schema.information_schema_catalog_name` table returning `catalog_name` which is **different** from `current_database`.
>
> So it looks like we should use those functions rather than `current_database()` if we want to treat catalogs somehow. Is there a reason to ignore `current_catalog` PostgreSQL function?
Only if it creates a performance regression.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-20 12:00 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-02-20 12:00 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/test/java/org/postgresql/test/jdbc42/DatabaseMetaDataTest.java)
Latest commit is removing `current_database()` where possible and use either the argument or use the connection's catalog name.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-21 08:18 "vlsi (@vlsi)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: vlsi (@vlsi) @ 2025-02-21 08:18 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Ok, PostgreSQL has a regression test that ensures `current_database()` should return the same value as `current_catalog`: https://github.com/postgres/postgres/blob/7d6d2c4bbd730bd9af191d46d4fb01d5f5c30cf1/src/test/regress/...
At the same time, it looks like `current_catalog` was added for the SQL standard compatibility somewhere around 8.4.
So it is fine to keep using `current_database()`.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-21 08:19 "vlsi (@vlsi)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: vlsi (@vlsi) @ 2025-02-21 08:19 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java)
We should use connection's encoding here. Otherwise there's a risk the bytes will be decoded differently
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-21 10:53 "davecramer (@davecramer)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: davecramer (@davecramer) @ 2025-02-21 10:53 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
> Ok, PostgreSQL has a regression test that ensures `current_database()` should return the same value as `current_catalog`: https://github.com/postgres/postgres/blob/7d6d2c4bbd730bd9af191d46d4fb01d5f5c30cf1/src/test/regress/...
>
> At the same time, it looks like `current_catalog` was added for the SQL standard compatibility somewhere around 8.4.
>
> So it is fine to keep using `current_database()`.
Assuming no performance regression
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-21 12:51 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-02-21 12:51 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java)
Good point - I have added the encoded
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-02-27 15:49 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-02-27 15:49 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@davecramer @vlsi Do you need any changes before you can merge my PR?
When merging, you should probably merge #3528 too so the performance is improved.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-23 11:06 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-04-23 11:06 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@davecramer @vlsi I have rebased my branch on `master`, and tests pass locally (`gradlew test`). Is it possible to advance the PR to a state where it can be merged?
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-23 19:35 "davecramer (@davecramer)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: davecramer (@davecramer) @ 2025-04-23 19:35 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java)
From what I can tell we check to see if the catalog is null before calling this function.
Moot point: If we get a SQLException we should just throw it.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-23 19:35 "davecramer (@davecramer)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: davecramer (@davecramer) @ 2025-04-23 19:35 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
There's still a considerable amount of whitespace changes in this PR. Mostly around ( and beginning lines.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-24 10:58 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-04-24 10:58 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java)
There are many checks like the following in the code:
```java
if (catalog != null) {
sql += " AND current_database() = " + escapeQuotes(catalog);
}
```
They will not exclude `null`. Likewise, the additional checks in #3588 don't exclude `catalog` to be `null` afaik.
Removing the `try`/`catch` makes sense as errors can bubble up to the application.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-24 11:45 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-04-24 11:45 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@davecramer says
> [...] whitespace changes [...]
I have restored the white spaces
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-24 12:18 "davecramer (@davecramer)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: davecramer (@davecramer) @ 2025-04-24 12:18 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java)
In retrospect I should have removed all of those in https://github.com/pgjdbc/pgjdbc/pull/3588. By the time we get to the query we know that if the catalog is not null then it is the same as `current_database()` If you want to make those changes I'd appreciate it. Otherwise I'll make them which might make your's harder to rebase.
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-24 13:00 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-04-24 13:00 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java)
I'll take a look at it!
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-24 15:54 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-04-24 15:54 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java)
@davecramer @vlsi I gave it a shot, and I took the liberty to modify the condition as suggested in https://github.com/pgjdbc/pgjdbc/pull/3588#discussion_r2041077994
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-24 17:25 "davecramer (@davecramer)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: davecramer (@davecramer) @ 2025-04-24 17:25 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Yes, checkstyle seems a little specific at times, but one more fix and we should be good to go
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-25 11:52 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-04-25 11:52 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@davecramer
> one more fix
... and done 😄
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-25 13:37 "kneth (@kneth)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: kneth (@kneth) @ 2025-04-25 13:37 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Thank you for merging
^ permalink raw reply [nested|flat] 31+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries
@ 2025-04-25 13:39 "davecramer (@davecramer)" <[email protected]>
29 siblings, 0 replies; 31+ messages in thread
From: davecramer (@davecramer) @ 2025-04-25 13:39 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
> Thank you for merging
Thank you for persisting through this process
^ permalink raw reply [nested|flat] 31+ messages in thread
end of thread, other threads:[~2025-04-25 13:39 UTC | newest]
Thread overview: 31+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-02-16 06:22 [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries "kneth (@kneth)" <[email protected]>
2025-02-18 16:48 ` "davecramer (@davecramer)" <[email protected]>
2025-02-18 18:33 ` "jankohlmann (@jankohlmann)" <[email protected]>
2025-02-18 18:45 ` "davecramer (@davecramer)" <[email protected]>
2025-02-19 05:57 ` "vlsi (@vlsi)" <[email protected]>
2025-02-19 06:39 ` "vlsi (@vlsi)" <[email protected]>
2025-02-19 10:04 ` "davecramer (@davecramer)" <[email protected]>
2025-02-19 10:21 ` "kneth (@kneth)" <[email protected]>
2025-02-19 10:32 ` "kneth (@kneth)" <[email protected]>
2025-02-19 11:58 ` "vlsi (@vlsi)" <[email protected]>
2025-02-19 12:05 ` "vlsi (@vlsi)" <[email protected]>
2025-02-19 12:45 ` "davecramer (@davecramer)" <[email protected]>
2025-02-19 12:46 ` "davecramer (@davecramer)" <[email protected]>
2025-02-20 12:00 ` "kneth (@kneth)" <[email protected]>
2025-02-21 08:18 ` "vlsi (@vlsi)" <[email protected]>
2025-02-21 08:19 ` "vlsi (@vlsi)" <[email protected]>
2025-02-21 10:53 ` "davecramer (@davecramer)" <[email protected]>
2025-02-21 12:51 ` "kneth (@kneth)" <[email protected]>
2025-02-27 15:49 ` "kneth (@kneth)" <[email protected]>
2025-04-23 11:06 ` "kneth (@kneth)" <[email protected]>
2025-04-23 19:35 ` "davecramer (@davecramer)" <[email protected]>
2025-04-23 19:35 ` "davecramer (@davecramer)" <[email protected]>
2025-04-24 10:58 ` "kneth (@kneth)" <[email protected]>
2025-04-24 11:45 ` "kneth (@kneth)" <[email protected]>
2025-04-24 12:18 ` "davecramer (@davecramer)" <[email protected]>
2025-04-24 13:00 ` "kneth (@kneth)" <[email protected]>
2025-04-24 15:54 ` "kneth (@kneth)" <[email protected]>
2025-04-24 17:25 ` "davecramer (@davecramer)" <[email protected]>
2025-04-25 11:52 ` "kneth (@kneth)" <[email protected]>
2025-04-25 13:37 ` "kneth (@kneth)" <[email protected]>
2025-04-25 13:39 ` "davecramer (@davecramer)" <[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