Add PGBOUNCER_MAX_PREPARED_STATEMENTS config

[habovh]

Following issues I had when using this buildpack with Prisma ORM and prepared statements, I am suggesting these changes to add support for max_prepared_statements configuration on pgbouncer.

As it turns out, it seems that if not set, the default value would be 0. This is an issue when using transaction pooling mode, and prevents from using prepared statements in that case.

Giving control to the user on this important configuration seems appropriate, keeping the default value at zero to avoid breaking changes, basically making it an opt-in config.

I updated the *.tgz files but I'm not sure this was needed as I'm not familiar with how build packs work, maybe the changes to bin/gen-pgbouncer-conf.sh were enough. Let me know so I can update my branch accordingly.

Relevant issues:

• Update to more recent pgbouncer to support prepared statements
  • Update to 1.21.0 #182
• Prisma issue with prepared statements
  • Issues with PgBouncer on DigitalOcean - CODE: 26000 ERROR: prepared statement \"PGBOUNCER_34\" does not exist prisma/prisma#21877

[edmorley]

Hi - thank you for the PR!

I updated the *.tgz files but I'm not sure this was needed as I'm not familiar with how build packs work, maybe the changes to bin/gen-pgbouncer-conf.sh were enough. Let me know so I can update my branch accordingly.

I'm not on the Heroku Data team (so I'll leave it to them to properly review), but have occasionally contributed to this buildpack in the past. I don't think regenerating those *.tgz is needed for this change - and also we'd need to run the regeneration ourselves due to the risk of accepting potentially arbitrary binaries. So perhaps omit those changes for now? :-)

[habovh]

Hi @edmorley, thanks for your comment! It makes sense not to rely on a third-party to regenerate those. I've removed the commit responsible for updating the *.tgz files. Hopefully this can get reviewed & merged soon!

[mble-sfdc]

@habovh I will take a look at this by the end of the week. Main thing this is missing is tests - adding an extension to gen-pgbouncer-conf.bats to verify the new config is rendered correctly would be 🚀

[habovh]

@mble-sfdc ask and you shall receive. I've updated the test suite to include this new configuration.

[habovh]

Also just so everyone is reassured about the default value, here's an excerpt from the SHOW CONFIG; output when run within the heroku-24 container without the changes introduced by this PR:

            key            |                           value                           |                        default                         | changeable 
---------------------------+-----------------------------------------------------------+--------------------------------------------------------+------------
[...]
 max_client_conn           | 100                                                       | 100                                                    | yes
 max_db_connections        | 0                                                         | 0                                                      | yes
 max_packet_size           | 2147483647                                                | 2147483647                                             | yes
 max_prepared_statements   | 0                                                         | 0                                                      | yes
 max_user_connections      | 0                                                         | 0                                                      | yes
 min_pool_size             | 0                                                         | 0                                                      | yes
[...]

As you can see, the default value for max_prepared_statements is 0. What's odd is that the same pgbouncer version running locally on my Mac has a default value of 200. I'm not sure why that is, but it further increased the confusion when trying to understand what was missing to get prepared statements working in tx pooling mode.

Edit: my local version was indeed 1.24.x with the new default.

[mble-sfdc]

@habovh many thanks for the testing. The default value of 200 was added in pgbouncer 1.24.0, so that is likely why you are seeing it - this buildpack currently uses 1.23.1.

You'll need to add an entry to Changelog.md under ## Unreleased, something like:

* Add PGBOUNCER_MAX_PREPARED_STATEMENTS config with default of 0 (@habovh)

Then we should be all good.

[habovh]

@mble-sfdc added the suggested entry to the changelog and updated my previous message as well as you pointed out 1.24 brought this new default.

[binarycleric]

This all looks good to me. @mble-sfdc do you feel good about merging this?

[mble-sfdc]

@habovh we will need to do a formal release, but you can set your app to use the buildpack from main. Thanks for your contribution! 💜

[habovh]

@mble-sfdc awesome, I'll keep using my fork until you guys release an update, but I'm thrilled this got merged!

Happy to contribute and I hope this will help people looking to optimize their db connections ☺️