ne 28. 1. 2024 v 10:42 odesÃlatel Jelte Fennema-Nio <postg...@jeltef.nl> napsal:
> On Sat, 27 Jan 2024 at 20:44, Pavel Stehule <pavel.steh...@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.
