Thread: proposal: psql: show current user in prompt

Hi

one visitor of p2d2 (Prague PostgreSQL Developer Day) asked if it is possible to show the current role in psql's prompt. I think it is not possible, but fortunately (with some limits) almost all necessary work is done, and the patch is short.

In the assigned patch I implemented a new prompt placeholder %N, that shows the current role name.

(2023-02-03 15:52:28) postgres=# \set PROMPT1 '%n as %N at '%/%=%#
pavel as pavel at postgres=#set role to admin;
SET
pavel as admin at postgres=>set role to default;
SET
pavel as pavel at postgres=#

Comments, notes are welcome.

Regards

Pavel

On Fri, Feb 3, 2023 at 9:56 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

one visitor of p2d2 (Prague PostgreSQL Developer Day) asked if it is possible to show the current role in psql's prompt. I think it is not possible, but fortunately (with some limits) almost all necessary work is done, and the patch is short.

In the assigned patch I implemented a new prompt placeholder %N, that shows the current role name.

(2023-02-03 15:52:28) postgres=# \set PROMPT1 '%n as %N at '%/%=%#
pavel as pavel at postgres=#set role to admin;
SET
pavel as admin at postgres=>set role to default;
SET
pavel as pavel at postgres=#

Comments, notes are welcome.

Regards

Pavel

This patch is cluttered with the BACKEND_PID patch and some guc_tables.c stuff that I don't think is related.

We'd have to document the %N.

I think there is some value here for people who have to connect as several different users (tech support), and need a reminder-at-a-glance whether they are operating in the desired role. It may be helpful in audit documentation as well.

Pavel Stehule <pavel.stehule@gmail.com> writes:
> Both patches are very simple - and they use almost already prepared
> infrastructure.

It's not simple at all to make the psql feature depend on marking
"role" as GUC_REPORT when it never has been before.  That will
cause the feature to misbehave when using older servers.  I'm
even less impressed by having it fall back on PQuser(), which
would be misleading at exactly the times when it matters.

                        regards, tom lane

pá 3. 2. 2023 v 21:21 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> writes:
> Both patches are very simple - and they use almost already prepared
> infrastructure.

It's not simple at all to make the psql feature depend on marking
"role" as GUC_REPORT when it never has been before.  That will
cause the feature to misbehave when using older servers.  I'm
even less impressed by having it fall back on PQuser(), which
would be misleading at exactly the times when it matters.

It is a good note. This can be disabled for older servers, and maybe it can  introduce its own GUC (and again - it can be disallowed for older servers).

My goal at this moment is to get some prototype. We can talk if this feature request is valid or not, and we can talk about implementation.

There is another possibility to directly execute "select current_user()" instead of looking at status parameters inside prompt processing. It can work too.

Regards

Pavel

                        regards, tom lane

Hi

pá 3. 2. 2023 v 21:43 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:

pá 3. 2. 2023 v 21:21 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> writes:
> Both patches are very simple - and they use almost already prepared
> infrastructure.

It's not simple at all to make the psql feature depend on marking
"role" as GUC_REPORT when it never has been before.  That will
cause the feature to misbehave when using older servers.  I'm
even less impressed by having it fall back on PQuser(), which
would be misleading at exactly the times when it matters.

It is a good note. This can be disabled for older servers, and maybe it can  introduce its own GUC (and again - it can be disallowed for older servers).

Here is another version. For older servers it shows the string ERR0A000. That is  ERR code of "feature is not supported"

My goal at this moment is to get some prototype. We can talk if this feature request is valid or not, and we can talk about implementation.

There is another possibility to directly execute "select current_user()" instead of looking at status parameters inside prompt processing. It can work too.

I tested using the query SELECT CURRENT_USER, but I don't think it is usable now, because it doesn't work in the broken transaction.

Regards

Pavel

 

Regards

Pavel

                        regards, tom lane

Kirk Wolak <wolakk@gmail.com> writes:
> Changed status to Ready for Committer. (100% Guessing here...)

Basically, I want to reject this on the grounds that it's not
useful enough to justify the overhead of marking the "role" GUC
as GUC_REPORT.  The problems with it not going to work properly
with old servers are an additional reason not to like it.

But, if I lose the argument and we do commit this, I think it
should just print an empty string when dealing with an old server.
"ERR02000" is an awful idea, not least because it could be a
real role name.

BTW, we should probably get rid of the PQuser() fallback in
%n (session_username()) as well.  It's unlikely that there are
still servers in the wild that don't report "session_authorization",
but if we did find one then the output is potentially misleading.
I'd rather print nothing than something that might not be your
actual session authorization setting.

                        regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
> út 4. 4. 2023 v 18:42 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> Basically, I want to reject this on the grounds that it's not
>> useful enough to justify the overhead of marking the "role" GUC
>> as GUC_REPORT.  The problems with it not going to work properly
>> with old servers are an additional reason not to like it.

> If I understand to next comment correctly, the overhead should not be too
> big

Yeah, but how big is the use-case?  The reason I'm skeptical is that
half the time what you're going to get is "none":

$ psql
psql (16devel)
Type "help" for help.

regression=# show role;
 role
------
 none
(1 row)

That's required by SQL spec I believe, but that doesn't make it useful
data to keep in one's prompt.

                        regards, tom lane

út 4. 4. 2023 v 19:55 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> writes:
> út 4. 4. 2023 v 18:42 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> Basically, I want to reject this on the grounds that it's not
>> useful enough to justify the overhead of marking the "role" GUC
>> as GUC_REPORT.  The problems with it not going to work properly
>> with old servers are an additional reason not to like it.

> If I understand to next comment correctly, the overhead should not be too
> big

Yeah, but how big is the use-case?  The reason I'm skeptical is that
half the time what you're going to get is "none":

$ psql
psql (16devel)
Type "help" for help.

regression=# show role;
 role
------
 none
(1 row)

That's required by SQL spec I believe, but that doesn't make it useful
data to keep in one's prompt.

