Thread

Commits

Document that search_path is reported by the server
- 0d06a7eac4b2 18.0 landed
Mark search_path as GUC_REPORT
- 28a1121fd912 18.0 landed
Avoid spamming the client with multiple ParameterStatus messages.
- 2432b1a04087 14.0 cited
Mark application_name as GUC_REPORT so that the value will be reported back
- 59ed94ad0c9f 9.0.0 cited

Report search_path value back to the client.

Alexander Kukushkin <cyberdemn@gmail.com> — 2014-12-02T16:59:53Z

Hi,

As of now postgres is reporting only "really" important variables, but
among them
there are also not so important, like 'application_name'. The only reason
to report
it, was: "help pgbouncer, and perhaps other connection poolers".
Change was introduced by commit: 59ed94ad0c9f74a3f057f359316c845cedc4461e

This fact makes me wonder, why 'search_path' value is not reported back to
the
client? Use-case is absolutely the same as with 'application_name' but a
little bit
more important.

Our databases provides different version of stored procedures which are
located
in a different schemas: 'api_version1', 'api_version2', 'api_version5',
etc...
When application establish connection to the database it set search_path
value
for example to api_version1. At the same time, new version of the same
application
will set search_path value to api_version2. Sometimes we have hundreds of
instances of applications which may use different versions of stored
procedures
which are located in different schemas.

It's really crazy to keep so many (hundreds) connections to the database and
it would be much better to have something like pgbouncer in front of
postgres.
Right now it's not possible, because pgbouncer is not aware of search_path
and it's not really possible to fix pgbouncer, because there is no easy way
to
get current value of search_path.

I would like to mark 'search_path' as GUC_REPORT:
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2904,7 +2904,7 @@ static struct config_string ConfigureNamesString[] =
                {"search_path", PGC_USERSET, CLIENT_CONN_STATEMENT,
                        gettext_noop("Sets the schema search order for
names that are not schema-qualified."),
                        NULL,
-                       GUC_LIST_INPUT | GUC_LIST_QUOTE
+                       GUC_LIST_INPUT | GUC_LIST_QUOTE | GUC_REPORT
                },
                &namespace_search_path,
                "\"$user\",public",

What do you think?

Regards,
--
Alexander Kukushkin

Re: Report search_path value back to the client.

Oleksii Kliukin <alexk@hintbits.com> — 2015-02-20T09:15:56Z

Hi,

On Tue, Dec 2, 2014 at 5:59 PM, Alexander Kukushkin <cyberdemn@gmail.com> wrote:

> It's really crazy to keep so many (hundreds) connections to the database and
> it would be much better to have something like pgbouncer in front of
> postgres.
> Right now it's not possible, because pgbouncer is not aware of search_path
> and it's not really possible to fix pgbouncer, because there is no easy way
> to
> get current value of search_path.
>
> I would like to mark 'search_path' as GUC_REPORT:
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -2904,7 +2904,7 @@ static struct config_string ConfigureNamesString[] =
>                 {"search_path", PGC_USERSET, CLIENT_CONN_STATEMENT,
>                         gettext_noop("Sets the schema search order for names
> that are not schema-qualified."),
>                         NULL,
> -                       GUC_LIST_INPUT | GUC_LIST_QUOTE
> +                       GUC_LIST_INPUT | GUC_LIST_QUOTE | GUC_REPORT
>                 },
>                 &namespace_search_path,
>                 "\"$user\",public",
>

I know quite a lot of companies using search_path to switch to the
version of sprocs in the database
without changes to the application. Not being able to use pgbouncer in
such scenario is rather annoying.

It would be really great to have this functionality in the upcoming
9.5 and not wait for another year for it.

Given this is a one-liner, which doesn't introduce any new code, but
one flag to the function call, would it be
possible to review it as a part of the current commitfest?

Kind regards,
-- 
Alexey Klyukin

Re: Report search_path value back to the client.

Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-20T15:19:12Z

Alexey Klyukin <alexk@hintbits.com> writes:
> On Tue, Dec 2, 2014 at 5:59 PM, Alexander Kukushkin <cyberdemn@gmail.com> wrote:
>> I would like to mark 'search_path' as GUC_REPORT:

> Given this is a one-liner, which doesn't introduce any new code, but
> one flag to the function call, would it be
> possible to review it as a part of the current commitfest?

I'm against this on a couple of different grounds:

1. Performance.  search_path is something that many applications change
quite a lot, so reporting changes in it would create enormous network
overhead.  Consider for example that a SQL function might set it as
part of a function SET clause, and that could be invoked thousands of
times per query.

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,
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.)

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.

			regards, tom lane

Re: Report search_path value back to the client.

Alexander Kukushkin <cyberdemn@gmail.com> — 2015-03-02T13:55:11Z

Hi,

2015-02-20 16:19 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:

> 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,
> 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.)
>

