Subject: [pgjdbc/pgjdbc] PR #3526: Set column name explicitely when using `current_database()` in queries

From: "kneth (@kneth)" <[email-masked]>
Date: Sun, 16 Feb 2025 06:22:33 +0000

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

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 18 Feb 2025 16:48:15 +0000

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

---

From: "jankohlmann (@jankohlmann)" <[email-masked]>
Date: Tue, 18 Feb 2025 18:33:49 +0000

@davecramer are you thinking about reverting https://github.com/pgjdbc/pgjdbc/pull/3390 ?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 18 Feb 2025 18:45:41 +0000

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

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 19 Feb 2025 05:57:07 +0000

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?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 19 Feb 2025 06:39:37 +0000

(on pgjdbc/src/test/java/org/postgresql/test/jdbc42/DatabaseMetaDataTest.java)

Can we add a verification for the `current_database` column name?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 19 Feb 2025 10:04:11 +0000

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

---

From: "kneth (@kneth)" <[email-masked]>
Date: Wed, 19 Feb 2025 10:21:42 +0000

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

---

From: "kneth (@kneth)" <[email-masked]>
Date: Wed, 19 Feb 2025 10:32:05 +0000

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

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 19 Feb 2025 11:58:52 +0000

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;
```

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 19 Feb 2025 12:05:51 +0000

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

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 19 Feb 2025 12:45:40 +0000

Yes, I had this in mind as well.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 19 Feb 2025 12:46:29 +0000

> It looks like the startup parameters do not return the database name.

Interesting, somehow I though it was.
> 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.

---

From: "kneth (@kneth)" <[email-masked]>
Date: Thu, 20 Feb 2025 12:00:24 +0000

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

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Fri, 21 Feb 2025 08:18:56 +0000

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

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Fri, 21 Feb 2025 08:19:47 +0000

(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

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Fri, 21 Feb 2025 10:53:17 +0000

> So it is fine to keep using `current_database()`.

Assuming no performance regression

---

From: "kneth (@kneth)" <[email-masked]>
Date: Fri, 21 Feb 2025 12:51:05 +0000

Good point - I have added the encoded

---

From: "kneth (@kneth)" <[email-masked]>
Date: Thu, 27 Feb 2025 15:49:52 +0000

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

---

From: "kneth (@kneth)" <[email-masked]>
Date: Wed, 23 Apr 2025 11:06:19 +0000

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

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 23 Apr 2025 19:35:11 +0000

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

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 23 Apr 2025 19:35:52 +0000

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

---

From: "kneth (@kneth)" <[email-masked]>
Date: Thu, 24 Apr 2025 10:58:25 +0000

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.

---

From: "kneth (@kneth)" <[email-masked]>
Date: Thu, 24 Apr 2025 11:45:43 +0000

@davecramer says
> [...] whitespace changes [...]

I have restored the white spaces

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 24 Apr 2025 12:18:26 +0000

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.

---

From: "kneth (@kneth)" <[email-masked]>
Date: Thu, 24 Apr 2025 13:00:31 +0000

I'll take a look at it!

---

From: "kneth (@kneth)" <[email-masked]>
Date: Thu, 24 Apr 2025 15:54:40 +0000

@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

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 24 Apr 2025 17:25:23 +0000

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

---

From: "kneth (@kneth)" <[email-masked]>
Date: Fri, 25 Apr 2025 11:52:34 +0000

@davecramer 

> one more fix

... and done 😄

---

From: "kneth (@kneth)" <[email-masked]>
Date: Fri, 25 Apr 2025 13:37:59 +0000

Thank you for merging

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Fri, 25 Apr 2025 13:39:04 +0000

> Thank you for merging

Thank you for persisting through this process