Set column name explicitely when using `current_database()` in queries by kneth · Pull Request #3526 · pgjdbc/pgjdbc

kneth · 2025-02-16T06:22:33Z

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests 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 crate/crate#17393.

davecramer · 2025-02-18T16:48:15Z

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

jankohlmann · 2025-02-18T18:33:49Z

@davecramer are you thinking about reverting #3390 ?

davecramer · 2025-02-18T18:45:41Z

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

vlsi · 2025-02-19T05:57:07Z

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?

vlsi · 2025-02-19T06:39:37Z

+  void getColumnsForSchema() throws Exception {
+    DatabaseMetaData dbmd = conn.getMetaData();
+
+    ResultSet rs = dbmd.getColumns(null, "%", "decimaltest", "%");
+    assertTrue(rs.next());
+    assertEquals("a", rs.getString("COLUMN_NAME"));
+    assertEquals(0, rs.getInt("DECIMAL_DIGITS"));


Can we add a verification for the current_database column name?

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/PgDatabaseMetaData.java#L1671

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

byte[] catalogName = /* rs.getBytes("current_database") */; // replace with a known literal ... tuple[0] = catalogName;

Yes, I had this in mind as well.

Latest commit is removing current_database() where possible and use either the argument or use the connection's catalog name.

davecramer · 2025-02-19T10:04:11Z

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 #3528 fixes all of the performance regressions then we can leave this one as is.

kneth · 2025-02-19T10:32:05Z

Thank you for taken the time to review my PR. I have expanded the test as outlined in #3526 (review)

vlsi · 2025-02-19T12:05:51Z

, 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?

davecramer · 2025-02-19T12:46:29Z

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

vlsi · 2025-02-21T08:18:56Z

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/expected/expressions.out#L91-L94

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().

vlsi · 2025-02-21T08:19:47Z


+  private byte[] getCatalogName(@Nullable String catalog) throws SQLException {
+    if (catalog == null) {
+      return connection.getCatalog().getBytes();


We should use connection's encoding here. Otherwise there's a risk the bytes will be decoded differently

Good point - I have added the encoded

davecramer · 2025-02-21T10:53:17Z

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/expected/expressions.out#L91-L94

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

kneth · 2025-02-27T15:49:52Z

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

kneth · 2025-04-23T11:06:19Z

@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?

davecramer · 2025-04-23T19:35:11Z

+    }
+    return catalog.getBytes(Charset.defaultCharset());
+  }
+


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.

There are many checks like the following in the code:

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.

In retrospect I should have removed all of those in #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.

I'll take a look at it!

@davecramer @vlsi I gave it a shot, and I took the liberty to modify the condition as suggested in #3588 (comment)

davecramer · 2025-04-23T19:35:52Z

There's still a considerable amount of whitespace changes in this PR. Mostly around ( and beginning lines.

kneth · 2025-04-24T11:45:43Z

@davecramer says

[...] whitespace changes [...]

I have restored the white spaces

davecramer · 2025-04-24T17:25:23Z

Yes, checkstyle seems a little specific at times, but one more fix and we should be good to go

kneth · 2025-04-25T11:52:34Z

@davecramer

one more fix

... and done 😄

kneth · 2025-04-25T13:37:59Z

Thank you for merging

davecramer · 2025-04-25T13:39:04Z

Thank you for merging

Thank you for persisting through this process

kneth mentioned this pull request Feb 18, 2025

Improve compatibility with non-Postgres databases #3527

Closed

vlsi reviewed Feb 19, 2025

View reviewed changes

amotl mentioned this pull request Feb 19, 2025

JDBC: Update to pgjdbc 42.7.7 crate/crate-qa#333

Merged

vlsi reviewed Feb 21, 2025

View reviewed changes

kneth added 3 commits April 22, 2025 16:18

Set column name explicitely when using current_database() in queries

2d9b030

Check that current database is correctly reported

27218eb

factor out catalog name where possible

3b15692

kneth force-pushed the kneth/set-column-name-explicitely branch from 2fc2e79 to ab8e2fb Compare April 22, 2025 14:34

use connection's encoding

de7a693

kneth force-pushed the kneth/set-column-name-explicitely branch from ab8e2fb to de7a693 Compare April 22, 2025 14:49

kneth added 3 commits April 22, 2025 16:53

make style happy

08307c7

styling

49d0818

restore code formatting

4766e04

kneth force-pushed the kneth/set-column-name-explicitely branch from 798d192 to 4766e04 Compare April 23, 2025 09:47

kneth added 2 commits April 23, 2025 12:00

smaller improvements

501e743

small change to code format

ceeca4d

kneth force-pushed the kneth/set-column-name-explicitely branch from 88373e1 to ceeca4d Compare April 23, 2025 11:04

davecramer reviewed Apr 23, 2025

View reviewed changes

kneth added 2 commits April 24, 2025 11:22

Restore whitespaces

c5c3bdb

Don't silently ignore exceptions

8e6f12c

kneth force-pushed the kneth/set-column-name-explicitely branch from 7764e57 to 8e6f12c Compare April 24, 2025 11:25

Simplify boolean expression; drop check for null when not needed

5f5eb0f

kneth force-pushed the kneth/set-column-name-explicitely branch from 40414fe to 5f5eb0f Compare April 24, 2025 15:50

Making style checkers happy

532c217

davecramer merged commit 1e0c88d into pgjdbc:master Apr 25, 2025
15 of 17 checks passed

Conversation

kneth commented Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Uh oh!

davecramer commented Feb 18, 2025

Uh oh!

jankohlmann commented Feb 18, 2025

Uh oh!

davecramer commented Feb 18, 2025

Uh oh!

vlsi commented Feb 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davecramer commented Feb 19, 2025

Uh oh!

kneth commented Feb 19, 2025

Uh oh!

vlsi commented Feb 19, 2025

Uh oh!

davecramer commented Feb 19, 2025

Uh oh!

vlsi commented Feb 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davecramer commented Feb 21, 2025

Uh oh!

kneth commented Feb 27, 2025

Uh oh!

kneth commented Apr 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davecramer commented Apr 23, 2025

Uh oh!

kneth commented Apr 24, 2025

Uh oh!

davecramer commented Apr 24, 2025

Uh oh!

kneth commented Apr 25, 2025

Uh oh!

Uh oh!

kneth commented Apr 25, 2025

Uh oh!

davecramer commented Apr 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kneth commented Feb 16, 2025 •

edited

Loading