Who needs it, and who uses different roles, then very quickly uses SET ROLE TO command.

But I fully agree so current behavior can be a little bit messy. I like this feature, and I think it can have some benefits. Proposed implementation is minimalistic. 

One hard problem is translation of the oid of current_user to name. It requires an opened transaction, and then it cannot be postponed to the end of the statement. On the other hand, when the change of role is done inside a nested command, then it should not be visible from the client side.

Can you accept the introduction of a new invisible GUC, that can be modified only by SET ROLE TO command when it is executed as top command?

Regards

Pavel

 

                        regards, tom lane

út 4. 4. 2023 v 20:50 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:

út 4. 4. 2023 v 19:55 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> writes:
> út 4. 4. 2023 v 18:42 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> Basically, I want to reject this on the grounds that it's not
>> useful enough to justify the overhead of marking the "role" GUC
>> as GUC_REPORT.  The problems with it not going to work properly
>> with old servers are an additional reason not to like it.

> If I understand to next comment correctly, the overhead should not be too
> big

Yeah, but how big is the use-case?  The reason I'm skeptical is that
half the time what you're going to get is "none":

$ psql
psql (16devel)
Type "help" for help.

regression=# show role;
 role
------
 none
(1 row)

That's required by SQL spec I believe, but that doesn't make it useful
data to keep in one's prompt.

Who needs it, and who uses different roles, then very quickly uses SET ROLE TO command.

But I fully agree so current behavior can be a little bit messy. I like this feature, and I think it can have some benefits. Proposed implementation is minimalistic. 

One hard problem is translation of the oid of current_user to name. It requires an opened transaction, and then it cannot be postponed to the end of the statement. On the other hand, when the change of role is done inside a nested command, then it should not be visible from the client side.

Can you accept the introduction of a new invisible GUC, that can be modified only by SET ROLE TO command when it is executed as top command?

It was stupid idea.

There can be implemented fallbacks. When the role is "none", then the :USER can be displayed instead.

It can work, because the custom role "none" is not allowed

(2023-04-04 21:10:25) postgres=# create role none;
ERROR:  role name "none" is reserved
LINE 1: create role none;

?

 

Regards

Pavel

 

                        regards, tom lane

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 4, 2023 at 12:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Basically, I want to reject this on the grounds that it's not
>> useful enough to justify the overhead of marking the "role" GUC
>> as GUC_REPORT.

> I agree with that. I think we need some method for optionally
> reporting values, so that stuff like this can be handled without
> adding it to the wire protocol for everyone.

It could probably be possible to provide some mechanism for setting
GUC_REPORT on specific variables locally within sessions.  I don't
think this'd be much of a protocol-break problem, because clients
should already be coded to deal gracefully with ParameterStatus messages
for variables they don't know.  However, connecting that up to something
like a psql prompt feature would still be annoying.  I doubt I'd want
to go as far as having psql try to turn on GUC_REPORT automatically
if it sees %N in the prompt ...

                        regards, tom lane

On Wed, Apr 5, 2023 at 9:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Tue, Apr 4, 2023 at 12:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Basically, I want to reject this on the grounds that it's not
> >> useful enough to justify the overhead of marking the "role" GUC
> >> as GUC_REPORT.
>
> > I agree with that. I think we need some method for optionally
> > reporting values, so that stuff like this can be handled without
> > adding it to the wire protocol for everyone.
>
> It could probably be possible to provide some mechanism for setting
> GUC_REPORT on specific variables locally within sessions.  I don't
> think this'd be much of a protocol-break problem, because clients
> should already be coded to deal gracefully with ParameterStatus messages
> for variables they don't know.  However, connecting that up to something
> like a psql prompt feature would still be annoying.  I doubt I'd want
> to go as far as having psql try to turn on GUC_REPORT automatically
> if it sees %N in the prompt ...

Oh, I had it in mind that it would do exactly that. And I think that
should be mediated by a wire protocol message, not a GUC, so that
users don't mess things up for psql or other clients -- in either
direction -- via SET commands.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Apr 5, 2023 at 11:34 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> If the GUC_REPORT should not  be used, then only one possibility is enhancing the protocol, about the possibility to read some predefined server's features from the client.
> It can be much cheaper than SQL query, and it can be used when the current transaction is aborted. I can imagine a possibility to read server time or a server session role from a prompt processing routine.
>
> But for this specific case, you need to cache the role name somewhere. You can simply get oid everytime, but for role name you need to access to system catalogue, and it is not possible in aborted transactions. So at the end, you probably should read "role" GUC.
>
> Can this design be  acceptable?

I don't think we want to add a dedicated protocol message that says
"send me the role GUC right now". I mean, we could, but being able to
tell the GUC mechanism "please send me the role GUC after every
command" sounds a lot easier to use.

--
Robert Haas
EDB: http://www.enterprisedb.com

st 5. 4. 2023 v 17:47 odesílatel Robert Haas <robertmhaas@gmail.com> napsal:

On Wed, Apr 5, 2023 at 11:34 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> If the GUC_REPORT should not  be used, then only one possibility is enhancing the protocol, about the possibility to read some predefined server's features from the client.
> It can be much cheaper than SQL query, and it can be used when the current transaction is aborted. I can imagine a possibility to read server time or a server session role from a prompt processing routine.
>
> But for this specific case, you need to cache the role name somewhere. You can simply get oid everytime, but for role name you need to access to system catalogue, and it is not possible in aborted transactions. So at the end, you probably should read "role" GUC.
>
> Can this design be  acceptable?

I don't think we want to add a dedicated protocol message that says
"send me the role GUC right now". I mean, we could, but being able to
tell the GUC mechanism "please send me the role GUC after every
command" sounds a lot easier to use.

I'll try it

Regards

Pavel

 

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi

rebased version + fix warning possibly uninitialized variable

Regards

Pavel

I'm very much in favor of adding a way to support reporting other GUC
variables than the current hardcoded list. This can be quite useful to
support some amount of session state in connection poolers.

