Re: proposal: psql: show current user in prompt

ne 28. 1. 2024 v 10:42 odesílatel Jelte Fennema-Nio <postgres(at)jeltef(dot)nl>
napsal:

> On Sat, 27 Jan 2024 at 20:44, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.
>

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.

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

I am not against GUC based solutions. I think so for proxies it is probably
the best solution. But I just see only a GUC based solution not practical
for application.

Things are more complex when we try to think about possibility so
maintaining a list of reported GUC is more than one application. But now, I
don't see any design without problems. Your look a little bit fragile to
me, my proposal probably needs two independent lists of reported GUC, which
is not nice too. 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. 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.

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

I see one problematic scenario (my patch doesn't handle it well too).

When a user explicitly calls RESET ALL, or DISCARD ALL, and the connect -
client continues, then _pq_.report_parameters should not be changed.

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.