Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support long database names in PostgreSQL 17.x #6059

Open
wants to merge 2 commits into
base: release24.11-SNAPSHOT
Choose a base branch
from

Conversation

labkey-adam
Copy link
Contributor

@labkey-adam labkey-adam commented Nov 15, 2024

Rationale

PostgreSQL used to accept database names longer than 63 characters, truncating them for all operations. As of 17.x, it now fails to connect when the database name is too long. TeamCity uses very long database names, so we now truncate and replace the URL in the DataSource to maintain previous behavior. https://www.labkey.org/home/Developer/issues/issues-details.view?issueId=51676

Also, change getIdentifierMaxLength() to report the database's actual limit. Previously, the dialect reported two or three characters less than the actual limit (we were inconsistent) in an attempt to help callers that might be adding suffixes. Seems like the dialect should just report the facts; callers are now responsible for respecting the database-imposed limit.

Copy link
Contributor

@labkey-jeckels labkey-jeckels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this change, though it seems wiser to stop generating ones that are so long.

api/src/org/labkey/api/query/AliasManager.java Outdated Show resolved Hide resolved
@labkey-adam
Copy link
Contributor Author

I'm OK with this change, though it seems wiser to stop generating ones that are so long.

@labkey-tchad ?

@labkey-tchad
Copy link
Member

I'm OK with this change, though it seems wiser to stop generating ones that are so long.

@labkey-tchad ?

Agreed; it seems bad to just silently truncate to something other than what the configuration specifies. I presume the Postgres folks made the same conclusion.
I can configure TeamCity to override the database name provided by the gradle plugin. It isn't really necessary to use the build ID, that was just something easy and relatively unique.

@labkey-adam
Copy link
Contributor Author

I'm OK with this change, though it seems wiser to stop generating ones that are so long.

@labkey-tchad ?

Agreed; it seems bad to just silently truncate to something other than what the configuration specifies. I presume the Postgres folks made the same conclusion. I can configure TeamCity to override the database name provided by the gradle plugin. It isn't really necessary to use the build ID, that was just something easy and relatively unique.

I filed an issue with the PostgreSQL team given the long-standing truncation behavior, the now inconsistent behavior (CREATE and ALTER DATABASE work fine, but connecting fails), and the lack of any mention in the release notes... while fully acknowledging that we were relying on undocumented behavior and we can easily work around this. Once TeamCity is using shorter database names I can remove the code that truncates and futzes with the URL in the DataSource. The rest of this PR (having the dialect report the actual max length instead of guessing what callers might do) seems worthwhile, so I'll merge it eventually.

@labkey-tchad The current truncation code should allow us to start testing on PostgreSQL 17.x without adjusting those names. Can you enable PG 17 on this branch?

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