Make varcache (the pg parameters cached per connection) dynamically configurable. #867

emelsimsek · 2023-06-12T12:23:51Z

This change enables caching other postgres parameters in addition to the static list {"client_encoding", "DateStyle", "TimeZone", "standard_conforming_strings", "application_name"} that is already supported.

Testing:

Add track_extra_parameters in *.ini file to enable caching search_path per connection.

[pgbouncer]
  ...
  track_extra_parameters = search_path, IntervalStyle

Related to #452 and #482
Fixes #470
Fixes #821

include/varcache.h

src/main.c

src/client.c

src/varcache.c

src/main.c

src/client.c

doc/config.md

eulerto · 2023-06-15T13:15:47Z

This is the 2nd patch that is proposing to include uthash. I'm wondering if, for maintenance reason, it would be interesting to include this code into the tree. It is another task to do before releasing a new PgBouncer version. It is also hard to track bugs in this code. Let's not be responsible to this code unless it is no longer maintained. My suggestion is to create another submodule (like libusual).

Should uthash be added to libusual? Couldn't the hash code from libusual be used?

Regarding the track_startup_parameters, it is described as:

By default, PgBouncer tracks client_encoding, datestyle, timezone, standard_conforming_strings
and application_name parameters per client. To allow other parameters to be tracked, they can be
specified here, so that PgBouncer knows that they should be maintained in the client variable cache
and restored in the server whenever the client becomes active.

It means there is an list that is part of tracked parameters but these parameters aren't part of the default value. These parameters are part of an implementation detail but your PR is exposing it but keeping the original part hidden. If I was proposing such feature I wouldn't use 2 lists. Instead, use only a dynamic list and the default value is the current list. I would also add a check to avoid someone from removing the original list (I don't have a strong opinion if we should allow removing some of these original parameters from the list.)

test/test_misc.py

src/main.c

src/varcache.c

JelteF · 2023-06-20T13:38:13Z

@eulerto @emelsimsek

This is the 2nd patch that is proposing to include uthash. I'm wondering if, for maintenance reason, it would be interesting to include this code into the tree. It is another task to do before releasing a new PgBouncer version. It is also hard to track bugs in this code. Let's not be responsible to this code unless it is no longer maintained. My suggestion is to create another submodule (like libusual).

I totally agree with using a submodule for uthash instead of vendoring the file in.

Should uthash be added to libusual?

I think submodule in submodule sounds like a recipe for problems.

Couldn't the hash code from libusual be used?

I think for the usecase in this PR the one from libusual would be fine too (to clarify I mean this one: https://github.com/libusual/libusual/blob/master/usual/hashtab-impl.h). But for the prepared statement one the one from libusual is way too basic (e.g. the hash tables from libusual are fixed size). So for that one we'll need uthash anyway.

I'm a bit on the fence for this, because of two competing considerations:

on one hand I don't like introducing a dependency unnecessarily
on the other hand, I'd rather use only a single hashtable implementation in PgBouncer instead of two.

I think consideration 2 weighs more heavily for me, given the fact that we'll quite likely want to introduce uthash for the prepared statements PR. Which is why I also suggested using uthash to @emelsimsek (also because I thought that this PR also needed a growable hashtable, but that's not actually the case)

Regarding the track_startup_parameters, it is described as:

By default, PgBouncer tracks client_encoding, datestyle, timezone, standard_conforming_strings
and application_name parameters per client. To allow other parameters to be tracked, they can be
specified here, so that PgBouncer knows that they should be maintained in the client variable cache
and restored in the server whenever the client becomes active.

It means there is an list that is part of tracked parameters but these parameters aren't part of the default value. These parameters are part of an implementation detail but your PR is exposing it but keeping the original part hidden. If I was proposing such feature I wouldn't use 2 lists. Instead, use only a dynamic list and the default value is the current list. I would also add a check to avoid someone from removing the original list (I don't have a strong opinion if we should allow removing some of these original parameters from the list.)

Without changing the SHOW commands in breaking ways we cannot allow removing values from the original list. Because all the values are shown in some of the columns there. The application_name is needed even more than just for SHOW output, because it's used by the application_name_add_host config option.

So while I agree two lists is a bit confusing and might not seem very clean from a code design perspective. On the other hand, from a user perspective it seems quite annoying to be forced to prefix a value in your config with a (pretty long) required list. For instance only adding intervalstyle would need to look like this:

track_startup_parameters = application_name, client_encoding, datestyle, timezone, standard_conforming_strings, intervalstyle

The main advantage of doing this seems to me having the possibility to allow removal from this list in the future. I'll think a bit longer if I think that's worth the configuration hassle.

emelsimsek · 2023-06-21T11:58:50Z

Regarding the track_startup_parameters, it is described as:

By default, PgBouncer tracks client_encoding, datestyle, timezone, standard_conforming_strings
and application_name parameters per client. To allow other parameters to be tracked, they can be
specified here, so that PgBouncer knows that they should be maintained in the client variable cache
and restored in the server whenever the client becomes active.

It means there is an list that is part of tracked parameters but these parameters aren't part of the default value. These parameters are part of an implementation detail but your PR is exposing it but keeping the original part hidden. If I was proposing such feature I wouldn't use 2 lists. Instead, use only a dynamic list and the default value is the current list. I would also add a check to avoid someone from removing the original list (I don't have a strong opinion if we should allow removing some of these original parameters from the list.)

This is actually in line with ignore_startup_parameters in the way it has an immutable internal default list which can be extended via configuration. The configuration can only add to the default list.

What do you think about the name track_startup_parameters? Not every postgres parameter can be part of connection string:
https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING:~:text=for%20different%20hosts.-,34.1.2.%C2%A0Parameter%20Key%20Words,-The%20currently%20recognized

In fact IntervalStyle is not a startup parameter.

Should we rename track_startup_parameters to something else, e.g. track_postgres_parameters?

src/main.c

src/varcache.c

doc/config.md

etc/pgbouncer.ini

src/varcache.c

It's allowed to specify any Postgres command line parameter in the `options` startup packet #867. This is commonly used to specify GUC values on connection startup, using the `-c key=value` command line flag. I believe the main reason this is used instead of specifying the desired GUC as a startup parameter directly is because libpq does not allow specifying arbitrary startup parameters. This PR adds support for configuring GUCs using the `-c` flag in the options startup packet. While in theory people can specify any flag they want. In practice the `-c` flag is the only one that I know is commonly used. Probably because it is the only one that's documented in the Postgres docs for `PGOPTIONS`. Any of the other command line flags are simply shorthands for specific GUCs, and so even when those would be used they can be easily rewritten using `-c`. So to keep parsing of the options startup parameter simple, this only supports the `-c` flag. Fixes #875

This reverts commit 8c18fc4.

ReganRyanNZ · 2024-02-29T21:30:11Z

Does this mean I can run pg_dump with pgbouncer now? In the past there were issues with search_path being set and things breaking. So now that it's cached per connection...?

JelteF · 2024-03-03T14:48:15Z

If you have the Citus extension installed and have set track_extra_parameters = search_path, IntervalStyle, then yes. Otherwise you'll need to create your own extension that marks search_path as GUC_REPORT or wait for some of my relevant postgres patches to be merged and released.

emelsimsek marked this pull request as draft June 12, 2023 12:24

JelteF reviewed Jun 12, 2023

View reviewed changes

include/varcache.h Outdated Show resolved Hide resolved

emelsimsek requested a review from JelteF June 14, 2023 13:14