Subject: [pgjdbc/pgjdbc] PR #3390: Fix PgDatabaseMetaData implementation of catalog as param and return value

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Thu, 26 Sep 2024 20:29:24 +0000

Fixes https://github.com/pgjdbc/pgjdbc/issues/3389 "Metadata results from getSchemas(String catalog, String schemaPattern) violate JDBC spec"
Fixes https://github.com/pgjdbc/pgjdbc/issues/3394 "getSchemas with empty string for schemaPattern returns schemas, violating JDBC spec"

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

<!-- You can erase any parts of this template not applicable to your Pull Request. -->

### Changes to Existing Features:

* [ ] Does this break existing behaviour? If so please explain.
* [x] Have you added an explanation of what your changes do and why you'd like us to include them?
* [x] Have you written new tests for your core changes, as applicable?
* [x] Have you successfully run tests with your changes locally?

---

From: "svendiedrichsen (@svendiedrichsen)" <[email-masked]>
Date: Thu, 26 Sep 2024 21:47:37 +0000

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

Why not ignore this case like the NULL case? And if ignore it like NULL you can simplify the whole if like in the schemaPattern if below.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Fri, 27 Sep 2024 13:01:19 +0000

as appealing as it is to use the `information schema`, this is actually a view and adds another select into this query. Is there any reason to do this ?

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Fri, 27 Sep 2024 15:03:06 +0000