Well, I will try to explain in details why do we want to use pgbouncer in
front of postgres and what stops us from doing this.
Usually our applications don't access data directly but via stored
procedures. One database can be accessed by different applications in
different versions. Different versions of applications may use different
sets and versions of stored procedures. It achieved by keeping stored
procedures in different schemas (we call them API schema). For example at
the same time 'app-v1' is working with stored procedures located in schema
'app_api_v1', 'app-v2' with 'app_api_v2' and 'other-app-v3' is working with
'other_app_api_v3'. This is very convenient. Before deploying new version
of application we create new 'api schema' for it and then create all stored
procedures in this schema. Directly after connecting to database
application executes 'set search_path to app_api_vX, public' and after this
point it always call stored procedures without specifying schema name. This
way we are able to keep different versions of the same stored procedure in
database.
And here the problem come. Sometimes we have dozens of application
instances. Each application instance might keep more than one connection to
database. In total there can be hundreds connections but most of the time
they are not in state 'active'. For such cases connection pools have been
invented. It's cheep to keep even thousands open connections to event-based
connection pool comparing with the real database. The first idea that comes
into mind - lets put pgbouncer with pool_mode = transaction in front of
database. But here our good old friend 'set search_path to app_api_vX,
public' does weird things with pgbouncer. The same server connection might
be reused by different applications in different versions and soon or later
application will get: 'ERROR:  function foo() does not exist' because
search_path for this connection between pgbouncer and postgres has been
changed by another application.

My first idea was - it should be possible to fix pgbouncer and make it
aware of current search_path value of connections between application <->
pgbouncer and pgbouncer <-> postgres. I started to look into it's source
code and yea, this can be done relatively easy, because pgbouncer already
does similar job for "client_encoding", "DateStyle", "TimeZone",
"standard_conforming_strings" and "application_name", which doesn't relate
to the 'application visible behaviour' more than the search_path. Pgbouncer
always keeps current values of these variables for each connection and when
it takes a connection from the pool, it compares the values of
client_connection and server_connection parameters. If they don't match -
it changes the value on the server side by calling 'set variable to
client_value'.

Execution of statement 'set some_variable to some_value' from the client
side is not the unique way to change value of variable. As you already
mentioned such statement can be executed from stored procedure as well. So
scanning of client statements is not an option. Only postgres is able to
tell at any time current value of search_path and asking it every time for
current value also is not an option, because this will double amount of
queries executed by database. Luckily, postgres already has this wonderful
mechanism as GUC_REPORT...

If the postgres will report the value of search_path on connect and on
change - pgbouncer could be really easy patched:

diff --git a/include/varcache.h b/include/varcache.h
index 4984b01..916fa01 100644
--- a/include/varcache.h
+++ b/include/varcache.h
@@ -5,6 +5,7 @@ enum VarCacheIdx {
        VTimeZone,
        VStdStr,
        VAppName,
+       VSearchPath,
        NumVars
 };

