Skip to content

Expand warnings around the implications of transaction pooling #1180

Closed
cosgroveb wants to merge 2 commits into
pgbouncer:masterfrom
cosgroveb:docs-improve-transaction-pooling-warnings
Closed

Expand warnings around the implications of transaction pooling #1180
cosgroveb wants to merge 2 commits into
pgbouncer:masterfrom
cosgroveb:docs-improve-transaction-pooling-warnings

Conversation

@cosgroveb

@cosgroveb cosgroveb commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

The motivation for this PR is to better-educate users who, reasonably, may want to use transaction pooling but may also be running one of the world's most popular web frameworks which would be, cryptically, considered a "broken setup" by the current docs since it manipulates the session extensively out of the box with no levers that I am aware of to prevent that! 😄

cosgroveb and others added 2 commits October 2, 2024 17:59
@cosgroveb cosgroveb changed the title Expand warnings around the implications of transaction pooling to note plan caches Expand warnings around the implications of transaction pooling Oct 3, 2024
Comment thread doc/config.md
Comment on lines +618 to +625
Note: Using `DISCARD ALL` also works around the very non-obvious potential performance land-mine
that can happen to query plan caching with the server-side `plan_cache_mode` default of `auto`.
Clients can be stuck with a cached plan that was efficient for a previous client that is
no longer because the dataset being queried has changed. With transaction pooling it may
be worth considering setting `plan_cache_mode` to `custom`. When using transaction pooling it
is at a minimum [strongly adised to review the Postgres docs][postgres-plan-cache-mode-docs].

[postgres-plan-cache-mode-docs]: https://www.postgresql.org/docs/current/runtime-config-query.html#GUC-PLAN-CACHE-MODE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this note should maybe be part of the max_prepared_statements config, since that is enabling prepared statements in the first place. At minimum we should link there from here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah maybe I didn't outline this gotcha very well because you don't need to use prepared statements to get a backend conn with a suboptimal cached plan

@cosgroveb cosgroveb Oct 7, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rather implicit prepared statements being the surprising culprit

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah but yes that warning may be better fit if moved per your suggestion

Comment thread doc/config.md
Comment on lines +613 to +614
unintentional breakage in an application or its' dependencies (for instance ActivRecord
uses session features out of the box).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be good to clarify exactly what the session level features are unsupported? Because a bunch of session features actually are supported these days, see max_prepared_statements and track_extra_parameters. So I think it would be good to call out what is and what isn't supported. And it might be useful to say what exactly ActiveRecord is doing that is unsupported.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we really need to mention tools here? These tools might eventually change and this information is obsolete. I prefer the other way, open a PR for the referred product saying that it doesn't work with PgBouncer if you are using feature X. Once they fix this issue, their documentation is updated accordingly saying that PgBouncer is supported.

@cosgroveb

Copy link
Copy Markdown
Contributor Author

I want to think more about helpful ways to communicate the things about transaction pooling that have been surprising to me and also plan caching so that I can make very clear and easy to understand updates to the documentation here. Thank you for reviewing these thoughts @eulerto and @JelteF

@cosgroveb cosgroveb closed this Oct 19, 2024
@cosgroveb cosgroveb deleted the docs-improve-transaction-pooling-warnings branch December 12, 2024 21:23
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