If `catalog` is `""`, then getSchemas should return schemas "without a catalog" per [JDBC spec:](https://docs.oracle.com/en/java/javase/17/docs/api/java.sql/java/sql/DatabaseMetaData.html#getSchemas(java.lang.String,java.lang.String))

> catalog - a catalog name; must match the catalog name as it is stored in the database;"" retrieves those without a catalog; null means catalog name should not be used to narrow down the search.

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Fri, 27 Sep 2024 15:03:29 +0000

Fixed

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Tue, 01 Oct 2024 13:24:06 +0000

I removed the empty string check for `schemaPattern`, because no rows should be returned if `schemaPattern` is an empty string per JDBC spec.
 
> schemaPattern - a schema name; must match the schema name as it is stored in the database; null means schema name should not be used to narrow down the search.

Please approve.

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Thu, 03 Oct 2024 14:14:08 +0000

I have added new tests and updated the existing test.

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Fri, 04 Oct 2024 15:05:56 +0000

Mapping the catalog name to the database name is already canonical in postgres itself, because when I `select pg_get_viewdef('information_schema.schemata', true);`, postgres returns the following view definition:
```sql
 SELECT current_database()::information_schema.sql_identifier AS catalog_name,
    n.nspname::information_schema.sql_identifier AS schema_name,
    u.rolname::information_schema.sql_identifier AS schema_owner,
    NULL::name::information_schema.sql_identifier AS default_character_set_catalog,
    NULL::name::information_schema.sql_identifier AS default_character_set_schema,
    NULL::name::information_schema.sql_identifier AS default_character_set_name,
    NULL::character varying::information_schema.character_data AS sql_path
   FROM pg_namespace n,
    pg_authid u
  WHERE n.nspowner = u.oid AND (pg_has_role(n.nspowner, 'USAGE'::text) OR has_schema_privilege(n.oid, 'CREATE, USAGE'::text));
```

An alternative to using `current_database()::information_schema.sql_identifier` for `TABLE_CATALOG` is to select `catalog_name` from the view `information_schema.schemata` to make the driver future-compatible with any design changes in postgres that would allow a postgres connection to access multiple databases. However, as you had alluded, the disadvantage would be the performance hit from the view selecting from `pg_authid`.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 15 Oct 2024 13:54:44 +0000

@SophiahHo I don't think in the case of PostgreSQL there will ever be a situation where a connection can access another database.

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Thu, 17 Oct 2024 15:07:20 +0000

@davecramer I added a comment on https://github.com/pgjdbc/pgjdbc/issues/3389 to explain that https://github.com/pgjdbc/pgjdbc/commit/9ea59bd87f7920f1c2aba20ffef15acef85def60 added partial support for catalogs and broke things.

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Tue, 10 Dec 2024 14:56:34 +0000

@davecramer When our enterprise customers upgraded their pgjdbc driver from a version older than 42.7.0 to a version between 42.7.0 and 42.7.4, they are getting [duplicated database objects for metadata queries.](https://github.com/pgjdbc/pgjdbc/issues/3389#issuecomment-2414487622) As a workaround, we asked our enterprise customers to downgrade their pgjdbc driver to a version before 42.7.0. This MR fixes this bug so that our enterprise users would be able to use the latest pgjdbc driver with security updates.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 10 Dec 2024 18:17:28 +0000

(on pgjdbc/src/test/java/org/postgresql/test/jdbc2/DatabaseMetaDataTest.java:283)

Searching with an empty catalog and schema is valid. Please add another test instead of just changing this one

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Tue, 10 Dec 2024 19:45:07 +0000

@davecramer Per https://docs.oracle.com/javase/8/docs/api/java/sql/DatabaseMetaData.html#getColumns-java.lang.String-java.lang.String-java.lang.String-java.lang.String- : 
> Parameters:
catalog - a catalog name; must match the catalog name as it is stored in the database; "" retrieves those without a catalog; null means that the catalog name should not be used to narrow the search
schemaPattern - a schema name pattern; must match the schema name as it is stored in the database; "" retrieves those without a schema; null means that the schema name should not be used to narrow the search

Searching with an empty catalog and schema should return no results. If you don't want the change in behaviour for how empty strings are handled, I can remove it, because it's not a priority for me/us. However, the current behaviour technically violates the JDBC spec.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Tue, 10 Dec 2024 20:53:42 +0000

I didn't say I wanted to change the behaviour; I said I wanted the test in the same place as the original.

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Tue, 10 Dec 2024 21:47:14 +0000

@davecramer I separated out the tests for empty vs null.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 11 Dec 2024 13:09:40 +0000

@SophiahHo please update all the tests where you removed "","" and add them back in. Keep the null, null but removing coverage doesn't seem like the right thing to do
Also who is the company you are working for ?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 11 Dec 2024 19:26:24 +0000

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

As we update code, can you change this to use try with resources and get rid of the closeDB below ?

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 11 Dec 2024 19:26:58 +0000

Use try with resources

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 11 Dec 2024 19:28:27 +0000

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

fix the error message to say noprocedures

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 11 Dec 2024 19:28:39 +0000

(on pgjdbc/src/test/java/org/postgresql/test/jdbc4/DatabaseMetaDataTest.java:345)

fix error message

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Wed, 11 Dec 2024 19:29:42 +0000

Once we fix the code can you also do the release notes for this please

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Wed, 11 Dec 2024 21:36:03 +0000

@davecramer Do you mean run `release_notes.sh`? I'm using Windows Subsystem for Linux (WSL) to run shell scripts, and I ran into an issue compiling a Perl module in WSL that the script uses.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 12 Dec 2024 10:13:51 +0000

No, I mean explain what the changes are in the commit message.

Thanks!

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Thu, 12 Dec 2024 14:52:49 +0000

Changes:
- For DatabaseMetaData methods, the `catalog` argument filters the results by catalog: it must match the current database name or be null to return non-empty results.
- For DatabaseMetaData methods, the returned ResultSet populates the current database name to columns like TABLE_CAT, TABLE_CATALOG, FUNCTION_CAT, PROCEDURE_CAT, PKTABLE_CAT, FKTABLE_CAT, TYPE_CAT.
- For DatabaseMetaData methods, empty strings for the following arguments can filter out results: `schemaPattern`, `tableNamePattern`, `columnNamePattern`, `functionNamePattern`, `procedureNamePattern`, `table`, `schema`, `primaryTable`, `foreignTable`, `primarySchema`, `foreignSchema`, `primaryCatalog`, `foreignCatalog`.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 12 Dec 2024 16:16:23 +0000

(on pgjdbc/src/main/java/org/postgresql/jdbc/PgDatabaseMetaData.java:1195)

I hadn't seen this until now. I would think one should be able to use SPECIFIC_NAME to refer to an object in the database, no? This would create a name such as 123_somefunction which while unique can't be used to find the function.

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Thu, 12 Dec 2024 16:29:13 +0000

@davecramer I did not add or update line 1194/1195. This is existing code.

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 12 Dec 2024 17:07:57 +0000

you are correct. thanks

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 12 Dec 2024 17:08:24 +0000

I'd still be curious who the company is ?

---

From: "SophiahHo (@SophiahHo)" <[email-masked]>
Date: Thu, 12 Dec 2024 17:36:49 +0000

@davecramer Thanks for merging. I work at [Precisely](https://www.precisely.com/).

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Thu, 12 Dec 2024 17:43:40 +0000

You are welcome, thanks for hanging in there :)

---

From: "vnobo (@vnobo)" <[email-masked]>
Date: Fri, 21 Feb 2025 04:13:05 +0000

Very slow, very slow, for this investigation for a day, it is here

---

From: "davecramer (@davecramer)" <[email-masked]>
Date: Fri, 21 Feb 2025 13:14:08 +0000

> Very slow, very slow, for this investigation for a day, it is here

Please, please elaborate. I have no idea what you are trying to say here ?

---

From: "kdebski85 (@kdebski85)" <[email-masked]>
Date: Tue, 25 Feb 2025 13:47:44 +0000

@davecramer 

[vnobo](https://github.com/vnobo) probably meant the performance regression in 24.7.5.
Please see https://github.com/pgjdbc/pgjdbc/issues/3505