Some general feedback on the patch:
1. I don't think the synchronization mechanism that you added should
be part of PQlinkParameterStatus. It seems likely for people to want
to turn on reporting for multiple GUCs in one go. Having to
synchronize for each would introduce unnecessary round trips. Maybe
you also don't care about syncing at all at this point in time.

2. Support for this message should probably require a protocol
extension. There is another recent thread that discusses about adding
message types and protocol extensions:
https://www.postgresql.org/message-id/flat/CA%2BTgmoaxfJ3whOqnxTjT-%2BHDgZYbEho7dVHHsuEU2sgRw17OEQ%40mail.gmail.com#acd99fde0c037cc6cb35d565329b6e00

3. Some tests for this new libpq API should be added in
src/test/modules/libpq_pipeline

4. s/massages/messages


Finally, I think this patch should be split into two commits:
1. adding custom GUC_REPORT protocol support+libpq API
2. using this libpq API in psql for the user prompt

If you have multiple commits (which are rebased on master), you can
very easily create multiple patch files using this command:
git format-patch master --base=master --reroll-count={version_number_here}

On Fri, 28 Apr 2023 at 07:00, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> čt 27. 4. 2023 v 7:39 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:
>>
>> Hi
>>
>> rebased version + fix warning possibly uninitialized variable
>
>
> fix not unique function id
>
> Regards
>
> Pavel
>
>>
>> Regards
>>
>> Pavel
>>

čt 27. 4. 2023 v 7:39 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:

Hi

rebased version + fix warning possibly uninitialized variable

fix not unique function id 

Regards

Pavel

Regards

Pavel

I'm very much in favor of adding a way to support reporting other GUC
variables than the current hardcoded list. This can be quite useful to
support some amount of session state in connection poolers.

Some general feedback on the patch:
1. I don't think the synchronization mechanism that you added should
be part of PQlinkParameterStatus. It seems likely for people to want
to turn on reporting for multiple GUCs in one go. Having to
synchronize for each would introduce unnecessary round trips. Maybe
you also don't care about syncing at all at this point in time.

2. Support for this message should probably require a protocol
extension. There is another recent thread that discusses about adding
message types and protocol extensions:
https://www.postgresql.org/message-id/flat/CA%2BTgmoaxfJ3whOqnxTjT-%2BHDgZYbEho7dVHHsuEU2sgRw17OEQ%40mail.gmail.com#acd99fde0c037cc6cb35d565329b6e00

3. Some tests for this new libpq API should be added in
src/test/modules/libpq_pipeline

4. s/massages/messages


Finally, I think this patch should be split into two commits:
1. adding custom GUC_REPORT protocol support+libpq API
2. using this libpq API in psql for the user prompt

If you have multiple commits (which are rebased on master), you can
very easily create multiple patch files using this command:
git format-patch master --base=master --reroll-count={version_number_here}


On Fri, 28 Apr 2023 at 07:00, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> čt 27. 4. 2023 v 7:39 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:
>>
>> Hi
>>
>> rebased version + fix warning possibly uninitialized variable
>
>
> fix not unique function id
>
> Regards
>
> Pavel
>
>>
>> Regards
>>
>> Pavel
>>

On Mon, 24 Jul 2023 at 21:16, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I don't understand how it can be possible to do it without.  I need to process possible errors, and then I need to read and synchronize protocol. I didn't inject
> this feature to some oher flow, so I need to implement a complete process.

I think I might be missing the reason for this then. Could you explain
a bit more why you didn't inject the feature into another flow?
Because it seems like it would be better to inserting the logic for
handling the new response packet in pqParseInput3(), and then wait on
the result with PQexecFinish(). This would allow sending these
messages in a pipelined mode.

> For example, PQsetClientEncoding does a PQexec call, which is much more expensive.

Yeah, but you'd usually only call that once for the life of the
connection. But honestly it would still be good if there was a
pipelined version of that function.

> Unfortunately, for this feature, I cannot change some local state variables, but I need to change the state of the server. Own message is necessary, because we don't want to be limited by the current transaction state, and then we cannot reuse PQexec.

I guess this is your reasoning for why it needs its own state machine,
but I don't think I understand the problem. Could you expand a bit
more on what you mean? Note that different message types use
PQexecFinish to wait for their result, e.g. PQdescribePrepared and
PQclosePrepared use PQexecFinish too and those wait for a
RowDescription and a Close message respectively. I added the logic for
PQclosePerpared recently, that patch might be some helpful example
code: https://github.com/postgres/postgres/commit/28b5726561841556dc3e00ffe26b01a8107ee654

> The API can be changed from PQlinkParameterStatus(PGconn *conn, const char *paramName)
>
> to
>
> PQlinkParameterStatus(PGconn *conn, int nParamNames, const char *paramNames)

That would definitely address the issue with the many round trips
being needed. But it would still leave the issue of introducing a
second state machine in the libpq code. So if it's possible to combine
this new code into the existing state machine, then that seems a lot
better.

On Tue, 8 Aug 2023 at 07:20, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> The reason why I implemented separate flow is usage from psql and independence of transaction state.  It is used for the \set command, that is non-transactional, not SQL. If I inject this message to some other flow, I lose this independence.

I still don't understand the issue that you're trying to solve by
introducing a new flow for handling the response. What do you mean
with independence of the transaction state? That it is not rolled-back
in a case like this?

BEGIN;
\set PROMPT '%N'
ROLLBACK;

That seems more like a Postgres server concern, i.e. don't revert the
change back on ROLLBACK. (I think your current server-side
implementation already does that)

I guess one reason that I don't understand what you mean is that libpq
doesn't really care about "transaction state" at all. It's really a
wrapper around a socket with easy functions to send messages in the
postgres protocol over it. So it cares about the "connection state",
but not really about a "transaction state". (it does track the current
connection state, but it doesn't actually use the value except when
reporting the value when PQtransactionStatus is called by the user of
libpq)

