Skip to content

Conversation

SophiahHo
Copy link
Contributor

Fixes DatabaseMetaData performance issue #3511

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?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does ./gradlew styleCheck pass ?
  3. Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

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

@davecramer
Copy link
Member

Did you check to make sure the plan is the same as previously ?

@SophiahHo
Copy link
Contributor Author

@davecramer I assume the plan would be different, because I verified that the change drastically improved the performance of getCrossReference. With the same arguments and environment, version 42.7.5 took 3835 ms and the new version took 1504 ms.

@davecramer
Copy link
Member

@SophiahHo thanks!

@davecramer
Copy link
Member

@SophiahHo reviewing this as I understand this you are adding and FALSE if the catalog isn't equal to the current catalog. Why even run the SQL? We know the ResultSet is going to be empty, we could fabricate an empty ResultSet instead.

@SophiahHo
Copy link
Contributor Author

SophiahHo commented Apr 3, 2025

@davecramer Fabricating an empty ResultSet would be pages and pages long. Additionally, returning an empty ResultSet would be incorrect behaviour if the database is down, the database user credentials were revoked or changed, etc., which should throw an Exception.

@vlsi
Copy link
Member

vlsi commented Apr 9, 2025

I think it should be uncommon for the client code to ask for a list of "procedures in a foreign catalog", so it should be fine to go with and false trick.
At the same time, certain cases make it easy to return empty resultset without making a database roundtrip.

@SophiahHo , in some of the cases, we use org.postgresql.core.BaseStatement#createDriverResultSet(Field[] fields, List<Tuple> tuples) can create resultset. For instance, getProcedureColumns prepares Field[] declaration, and then it executes a query.
In that case we could use something like

if (catalog != null && !catalog.equals(...)) {
  // Returns empty resultset
  ((BaseStatement) createMetaDataStatement()).createDriverResultSet(f, v)
}

I guess Dave meant something like that. WDYT?

@davecramer
Copy link
Member

@vlsi I already merged #3588

@vlsi
Copy link
Member

vlsi commented Apr 13, 2025

Does it mean we should close the current PR then?

@davecramer
Copy link
Member

Does it mean we should close the current PR then?

Yes, I would think so.

@davecramer davecramer closed this Apr 14, 2025
@davecramer
Copy link
Member

Closed in favour of #3588

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants