Re: proposal: psql: show current user in prompt

Поиск
Список
Период
Сортировка
От Pavel Stehule
Тема Re: proposal: psql: show current user in prompt
Дата
Msg-id CAFj8pRATHOygHNMOoNFz-D1jkb=jSCmuqT4ss7OBXvE1q1XR3A@mail.gmail.com
обсуждение исходный текст
Ответ на Re: proposal: psql: show current user in prompt  (Jelte Fennema-Nio <postgres@jeltef.nl>)
Ответы Re: proposal: psql: show current user in prompt  (Jelte Fennema-Nio <postgres@jeltef.nl>)
Список pgsql-hackers


ne 28. 1. 2024 v 10:42 odesílatel Jelte Fennema-Nio <postgres@jeltef.nl> napsal:
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.

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.


В списке pgsql-hackers по дате отправления:

Предыдущее
От: "Andrey M. Borodin"
Дата:
Сообщение: Re: MultiXact\SLRU buffers configuration
Следующее
От: Tomas Vondra
Дата:
Сообщение: Re: Add the ability to limit the amount of memory that can be allocated to backends.