diff --git a/lib b/lib
index fe4dd02..c0111d4 160000
--- a/lib
+++ b/lib
@@ -1 +1 @@
-Subproject commit fe4dd0255ea796e64599c0fc28e6900fbc07e6aa
+Subproject commit c0111d42712202df5ccb01c4e07e88ab5c8b94b8
diff --git a/src/varcache.c b/src/varcache.c
index 6321dc5..cb3f559 100644
--- a/src/varcache.c
+++ b/src/varcache.c
@@ -35,6 +35,7 @@ static const struct var_lookup lookup [] = {
  {"TimeZone",                    VTimeZone },
  {"standard_conforming_strings", VStdStr },
  {"application_name",            VAppName },
+ {"search_path",                 VSearchPath },
  {NULL},
 };

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

Probably for some variables this really make sense. Inside stored
procedures any of GUC_REPORT variables can be changed the same way as
search_path (thousands of times), but not all of these variables directly
relate to the application visible behaviour.

Regards,
Alexander Kukushkin

Re: Report search_path value back to the client.

Jelte Fennema <me@jeltef.nl> — 2023-11-03T09:06:45Z

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

> 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

Re: Report search_path value back to the client.

Jelte Fennema-Nio <postgres@jeltef.nl> — 2023-11-03T09:22:35Z
```
For completeness attached is a tiny patch implementing this, so this
thread can be added to the commit fest.
```

Re: Report search_path value back to the client.

Tomas Vondra <tomas.vondra@enterprisedb.com> — 2024-07-18T20:47:30Z

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

Re: Report search_path value back to the client.

Jelte Fennema <me@jeltef.nl> — 2024-07-19T15:16:19Z

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.

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

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

Re: Report search_path value back to the client.

Tomas Vondra <tomas.vondra@enterprisedb.com> — 2024-07-19T19:48:10Z


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

Re: Report search_path value back to the client.

Jelte Fennema <me@jeltef.nl> — 2024-07-20T12:09:30Z

On Fri, 19 Jul 2024 at 21:48, Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
>
>
> On 7/19/24 17:16, Jelte Fennema-Nio wrote:
> > 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).

Yeah, I think we're aligned on this. Because that's totally what I
meant to say, but I don't seem to have written down that conclusion
explicitly.

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

This could maybe work, I'll think a bit about it. The main downside I
see is that PgBouncer can then not act as a transparent proxy, because
it cannot reset value to the value that the client expects the GUC to
be. But in practice that doesn't matter much for this specific case
because all that happens in this case is that the client gets a few
more ParameterStatus messages that it did not want.

Re: Report search_path value back to the client.

Tomas Vondra <tomas.vondra@enterprisedb.com> — 2024-07-20T23:32:46Z

On 7/20/24 14:09, Jelte Fennema-Nio wrote:
> On Fri, 19 Jul 2024 at 21:48, Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>>
>>
>> On 7/19/24 17:16, Jelte Fennema-Nio wrote:
>>> 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).
> 
> Yeah, I think we're aligned on this. Because that's totally what I
> meant to say, but I don't seem to have written down that conclusion
> explicitly.
> 
>> 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.
> 
> This could maybe work, I'll think a bit about it. The main downside I
> see is that PgBouncer can then not act as a transparent proxy, because
> it cannot reset value to the value that the client expects the GUC to
> be. But in practice that doesn't matter much for this specific case
> because all that happens in this case is that the client gets a few
> more ParameterStatus messages that it did not want.

I understand that point of view, but I don't quite share it. I don't
really see this as a GUC, even if it's implemented as one. It's just a
convenient way to enable a protocol feature without having to modify the
protocol ... It could easily be a enable_param_report() function, doing
the same thing, for example.

It's a bit of a tangent, but this reminds me how Oracle uses logon
triggers to set "application context" for their variant of RLS. In
postgres that was not possible because we did not have "logon" triggers,
but in 17 that will change. I wonder it that could be handy, but maybe
not - it seems unnecessarily complex.


regards

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

Re: Report search_path value back to the client.

Tomas Vondra <tomas@vondra.me> — 2024-08-14T16:22:37Z

Here's the patch with a somewhat expanded / improved commit message.
Jelte, can you take a look there's no silly mistake?

