Re: Report search_path value back to the client.

On 11/3/23 10:06, Jelte Fennema-Nio wrote:
> 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@sss.pgh.pa.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].
> 

Not sure about whether search_path should be considered client-visible.
It's true most GUCs either don't affect query execution, or affect how
things are executed, but not what results are produced. Which
search_path does, so in this sense it is "client visible".

That being said, it this makes using pgbouncer easier (or even possible
in some applications where it currently does not work), I'd vote to get
this committed.

>> 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].
> 

So, did that other patch move forward, in some way? The last message is
from January, I'm not sure I want to read through that thread just to
find out it's stuck on something.

Also, I recall we had a session at pgconf.dev to discuss this protocol
stuff. I don't remember what the conclusions from that part were :-(

Stupid idea - could we have a GUC/function/something to define which
GUCs the client wants to get reports for? Maybe that'd be simpler and
would not require protocol changes? Not as pretty, of course, and maybe
it has some fatal flaw.


In any case, I think it'd be good to decide what to do with this patch.
Whether to reject it or get it committed, even if we hope to get a
better / extensible solution in the future. I'd vote to commit.

What concerns me a little bit is if this will make our life more
complicated in the future. I mean, imagine we get it committed, and then
get the protocol stuff later. Wouldn't that mean pgbouncer needs to do
three different things, depending on the server version? (without
search_path reporting, with reporting and with the new protocol stuff?)


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company