Re: Report search_path value back to the client.

On 7/19/24 17:16, Jelte Fennema-Nio wrote:
> On Thu, 18 Jul 2024 at 22:47, Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>> 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.
> 
> It definitely is a pain point for some setups. A speaker brought it up
> at FOSDEM pgday too, the speaker was talking about multi-tenant/SaaS
> applications.
> 
>> 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.
> 
> Basically, the discussion continued on this commitfest entry:
> https://commitfest.postgresql.org/48/4736/
> 
> That's been moving forward, even relatively fast imho for the
> size/impact of that patchset. But those changes are much less
> straight-forward than this patch. And while I hope that they can get
> committed for PG18 this year, I'm not confident about that.
> 

I don't know. My experience is that whenever we decided to not do a
simple improvement because a more complex patch was expected to do
something better/smarter, we often ended up getting nothing.

So maybe it'd be best to get this committed, and then if the other patch
manages to get into PG18, we can revert this change (or rather that
patch would do that).

>> 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.
> 
> The GUC idea was proposed before, but that has the flaw that a user
> with SQL access can change this GUC without the client-library or
> pooler realizing. Maybe that's okay, especially if it's only additive
> to the current defaults (e.g. removing client_encoding from the list
> is bound to cause issues for many clients). Basically the protocol
> change is to add support for protocol parameters, which can only be
> set at the protocol level and not the SQL level. One of such protocol
> parameters would then have the same behaviour as the GUC that you're
> describing (except that you cannot change the value using SQL).
> 

Maybe it's crazy-talk, but couldn't we make the GUC settable exactly
once? That should be doable using the GUC hooks, I think. That means
pgbouncer would set the GUC right after opening the connection, and then
the following attempts to set it would either error out, or silently do
nothing?

Perhaps it could even allow enabling reporting for more GUCs, but once a
GUC is on the list, it's reported forever.

>> 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.
> 
> Sounds good to me.
> 
>> 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?)
> 
> For PgBouncer (or other clients) its perspective I don't see any
> issues here. The other protocol stuff would still reuse the same
> messages, and so PgBouncer can track the search_path if it receives
> the update for the search_path (and not track it if it doesn't). So
> imho there are basically no downsides to committing this patch. The
> only impact it has is that if people change their search_path in a
> query, then they'll get slightly more network traffic back than before
> this patch.

OK

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