As mentioned previously, I plan to push this, so that if the protocol
improvements from [1] don't land in PG18 we have at least something. I
did take a brief look at the other thread, but it's hard to predict.

[1]
https://www.postgresql.org/message-id/CAGECzQTg2hcmb5GaU53uuWcdC7gCNJFLL6mnW0WNhWHgq9UTgw%40mail.gmail.com

regards

-- 
Tomas Vondra

Re: Report search_path value back to the client.

Jelte Fennema <me@jeltef.nl> — 2024-08-14T16:30:52Z

On Wed, 14 Aug 2024 at 18:22, Tomas Vondra <tomas@vondra.me> wrote:
> Here's the patch with a somewhat expanded / improved commit message.
> Jelte, can you take a look there's no silly mistake?

Looks good to me.

Re: Report search_path value back to the client.

Tomas Vondra <tomas@vondra.me> — 2024-08-19T15:20:57Z

On 8/14/24 18:30, Jelte Fennema-Nio wrote:
> On Wed, 14 Aug 2024 at 18:22, Tomas Vondra <tomas@vondra.me> wrote:
>> Here's the patch with a somewhat expanded / improved commit message.
>> Jelte, can you take a look there's no silly mistake?
> 
> Looks good to me.

Pushed, after rewording the commit message a bit.

Now let's see how the other protocol thread goes ...


regards

-- 
Tomas Vondra

Re: Report search_path value back to the client.

Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-19T16:02:33Z

Tomas Vondra <tomas@vondra.me> writes:
> On 8/14/24 18:30, Jelte Fennema-Nio wrote:
>> Looks good to me.

> Pushed, after rewording the commit message a bit.

This patch does not appear to have updated any of the relevant
documentation.

			regards, tom lane

Re: Report search_path value back to the client.

Tomas Vondra <tomas@vondra.me> — 2024-08-19T16:10:58Z

On 8/19/24 18:02, Tom Lane wrote:
> Tomas Vondra <tomas@vondra.me> writes:
>> On 8/14/24 18:30, Jelte Fennema-Nio wrote:
>>> Looks good to me.
> 
>> Pushed, after rewording the commit message a bit.
> 
> This patch does not appear to have updated any of the relevant
> documentation.
> 

Oh, I haven't realized we explicitly list which parameters are reported.

I see there are two places in libpq.sgml and protocol.sgml that should
list search_path - will fix. I haven't found any other place in the docs
that would need an update, or did I miss something?

regards

-- 
Tomas Vondra

Re: Report search_path value back to the client.

Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-19T16:19:07Z

Tomas Vondra <tomas@vondra.me> writes:
> I see there are two places in libpq.sgml and protocol.sgml that should
> list search_path - will fix. I haven't found any other place in the docs
> that would need an update, or did I miss something?

I recall there being two places, so that's probably the extent of it
... yeah, the last similar patch was d16f8c8e4, and that's what
it did.

			regards, tom lane

Re: Report search_path value back to the client.

Tomas Vondra <tomas@vondra.me> — 2024-08-19T17:55:09Z

On 8/19/24 18:19, Tom Lane wrote:
> Tomas Vondra <tomas@vondra.me> writes:
>> I see there are two places in libpq.sgml and protocol.sgml that should
>> list search_path - will fix. I haven't found any other place in the docs
>> that would need an update, or did I miss something?
> 
> I recall there being two places, so that's probably the extent of it
> ... yeah, the last similar patch was d16f8c8e4, and that's what
> it did.
> 

Thanks, updated in a similar way.

-- 
Tomas Vondra

Re: Report search_path value back to the client.

Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-19T17:57:59Z

Tomas Vondra <tomas@vondra.me> writes:
> On 8/19/24 18:19, Tom Lane wrote:
>> I recall there being two places, so that's probably the extent of it
>> ... yeah, the last similar patch was d16f8c8e4, and that's what
>> it did.

> Thanks, updated in a similar way.

LGTM, thanks.

			regards, tom lane