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

fix: use sys info function instead of connection db name #4623

Merged
merged 2 commits into from Aug 20, 2021

Conversation

sifer
Copy link
Contributor

@sifer sifer commented Aug 17, 2021

There is an edge case when getting proxied by for example PgBouncer that could result in client connection not being the same as the current database.
Using current_database() seems like a safer approach.

@kibertoad
Copy link
Collaborator

can you add test for this?

@sifer
Copy link
Contributor Author

sifer commented Aug 17, 2021

@kibertoad sorry, I updated affected tests. There were already tests covering this, not only including the two i updated.

@kibertoad
Copy link
Collaborator

@rijkvanzanten Does this make sense for you?

@rijkvanzanten
Copy link
Collaborator

Yes it does!

Like @sifer mentioned, it is a bit of an edge case, but you can in fact tell PgBouncer to expose a different database name than the one used in the actual database, effectively creating an alias for the database. By relying on the connection config in knex, it'll use the name of the alias in the SQL query instead of the name of the actual database, which will fail.

It does make me wonder: does the same problem occur with other databases and load balancer combinations?

@kibertoad kibertoad merged commit 3a083d9 into knex:master Aug 20, 2021
@kibertoad
Copy link
Collaborator

Released in 0.95.11

OlivierCavadenti pushed a commit to AbeonaPascha/knex that referenced this pull request Nov 4, 2021
Co-authored-by: Maximilian-Albin Ekström <maximilian-albin.ekstrom@instabox.se>
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.

None yet

3 participants