Subject: Re: [pgjdbc/pgjdbc] PR #3565: Use query to find the current catalog instead of relying on the database in the connection URL or connection properties as this could be different if connected through a pooler or proxy

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 01 Apr 2025 18:39:00 +0000

@vlsi I'd like to merge this. Any issues ?

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 01 Apr 2025 19:28:01 +0000

(on pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java)

```suggestion
  private @Nullable String catalog;
```

I guess we have `@SuppressWarning` for `PgConnection` constructor, so it results in silencing warning for the newly added field.
However, the field is nullable, so let's specify nullable type.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Tue, 01 Apr 2025 19:30:09 +0000

```suggestion
    String catalog = this.catalog;
    if (catalog == null) {
      try (Statement stmt = createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_READ_ONLY);
            ResultSet rs = stmt.executeQuery("select current_catalog")) {
        if (rs.next()) {
          this.catalog = catalog = rs.getString(1);
        }
      }
    }
    return catalog;
```

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Wed, 02 Apr 2025 12:07:02 +0000

Let me preface this by saying I'm not opposed to this PR ... though thinking about this a couple questions came to mind.

What happens if the DB changes? The whole point of this PR is to skip the fixed value in connection properties and pass through to the actual database, potentially bypassing any intermediate connection pool(s). If the pooler reroutes the connection somewhere else then it'd be wrong as we would have cached the previous value. I suppose that type of pooling is less common, but it does come up for things like live migrations and upgrades.

There's also a slight change in behavior here. Previously the method was a pass through to `QueryExecutor.getDatabase()` which returned a cached `String` value. Now that we always execute the command on the first call, this means you cannot call this method while the connection is otherwise in use. I'm worried this may lead to some weird edge case where a user runs a query or COPY operation, and then tries to use this data while processing the result.

We could resolve this by caching the value at connection startup. But I don't think we want to add more mandatory startup round trips.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 02 Apr 2025 12:09:36 +0000

Actually one point of the PR is to make sure we get the right database. If you use pg_bouncer then you connect to some database in pg_bouncer which redirects you to the actual database. 

Q: How can the database change ?

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Wed, 02 Apr 2025 12:17:49 +0000

The situation I'm describing is where the pooler swaps out the physical connection. There's no guarantee I get the same catalog when it opens a new connection.

Interestingly I found this pgbouncer fork (replacement?) which seems to do exactly what I'm describing: https://github.com/awslabs/pgbouncer-fast-switchover

Imagine configuring that with multiple targets that have different database names. Could be the same actual database, just the return value of `SELECT catalog_name` is different.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 02 Apr 2025 12:28:58 +0000

Well our options are to query the database every time or cache it. 
99.99999% would be OK with caching it.

---

From: "sehrope (@sehrope)" <[email-masked]>
Date: Wed, 02 Apr 2025 12:41:43 +0000

Or return the original value and have people who are doing that pooling stuff explicitly query for the live one themselves.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 02 Apr 2025 13:39:41 +0000

It would be great if `catalog_name` was `GUC_REPORT`.
The bouncer could emit notifications if the `catalog_name` ever changed.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 02 Apr 2025 13:49:23 +0000

> It would be great if `catalog_name` was `GUC_REPORT`. The bouncer could emit notifications if the `catalog_name` ever changed.

That is really a pg_bouncer problem. In reality the server will never change the catalog once the connection is established. I think we could get pg_bouncer to emit `database` however.

---

From: "vlsi (@vlsi)" <[email-masked]>
Date: Wed, 02 Apr 2025 16:06:41 +0000

My feeling exactly. However, if we expect `pg_bouncer` to emit `database`, we should expect the DB to emit it as well.
However, hope the current PR is good enough.

---

From: "landryb (@landryb)" <[email-masked]>
Date: Sat, 31 May 2025 05:14:23 +0000

thanks for this PR, 42.7.6 fixes a regression that was introduced with #3390.

Using 42.7.3 against a database cnx handled by pgbouncer having a different name/alias than the real database, geotools was able to list attributes/fields in a layer/table without issues, while it started failing after an update to 42.7.5.

turning `log_statement` on, i saw that the query done had `AND current_database() = 'pgbouncer_alias'` which wasn't the real table name, thus no attributes were returned and geotools failed accessing the layer geometry.

replacing 42.7.5 jar by 42.7.6 jar, everything works as before, so thanks @davecramer !