> Without independence on transaction state and SQL, I can just implement some SQL function that sets reporting for any GUC, and it is more simple than extending protocol.

I don't understand why this is not possible. As far as I can tell this
should work fine for the usecase of psql. I still prefer the protocol
message approach though, because that makes it possible for connection
poolers to intercept the message and handle it accordingly. And I see
some use cases for this reporting feature for PgBouncer as well.

However, I think this is probably the key thing that I don't
understand about the problem you're describing: So, could you explain
in some more detail why implementing a SQL function would not work for
psql?

On Thu, 10 Aug 2023 at 14:44, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> čt 10. 8. 2023 v 14:05 odesílatel Jelte Fennema <postgres@jeltef.nl> napsal:
>> That it is not rolled-back
>> in a case like this?
>>
>> BEGIN;
>> \set PROMPT '%N'
>> ROLLBACK;
>
>
> surely not.
>
> \set is client side setting, and it is not transactional. Attention - "\set" and "set" commands are absolutely different creatures.

To clarify: I agree it's the desired behavior that \set is not rolled back.

> It Would be strange (can be very messy) if I had a message like "cannot set a prompt, because you should do ROLLBACK first"

This was a very helpful sentence for my understanding. To double check
that I'm understanding you correctly. This is the kind of case that
you're talking about.

postgres=# BEGIN;
postgres=# SELECT some syntax error;
ERROR:  42601: syntax error at or near "some"
postgres=# \set PROMPT '%N'
ERROR:  25P02: current transaction is aborted, commands ignored until
end of transaction block

I agree that it should not throw an error like that. So indeed a
dedicated message type is needed for psql too. Because any query will
cause that error.

But afaict there's no problem with using pqParseInput3() and
PQexecFinish() even if the message isn't handled as part of the
transaction. Some other messages that pqParseInput3 handles which are
not part of the transaction are 'N' (Notice) and 'K' (secret key).

 

But afaict there's no problem with using pqParseInput3() and
PQexecFinish() even if the message isn't handled as part of the
transaction. Some other messages that pqParseInput3 handles which are
not part of the transaction are 'N' (Notice) and 'K' (secret key).

I have to recheck it

Regards

Pavel

 

Hi

 

But afaict there's no problem with using pqParseInput3() and
PQexecFinish() even if the message isn't handled as part of the
transaction. Some other messages that pqParseInput3 handles which are
not part of the transaction are 'N' (Notice) and 'K' (secret key).

I have to recheck it

here is new version based on usage of PQexecFinish

Regards

Pavel

 

Regards

Pavel

 

On Mon, 28 Aug 2023 at 15:00, Pavel Stehule <pavel.stehule@gmail.com> wrote:
+                       minServerMajor = 1600;
+                       serverMajor = PQserverVersion(pset.db) / 100;

Instead of using the server version, we should instead use the
protocol version negotiation that's provided by the
NegotiateProtocolVersion message type. We should bump the requested
protocol version from 3.0 to 3.1 and check that the server supports
3.1. Otherwise proxies or connection poolers might get this new
message type, without knowing what to do with them.

On Sun, 3 Sept 2023 at 08:24, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> My personal feeling from this area is that the protocol design is done, but it is not implemented on libpq level. My feelings can be wrong. The protocol number is hardcoded in libpq, so I cannot change it from the client side.


No, I agree you're right the client side code to fall back to older
versions is not implemented. But that seems fairly simple to do. We
can change pqGetNegotiateProtocolVersion3 its behaviour. That function
should change conn->pversion to the server provided version if it's
lower than the client version (as long as the server provided version
is 3.0 or larger). And then we should return an error when calling
PQlinkParameterStatus/PQunlinkParameterStatus if the pversion is not
high enough.

On Sun, 3 Sept 2023 at 20:58, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> here is an try

Overall it does what I had in mind. Below a few suggestions:

+int
+PQprotocolSubversion(const PGconn *conn)

Ugh, it's quite annoying that the original PQprotocolVersion only
returns the major version and thus we need this new function. It
seems like it would be much nicer if it returned a number similar to
PQserverVersion. I think it might be nicer to change PQprotocolVersion
to do that than to add another function. We could do:

return PG_PROTOCOL_MAJOR(conn->pversion) * 100 +
PG_PROTOCOL_MINOR(conn->pversion);

or even:

if (PG_PROTOCOL_MAJOR(conn->pversion) == 3 && PG_PROTOCOL_MINOR(conn->pversion))
    return 3;
else
    return PG_PROTOCOL_MAJOR(conn->pversion) * 100 +
PG_PROTOCOL_MINOR(conn->pversion);

The second option would be safest backwards compatibility wise, but in
practice you would only get another value than 3 (or 0) when
connecting to pre 7.4 servers. That seems old enough that I don't
think anyone is actually calling this function. **I'd like some
feedback from others on this though.**

+       /* The protocol 3.0 is required */
+       if (PG_PROTOCOL_MAJOR(their_version) == 3)
+           conn->pversion = their_version;

Let's compare against the actual PG_PROTOCOL_EARLIEST and
PG_PROTOCOL_LATEST to determine if the version is supported or not.

On Sun, 3 Sept 2023 at 20:58, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> here is an try

Overall it does what I had in mind. Below a few suggestions:

+int
+PQprotocolSubversion(const PGconn *conn)

Ugh, it's quite annoying that the original PQprotocolVersion only
returns the major version and thus we need this new function. It
seems like it would be much nicer if it returned a number similar to
PQserverVersion. I think it might be nicer to change PQprotocolVersion
to do that than to add another function. We could do:

return PG_PROTOCOL_MAJOR(conn->pversion) * 100 +
PG_PROTOCOL_MINOR(conn->pversion);

or even:

if (PG_PROTOCOL_MAJOR(conn->pversion) == 3 && PG_PROTOCOL_MINOR(conn->pversion))
    return 3;
else
    return PG_PROTOCOL_MAJOR(conn->pversion) * 100 +
