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

Handle DEALLOCATE ALL and DISCARD ALL for prepared statements #972

Merged
merged 10 commits into from Nov 1, 2023

Conversation

knizhnik
Copy link
Contributor

@knizhnik knizhnik commented Oct 27, 2023

This implements a hacky/clever approach of handling the DEALLOCATE ALL and DISCARD ALL SQL statements. Instead of parsing the queries themselves, which PgBouncer avoids doing completely, this parses the CommandComplete packet returned by the server.

Fixes prisma/prisma#21635
Fixes #974

Copy link
Member

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

I think supporting DEALLOCATE ALL is a worthy effort. It currently silently breaks the whole server. And because there is no protocol level equivalent, all clients that want send this need to send it as SQL.

When I saw the title of this PR I thought I was going to hate it, because I'd expect it to implement a very simple query parser. And I was happily surprised that's not what's going on. Instead it's using the command tag from the CommandComplete packet. And I actually think that idea is quite elegant, since the command tags are well defined by postgres and easy to parse (as opposed to SQL which can have comments or extra spaces or lowercase). So I quite like this general approach. I left some comments on changes that should be done to make this handle some more edge cases.

Also this needs some tests. And docs need to be updated that we now actually do support DEALLOCATE ALL

src/server.c Show resolved Hide resolved
src/server.c Show resolved Hide resolved
src/server.c Outdated Show resolved Hide resolved
@janpio
Copy link

janpio commented Oct 30, 2023

I have just opened #974, which explains the problem and thus motivation behind such a change a bit more.

Great to see there is already a PR aiming to fix this problem! 💚

Copy link
Member

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

Code looks good. All it now needs is a test.

src/server.c Outdated Show resolved Hide resolved
lib Outdated Show resolved Hide resolved
@JelteF
Copy link
Member

JelteF commented Oct 31, 2023

Also CI is complaining about the codeformatting. make format should fix that.

@JelteF JelteF changed the title Delete all prepared statements if DEALLOCATE ALL command was executed Handle DEALLOCATE ALL and DISCARD ALL for prepared statements Nov 1, 2023
@JelteF JelteF merged commit a7b3c0a into pgbouncer:master Nov 1, 2023
7 checks passed
@petere
Copy link
Member

petere commented Nov 2, 2023

While this is not "query parsing", it still assumes semantics about the contained SQL commands that are not part of the protocol specification. How might this affect non-PostgreSQL uses of PgBouncer.

@JelteF
Copy link
Member

JelteF commented Nov 2, 2023

How might this affect non-PostgreSQL uses of PgBouncer.

To be clear, you're talking about other databases that use the Postgres protocol, but don't speak Postgres its SQL dialect, right? If so, then (tl;dr) I think the impact would be negligable.

The "CommandComplete parsing" here would only have any impact in practice if the server responded with DISCARD ALL/DEALLOCATE ALL to a query. While not a strict requirement the protocol description of the CommandComplete packet indicates that this really only should be done when a DEALLOCATE ALL/DISCARD ALL query was sent:

The command tag. This is usually a single word that identifies which SQL command was completed.

While in theory it's possible that some other non-PostgreSQL server implements a DEALLOCATE ALL/DISCARD ALL command with different semantics than Postgres, it seems rather unlikely that in practice such different semantics would not include clearing of the prepared statements. Especially because there's no protocol level message that can be used for the purpose of clearing all prepared statements. So in practice I don't think this would impact anyone badly.

bayandin pushed a commit to neondatabase/pgbouncer that referenced this pull request Jan 2, 2024
…cer#972)

This implements a hacky/clever approach of handling
the `DEALLOCATE ALL` and `DISCARD ALL` SQL
statements. Instead of parsing the queries themselves,
which PgBouncer avoids doing completely, this parses
the CommandComplete packet returned by the server.

Fixes prisma/prisma#21635
Fixes pgbouncer#974
bayandin added a commit to neondatabase/neon that referenced this pull request Feb 2, 2024
## Problem
Update pgbouncer from 1.21 (and patches[0][1]) to 1.22 (which includes
these patches)
- [0] pgbouncer/pgbouncer#972
- [1] pgbouncer/pgbouncer#998

## Summary of changes
- Build pgbouncer 1.22.0 for neonVMs from upstream
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants