Enable prepared statement support by default

[JelteF]

By the time we'll release the next PgBouncer version it will be roughly
a year since we added prepared statement support as an opt-in feature.
Quite a few managed services enabled this feature by default. Only very
few issues were reported that were related to it, and all of them have
been fixed.

Since defaults matter, I think it's time that we change the default to
enable this feature by default in the next release. I'd like to merge
this early in the release cycle so that devs have the highest chance of
finding any more issues with it. But I expect no new issues to be found.

[eulerto]

Enabling prepared statements by default is a natural choice. The question is: when? I've heard people successfully using it. So far, no issues reported and we didn't have recent reports about it. However, enabling this feature will impose performance penalty to existing PgBouncer setups (?). I would be more comfortable if we already have the answers for some common questions like: it might increase your CPU usage up to +X%. It increases the memory usage by Y MB por connection (we already have this number which is good). My suggestion is that we run some benchmarks (pgbench, HammerDB, ...) to test the impact of: (a) increase the number of prepared statements, (b) number of prepared statements is greater than max_prepared_statements, (c) same number of prepared statements and increase the number of client connections.

I'm curious about the 200. That's because it is referred in the documentation or do you have a good reason to suggest this number?

[JelteF]

I think CPU/memory usage for users that use named prepared statements is not something we should really consider when discussing to turn this feature on by default. If a user uses named prepared statements, they want this setting turned on. If their app is broken, they won't care if CPU/memory usage is low. And named prepared statements are by definition broken when this feature is not turned on. To be clear, I definitely think we should care about CPU/memory usage in general and also for this feature, but the trade-off high resource usage vs broken app is pretty clear no matter the actual resource usage.

The thing I do think we should care about is CPU usage when not using named prepared statements at all. CPU usage shouldn't increase dramatically, because we turn a feature on that you don't care about. I don't think we have to worry here though because the impact should be negligible. The only additional code that is executed when turning the setting on is code that is guarded by is_prepared_statements_enabled. And this code does the following:

1. Check for DEALLOCATE ALL/DISCARD ALL in the returned data from the server
2. Check if the Parse/Bind/Close/Describe are for a named prepared statement

The additional CPU usage caused by 1, would be comparing 3 characters for DELETE queries and only 1 for INSERT/SELECT/UPDATE queries. This seems completely negligible in the grand scheme of things that PgBouncer does for every query.

And for 2 it's very similar. It checks the first 2 or 3 bytes of these messages, to see if the prepared statement name is something different than the empty string (the normal extended protocol uses an empty string as the statement name). If it's the empty string, no other code is executed.

[JelteF]

I'm curious about the 200. That's because it is referred in the documentation or do you have a good reason to suggest this number?

It's just a ballpark reasonable number for most usecases in my experience. Anything between 100 and 500 would be a fine default imho. The perf will be good if the number is higher than the number of commonly executed unique prepared statements, because than the cache actually works. For most people I hope that number is less than 100. Then really the only thing that matters in memory usage. Setting it to high can bloat memory because prepared statements are cached that are only executed once, e.g. because they get generated by some dynamic query builder based on user input.

So 200 seemed like a decent default tradeoff. High enough to cache the most commonly executed queries. But not so high that there's a bunch of useless stuff in your cache either.

Also, people can still tune this. The default is mainly meant to make PgBouncer work with named prepared statements out of the box. Even 1 would work for that honestly, but perf will be bad by default for pretty much everyone using the feature.

[JelteF]

I'm going to merge this, to give it some additional baking time on the master branch for the next release. Because as explained above, I don't think running benchmarks is required for this due to it having no impact on people not using prepared statements. If you disagree, we can revert the change before the actual release.

[alvherre]

I want to point out for the record that, ironically, this change caused overnight downtime for the PG buildfarm, which had started using pgbouncer merely a month ago. We upgraded from 1.23 to 1.24 last night, following the PGDG packaging.

[JelteF]

I want to point out for the record that, ironically, this change caused overnight downtime for the PG buildfarm, which had started using pgbouncer merely a month ago.

Could you share some details on what went wrong? (preferably in a dedicated issue)