PG_PROTOCOL_MINOR(conn->pversion);

The second option would be safest backwards compatibility wise, but in
practice you would only get another value than 3 (or 0) when
connecting to pre 7.4 servers. That seems old enough that I don't
think anyone is actually calling this function. **I'd like some
feedback from others on this though.**

+       /* The protocol 3.0 is required */
+       if (PG_PROTOCOL_MAJOR(their_version) == 3)
+           conn->pversion = their_version;

Let's compare against the actual PG_PROTOCOL_EARLIEST and
PG_PROTOCOL_LATEST to determine if the version is supported or not.

On Tue, 5 Sept 2023 at 05:50, Pavel Stehule <pavel.stehule@gmail.com> wrote:

> I prefer to introduce a new function - it is ten lines of code. The form is not important - it can be a full number or minor number. It doesn't matter I think. But my opinion in this area is not strong, and I like to see feedback from others too. It is true that this feature and interface is not fully complete.

I think when using the API it is nicest to have a single function that
returns the full version number. i.e. if we're introducing a new
function I think it should be PQprotocolFullVersion instead of
PQprotocolSubversion. Then instead of doing a version check like this:

if (PQprotocolVersion(pset.db) == 3 && PQprotocolSubversion(pset.db) >= 1)

You can do the simpler:

if (PQprotocolFullVersion(pset.db) >= 301)

This is also in line with how you do version checks for postgres versions.

So I think this patch should at least add that one instead of
PQprotocolSubversion. If we then decide to replace PQprotocolVersion
with this new implementation, that would be a trivial change.

>> +       /* The protocol 3.0 is required */
>> +       if (PG_PROTOCOL_MAJOR(their_version) == 3)
>> +           conn->pversion = their_version;
>>
>> Let's compare against the actual PG_PROTOCOL_EARLIEST and
>> PG_PROTOCOL_LATEST to determine if the version is supported or not.
>
>
> changed

I think we should also check the minor version part. So like this instead
+       if (their_version < PG_PROTOCOL_EARLIEST || their_version >
PG_PROTOCOL_LATEST)

PS. If you use the -v flag of git format-patch a version number is
prepended to your patches. That makes it easier to reference them.

Another thing that should be described there is that this falls
outside of the transaction flow, i.e. it's changes are not reverted on
ROLLBACK. But that leaves an important consideration: What happens
when an error occurs on the server during handling of this message
(e.g. the GUC does not exist or an OOM is triggered). Is any open
transaction aborted in that case? If not, we should have a test for
that.

+   if (PQresultStatus(res) != PGRES_COMMAND_OK)
+       pg_fatal("failed to link custom variable: %s", PQerrorMessage(conn));
+   PQclear(res);

The tests should also change the config after running
PQlinkParameterStatus/PQunlinkParameterStatus to show that the guc is
reported then or not reported then.

+   if (!PQsendTypedCommand(conn, PqMsg_ReportGUC, 't', paramName))
+       return NULL;


I think we'll need some bikeshedding on what the protocol message
should look like exactly. I'm not entirely sure what is the most
sensible here, so please treat everything I write next as
suggestions/discussion:
I see that you're piggy-backing on PQsendTypedCommand, which seems
nice to avoid code duplication. It has one downside though: not every
type, is valid for each command anymore.
One way to avoid that would be to not introduce a new command, but
only add a new type that is understood by Describe and Close, e.g. a
'G' (for GUC). Then PqMsg_Describe, G would become the equivalent of
what'the current patch its PqMsg_ReportGUC, 't' and PqMsg_Close, G
would be the same as PqMsg_ReportGUC, 'f'.

The rest of this email assumes that we're keeping your current
proposal for the protocol message, so it might not make sense to
address all of this feedback, in case we're still going to change the
protocol:

+                   if (is_set == 't')
+                   {
+                       SetGUCOptionFlag(name, GUC_REPORT);
+                       status = "SET REPORT_GUC";
+                   }
+                   else
+                   {
+                       UnsetGUCOptionFlag(name, GUC_REPORT);
+                       status = "UNSET REPORT_GUC";
+                   }

I think we should be strict about what we accept here. Any other value
than 'f'/'t' for is_set should result in an error imho.

Hi

Another thing that should be described there is that this falls
outside of the transaction flow, i.e. it's changes are not reverted on
ROLLBACK. But that leaves an important consideration: What happens
when an error occurs on the server during handling of this message
(e.g. the GUC does not exist or an OOM is triggered). Is any open
transaction aborted in that case? If not, we should have a test for
that.

I tested this scenario. I had to modify message handling to fix warning "message type 0x5a arrived from server while idle"

But if this is inside a transaction, the transaction is aborted.

+   if (PQresultStatus(res) != PGRES_COMMAND_OK)
+       pg_fatal("failed to link custom variable: %s", PQerrorMessage(conn));
+   PQclear(res);

done

 

The tests should also change the config after running
PQlinkParameterStatus/PQunlinkParameterStatus to show that the guc is
reported then or not reported then.

done

 

+   if (!PQsendTypedCommand(conn, PqMsg_ReportGUC, 't', paramName))
+       return NULL;


I think we'll need some bikeshedding on what the protocol message
should look like exactly. I'm not entirely sure what is the most
sensible here, so please treat everything I write next as
suggestions/discussion:
I see that you're piggy-backing on PQsendTypedCommand, which seems
nice to avoid code duplication. It has one downside though: not every
type, is valid for each command anymore.
One way to avoid that would be to not introduce a new command, but
only add a new type that is understood by Describe and Close, e.g. a
'G' (for GUC). Then PqMsg_Describe, G would become the equivalent of
what'the current patch its PqMsg_ReportGUC, 't' and PqMsg_Close, G
would be the same as PqMsg_ReportGUC, 'f'.

I am sorry, I don't understand this idea?

 

The rest of this email assumes that we're keeping your current
proposal for the protocol message, so it might not make sense to
address all of this feedback, in case we're still going to change the
protocol:

