Re: Report search_path value back to the client.

I wanted to revive this thread, since it's by far one of the most
common foot guns that people run into with PgBouncer. Almost all
session level SET commands leak across transactions, but SET
search_path is by far the one with the biggest impact when it is not
the setting that you expect. As well as being a very common setting to
change. In the Citus extension we actually change search_path to be
GUC_REPORT so that PgBouncer can track it, because otherwise Citus its
schema based sharding starts breaking completely when using PgBouncer.
[1]

On Fri, 3 Nov 2023 at 09:38, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I'm against this on a couple of different grounds:
>
> 1. Performance. ...
>
> 2. Semantics. The existing GUC_REPORT variables are all things that
> directly relate to client-visible behavior, eg how values of type
> timestamp will be interpreted and printed. search_path is no such thing,

>
> We could possibly alleviate problem #1 by changing the behavior of guc.c
> so it doesn't report every single transition of flagged variables, but
> only (say) once just before ReadyForQuery if the variable changed in
> the just-finished command. That's not exactly a one-line fix though.

The proposed fix for #1 has been committed since PG14 in
2432b1a04087edc2fd9536c7c9aa4ca03fd1b363

So that only leaves #2. I think search_path can arguably be considered
to be client visible, because it changes how the client and its
queries are parsed by Postgres. And even if you don't agree with that
argument, it's simply not true that the only GUCs that are GUC_REPORT
are related to client-visible behaviour. Things like application_name
and is_superuser are also GUC_REPORT [2].

> so it's hard to make a principled argument for reporting it that doesn't
> lead to the conclusion that you want *everything* reported. (In
> particular, I don't believe at all your argument that this would help
> pgbouncer significantly.)

To be clear, I would like it to be configurable which settings are
GUC_REPORT. Because there are others that are useful for PgBouncer to
track (e.g. statement_timeout and lock_timeout) That's why I've been
active on the thread proposing such a change [3]. But that thread has
not been getting a lot of attention, I guess because it's a large
change that includes protocol changes. So that's why I'm reviving this
thread again. Since search_path is by far the most painful setting for
PgBouncer users. A common foot-gun is that running pg_dump causes
breakage for other clients, because its "SET search_path" is leaked to
the next transaction [4].

[1]: https://github.com/citusdata/citus/blob/21646ca1e96175370be1472a14d5ab1baa55b471/src/backend/distributed/shared_library_init.c#L2686-L2695
[2]: https://www.postgresql.org/docs/15/protocol-flow.html#PROTOCOL-ASYNC
[3]: https://www.postgresql.org/message-id/flat/0a6f5e6b-65b3-4272-260d-a17ce8f5b3a4%40eisentraut.org#bd6d06db7db9e4ded8100e4160050b17
[4]: https://github.com/pgbouncer/pgbouncer/issues/452