+                   if (is_set == 't')
+                   {
+                       SetGUCOptionFlag(name, GUC_REPORT);
+                       status = "SET REPORT_GUC";
+                   }
+                   else
+                   {
+                       UnsetGUCOptionFlag(name, GUC_REPORT);
+                       status = "UNSET REPORT_GUC";
+                   }

I think we should be strict about what we accept here. Any other value
than 'f'/'t' for is_set should result in an error imho.

done

On Fri, 8 Sept 2023 at 21:08, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> ok changed - there is minor problem - almost all PQ functions are of int type, but ProtocolVersion is uint

Your current implementation requires using the PG_PROTOCOL macros to
compare versions. But clients cannot currently use those macros since
they are not exported from libpq-fe.h, only from pqcomm.h which is
then imported by libpq-int.h. (psql/command.c imports pcomm.h
directly, but I don't think we should expect clients to do that). We
could ofcourse export these macros from libpq-fe.h too. But then we'd
need to document those macros too.

> Using different mapping to int can be problematic - can be messy if we cannot use PG_PROTOCOL macro.

I see no big problems returning an unsigned or long from the new
function (even if existing functions all returned int). But I don't
even think that is necessary. Returning the following as an int from
PQprotocolVersionFull would work fine afaict:

return PG_PROTOCOL_MAJOR(version) * 1000 + PG_PROTOCOL_MINOR(version)

This would give us one thousand minor versions for each major version.
This seems fine for all practical purposes. Since postgres only
releases a version once every year, we'd need a protocol bump every
year for one thousand years for that to ever cause any problems. So
I'd prefer this approach over making the PG_PROTOCOL macros a public
interface.

On Tue, 12 Sept 2023 at 09:46, Peter Eisentraut <peter@eisentraut.org> wrote:
> ISTM that for a purpose like pgbouncer, it would be simpler to add a new
> GUC "report these variables" and send that in the startup message?  That
> might not help with the psql use case, but it would be much simpler.

FYI I implemented it that way yesterday on this other thread (patch
0010 of that patchset):
https://www.postgresql.org/message-id/flat/CAGECzQScQ3N-Ykv2j4NDyDtrPPc3FpRoa%3DLZ-2Uj2ocA4zr%3D4Q%40mail.gmail.com#cd9e8407820d492e8f677ee6a67c21ce

On 11.09.23 13:59, Jelte Fennema wrote:
> @Tom and @Robert, since you originally suggested extending the
> protocol for this, I think some input from you on the protocol design
> would be quite helpful. BTW, this protocol extension is the main
> reason I personally care for this patch, because it would allow
> PgBouncer to ask for updates on certain GUCs so that it can preserve
> session level SET commands even in transaction pooling mode.
> Right now PgBouncer can only do this for a handful of GUCs, but
> there's quite a few others that are useful for PgBouncer to preserve
> by default:
> - search_path
> - statement_timeout
> - lock_timeout

ISTM that for a purpose like pgbouncer, it would be simpler to add a new
GUC "report these variables" and send that in the startup message?  That
might not help with the psql use case, but it would be much simpler.

On Tue, 12 Sept 2023 at 14:39, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 11.09.23 13:59, Jelte Fennema wrote:
> > @Tom and @Robert, since you originally suggested extending the
> > protocol for this, I think some input from you on the protocol design
> > would be quite helpful. BTW, this protocol extension is the main
> > reason I personally care for this patch, because it would allow
> > PgBouncer to ask for updates on certain GUCs so that it can preserve
> > session level SET commands even in transaction pooling mode.
> > Right now PgBouncer can only do this for a handful of GUCs, but
> > there's quite a few others that are useful for PgBouncer to preserve
> > by default:
> > - search_path
> > - statement_timeout
> > - lock_timeout
>
> ISTM that for a purpose like pgbouncer, it would be simpler to add a new
> GUC "report these variables" and send that in the startup message?  That
> might not help with the psql use case, but it would be much simpler.

I have changed the status to "Waiting on Author" as Peter's comments
have not been followed up yet.

Regards,
Vignesh

On Thu, 25 Jan 2024 at 21:43, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2. using GUC for all reported GUC looks not too readable. Maybe it should be used just for customized reporting, not for default

I thought about this too, because indeed the default list is quite
long. But I decided against it because it seemed strange that you
cannot disable reporting for the options that are currently reporting
by default. Especially since the current default essentially boils
down to: "whatever the psql client needed"

> 3. Another issue of your proposal is less friendly enabling disabling reporting of specific GUC. Maintaining a list needs more work than just enabling and disabling one specific GUC.
> I think this is the main disadvantage of your proposal. In my proposal I don't need to know the state of any GUC. Just I send PQlinkParameterStatus or PQunlinkParameterStatus. With your proposal, I need to read _pq_.report_parameters, parse it, and modify, and send it back. This doesn't look too practical.

While I agree it's a little bit less friendly, I think you're
overestimating the difficulty of using my proposed approach. Most
importantly there's no need to parse the current GUC value. A client
always knows what variables it wants to have reported. So anytime that
changes the client can simply regenerate the full list of gucs that it
wants to report and send that. So something similar to the following
pseudo code (using += for string concatenation):

char *report_parameters = "server_version,client_encoding"
if (show_user_in_prompt)
   report_parameters += ",user"
if (show_search_path_in_prompt)
   report_parameters += ",search_path"
PQsetParameter("_pq_.report_parameters", report_parameters)

> Personally I prefer usage of a more generic API than my PQlinkParameterStatus and PQunlinkParameterStatus. You talked about it with Robert If I remember.
>
> Can be nice if I can write just
>
> /* similar princip is used inside ncurses */
> set_report_guc_message_no = PQgetMessageNo("set_report_guc");
> /* the result can be processed by server and by all proxies on the line */
>
> if (set_report_guc_message_no == -1)
>   fprintf(stderr, "feature is not supported");
> result = PQsendMessage(set_report_guc_message, "user");
> if (result == -1)
>   fprintf(stderr, "some error ...");
>
> With some API like this it can be easier to do some small protocol enhancement. Maybe this is overengineering. Enhancing protocol is not too common, and with usage PQsendTypedCommand enhancing protocol is less work too.

Honestly, I don't see a benefit to this over protocol extension
parameters using the ParameterSet message. Afaict this is also
essentially a key + value message type. And protocol extension
parameters have the benefit that they are already an established thing
in the protocol, and that they can easily be integrated with the
current GUC system.

On Fri, 26 Jan 2024 at 21:35, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I see a possibility of disabling reporting as possibly dangerous.  Without reporting encoding you can break psql. So it should be limited just to few values where is known behave.

I agree that client_encoding is a GUC that likely all clients would
want to request reporting for, so I can see the argument for always
sending it. But I wouldn't call it dangerous for a client to be able
to disable reporting for it. Ultimately the client is the one that
decides. A client might just as well completely ignore the reported
value.

>> While I agree it's a little bit less friendly, I think you're
>> overestimating the difficulty of using my proposed approach. Most
>> importantly there's no need to parse the current GUC value. A client
>> always knows what variables it wants to have reported. So anytime that
>> changes the client can simply regenerate the full list of gucs that it
>> wants to report and send that. So something similar to the following
>> pseudo code (using += for string concatenation):
>
>
> I disagree with this - I can imagine some proxies add some own reported GUC and the client can know nothing about it.

I've definitely thought about this case, since it's the main case I
care about as maintainer of PgBouncer. And a client wouldn't need to
know about the extra GUCs that the proxy requires for the proxy to
work correctly. A proxy can quite simply handle this itself in the
following manner: Whenever a client sends a ParameterSet for
_pq_.report_parameters, the proxy could forward to the server after
prepending its own extra GUCs at the front. The proxy wouldn't even
need to parse the list from the client to be able to do that. An even
better behaving proxy, should parse the list of GUCs though and would
only forward the ParameterStatus messages that it receives from the
server if the client requested ParameterStatus updates for them.

On Sat, 27 Jan 2024 at 08:35, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Until now, it is not possible to disable reporting. So clients can expect so reporting is workable.

Sure, so if we leave the default as is that's fine. It's not as if
this GUC would be changed without the client knowing, the client would
be the one changing the GUC and thus disabling the sending of
reporting for the default GUCs. If it doesn't want to disable the
reporting, than it simply should not send such a request.

> Do you have a use case, when disabling some of the defaultly reported GUC makes sense?

Mostly if the client doesn't actually use them, e.g. I expect many
clients don't care what the current application_name is. But I do
agree this is not a very strong usecase, so I'd be okay with always
sending the ones that we sent as default for now. But that does make
the implementation more difficult, since we'd have to special case
these GUCs instead of having the same consistent behaviour for all
GUCs.

> yes, inside gradual connect you can enhance the list of custom reported GUC easily.
>
> but for use cases like prompt in psql, I need to enable, disable reporting - but this use case should be independent of "protected" connection related GUC reporting.
>
> For example - when I disable %N, I can disable reporting "role" and disable showing role in prompt. But when "role" should be necessary for pgbouncer, then surely the disabling reporting should be ignored. The user by setting a prompt should not break communication.  And it can be ignored without any issue, there is not performance issue, because "role" is still necessary for pgbouncer that is used for connection. Without described behaviour we should not implement controlling reporting to psql, because there can be a lot of unhappy side effects in dependency if the user set or unset custom prompt or some other future feature.

Maybe I'm misunderstanding what you're saying, but it's not clear to
me why you are seeing two different use cases here. To me if we have
the ParameterSet message then they are both the same. When you enable
%N you would send a ParamaterSet message for _pq_.report_parameters
and set it to a list of gucs including "role". And when you disable %N
you would set _pq_.report_parameters to a list of GUCs without "role".
Then if any proxy still needed "role" even if the list it receives in
_pq_.report_parameters doesn't contain it, then this proxy would
simply prepend "role" to the list of GUCs before forwarding the
ParameterSet message.

Also to be clear, having a "protected" connection-start only GUC is
problematic for proxies. Because they need to update this setting
while the connection is active when they hand of one server connection
to another client.

On Sat, 27 Jan 2024 at 20:44, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> client_encoding, standard_conforming_strings, server_version, default_transaction_read_only, in_hot_standby and scram_iterations
> are used by libpq directly, so it can be wrong to introduce the possibility to break it.

libpq could add these ones automatically to the list, just like a
proxy. But I think you are probably right and always reporting our
current default set is probably easier.

>> Maybe I'm misunderstanding what you're saying, but it's not clear to
>> me why you are seeing two different use cases here. To me if we have
>> the ParameterSet message then they are both the same. When you enable
>> %N you would send a ParamaterSet message for _pq_.report_parameters
>> and set it to a list of gucs including "role". And when you disable %N
>> you would set _pq_.report_parameters to a list of GUCs without "role".
>> Then if any proxy still needed "role" even if the list it receives in
>> _pq_.report_parameters doesn't contain it, then this proxy would
>> simply prepend "role" to the list of GUCs before forwarding the
>> ParameterSet message.
>
>
> Your scenario can work but looks too fragile. I checked - GUC now cannot contain some special chars, so writing parser should not be hard work. But your proposal means the proxy should be smart about it, and have to check any change of _pq_.report_parameters, and this point can be fragile and a source of hardly searched bugs.

Yes, proxies should be smart about it. But if there's new message
types introduced specifically for this, then proxies need to be smart
about it too. Because they would need to remember which reporting was
requested by the client, to be able to correctly ask for reporting
GUCs it after server connection . Using GUCs actually makes this
easier to implement (and thus less error prone), because proxies
already have logic to re-sync GUCs after connection assignment.

I think this is probably one of the core reasons why I would very much
prefer GUCs over new message types to configure protocol extensions
like this: It means proxies would not to keep track of and re-sync a
new kind of connection state every time a protocol extension is added.
They can make their GUC tracking and re-syncing robust, and that's all
they would need.

> This is true, but how common is this situation? Probably every client  that uses one proxy will use the same defaultly reported GUC.

If you have different clients connecting to the same proxy, it seems
quite likely that this will happen. This does not seem uncommon to me,
e.g. actual application would need different things always reported
than some dev client. Or clients for different languages might ask to
report slightly different settings.

> Reporting has no extra overhead. The notification is reduced. When there is a different set of reported GUC, then the proxy can try to find another connection with the same set or can reconnect.

Honestly, this logic seems much more fragile to implement. And
requiring reconnection seems problematic from a performance point of
view.

> I think so there is still opened question what should be correct behaviour when client execute RESET ALL or DISCARD ALL.  Without special protection the communication with proxy can be broken - and we use GUC for reported variables, then my case, prompt in psql will be broken too. Inside psql I have not callback on change of reported GUC. So this is reason why reporting based on mutable GUC is fragile :-/

Specifically for this reason, the current patchset in the other thread
already ignores RESET ALL and DISCARD ALL for protocol extension
parameters (including _pq_.report_parameters). So this would be a
non-issue.

On Sun, 28 Jan 2024 at 20:01, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> There is another reason - compatibility with other drivers.  We maintain just libpq, but there are JDBC, Npgsql, and some native Python drivers. I didn't checked, but maybe they expect GUC with GUC_REPORT flag.

This doesn't matter, because these drivers themselves would only stop
receiving certain GUC report messages if they changed this the
_pq_.report_paramers GUC. And at that point the other driver is
disabling the reporting on purpose. But like I said, I'm fine with
forcing the currently default GUC_REPORT GUCs to be GUC_REPORT always
(maybe excluding application_name).

> But now, I don't see any design without problems. Your look a little bit fragile to me,

Can you explain what still looks fragile to you about my design? Like
I explained at least from a proxy perspective this is the least
fragile imho, since it can reuse already existing and battle tested
code.

> From my perspective the situation can be better if I know so defaultly reported GUC are fixed, and cannot be broken. Then for almost all clients (without pgbouncer users), the CUSTOM_REPORT_GUC GUC will contain just "role", and then the risk is minimal.

Which risk are you talking about here?

> But still there are problems with handling of RESET ALL - so that means I need to do a recheck of the local state every time, when I will show a prompt with %N - that is not nice, but probably with a short list it will not be a problem.

I'm not entirely sure what you mean here. Is this still a problem if
RESET ALL is ignored for _pq_.report_parameters? If so, what problem
are you talking about then?

> But I can imagine a client crash, and then pgbouncer executes RESET ALL, and at this moment I would like to reset GUC_REPORT on "role" GUC. Maybe the introduction of a new flag for DISCARD can solve it. But again there can be a problem for which GUC the flag GUC_REPORT should be removed, because there are not two independent lists.

I don't think this is a problem. PgBouncer wouldn't rely on RESET ALL
to reset the state of _pq_.report_parameters. Before handing off the
old connection to a new client, PgBouncer would simply change the
_pq_.report_parameters GUC back to its default value by sending a
ParameterSet message. i.e. PgBouncer would use the same logic as it
currently uses to correctly reset tracked GUCs (application_name,
client_encoding, etc).

ne 28. 1. 2024 v 22:52 odesílatel Jelte Fennema-Nio <postgres@jeltef.nl> napsal:

On Sun, 28 Jan 2024 at 20:01, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> There is another reason - compatibility with other drivers.  We maintain just libpq, but there are JDBC, Npgsql, and some native Python drivers. I didn't checked, but maybe they expect GUC with GUC_REPORT flag.

This doesn't matter, because these drivers themselves would only stop
receiving certain GUC report messages if they changed this the
_pq_.report_paramers GUC. And at that point the other driver is
disabling the reporting on purpose. But like I said, I'm fine with
forcing the currently default GUC_REPORT GUCs to be GUC_REPORT always
(maybe excluding application_name).

ok

 

> But now, I don't see any design without problems. Your look a little bit fragile to me,

Can you explain what still looks fragile to you about my design? Like
I explained at least from a proxy perspective this is the least
fragile imho, since it can reuse already existing and battle tested
code.

because there is not 100% isolation of different layers, there is one resource that can be modified by network layer (proxy) and by application layer (psql). Unfortunately, I don't see any better solution, how these layes separated.

 

> From my perspective the situation can be better if I know so defaultly reported GUC are fixed, and cannot be broken. Then for almost all clients (without pgbouncer users), the CUSTOM_REPORT_GUC GUC will contain just "role", and then the risk is minimal.

Which risk are you talking about here?

The risk of not wanted reporting. I'll return to my psql prompt case. Who will disable reporting of "rule", when psql crashes? pgbouncer will call DISCARD ALL before reuse connection, but it ignores _pq_.report_parameters/';.>"

 

> But still there are problems with handling of RESET ALL - so that means I need to do a recheck of the local state every time, when I will show a prompt with %N - that is not nice, but probably with a short list it will not be a problem.

I'm not entirely sure what you mean here. Is this still a problem if
RESET ALL is ignored for _pq_.report_parameters? If so, what problem
are you talking about then?

> But I can imagine a client crash, and then pgbouncer executes RESET ALL, and at this moment I would like to reset GUC_REPORT on "role" GUC. Maybe the introduction of a new flag for DISCARD can solve it. But again there can be a problem for which GUC the flag GUC_REPORT should be removed, because there are not two independent lists.

I don't think this is a problem. PgBouncer wouldn't rely on RESET ALL
to reset the state of _pq_.report_parameters. Before handing off the
old connection to a new client, PgBouncer would simply change the
_pq_.report_parameters GUC back to its default value by sending a
ParameterSet message. i.e. PgBouncer would use the same logic as it
currently uses to correctly reset tracked GUCs (application_name,
client_encoding, etc).

ok, this can work, and this is the reply to my previous query.

Regards

Pavel