Re: proposal: psql: show current user in prompt
po 29. 1. 2024 v 10:26 odesílatel Pavel Stehule napsal: > > > ne 28. 1. 2024 v 22:52 odesílatel Jelte Fennema-Nio > napsal: > >> On Sun, 28 Jan 2024 at 20:01, Pavel Stehule >> 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. > I marked my patch as withdrawn. I'll resend it when your patch _pq_.report_parameters will be committed. Regards Pavel > > Regards > > Pavel >
Re: proposal: psql: show current user in prompt
ne 28. 1. 2024 v 22:52 odesílatel Jelte Fennema-Nio napsal: > On Sun, 28 Jan 2024 at 20:01, Pavel Stehule > 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
Re: proposal: psql: show current user in prompt
On Sun, 28 Jan 2024 at 20:01, Pavel Stehule 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).
Re: proposal: psql: show current user in prompt
ne 28. 1. 2024 v 10:42 odesílatel Jelte Fennema-Nio napsal: > On Sat, 27 Jan 2024 at 20:44, Pavel Stehule > 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
Re: proposal: psql: show current user in prompt
On Sat, 27 Jan 2024 at 20:44, Pavel Stehule 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.
Re: proposal: psql: show current user in prompt
so 27. 1. 2024 v 10:24 odesílatel Jelte Fennema-Nio napsal: > On Sat, 27 Jan 2024 at 08:35, Pavel Stehule > 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. > 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. > > 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. > 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. > > 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. > This is true, but how common is this situation? Probably every client that uses one proxy will use the same defaultly reported GUC. 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. 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 :-/ Pavel
Re: proposal: psql: show current user in prompt
On Sat, 27 Jan 2024 at 08:35, Pavel Stehule 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.
Re: proposal: psql: show current user in prompt
so 27. 1. 2024 v 0:04 odesílatel Jelte Fennema-Nio napsal: > On Fri, 26 Jan 2024 at 21:35, Pavel Stehule > 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. > Until now, it is not possible to disable reporting. So clients can expect so reporting is workable. Do you have a use case, when disabling some of the defaultly reported GUC makes sense? My patch doesn't protect these GUC, and now I think it is a mistake. > > >> 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. > 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.
Re: proposal: psql: show current user in prompt
On Fri, 26 Jan 2024 at 21:35, Pavel Stehule 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.
Re: proposal: psql: show current user in prompt
pá 26. 1. 2024 v 11:40 odesílatel Jelte Fennema-Nio
napsal:
> On Thu, 25 Jan 2024 at 21:43, Pavel Stehule
> 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"
>
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.
> > 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):
>
I disagree with this - I can imagine some proxies add some own reported GUC
and the client can know nothing about it.
>
> 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)
>
It is simple only when default value of report_parameters is constant, but
when not it can be fragile
>
> > 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.
>
It was an idea.
Personally I like an idea that I described in mail to Peter. Using
dedicated connection related GUC for immutably reported GUC, and using
possibility to set dedicated function in protocol for enabling, disabling
other GUC. It looks (to me) like the most robust solution.
Regards
Pavel
Re: proposal: psql: show current user in prompt
On Thu, 25 Jan 2024 at 21:43, Pavel Stehule 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.
Re: proposal: psql: show current user in prompt
po 8. 1. 2024 v 6:08 odesílatel vignesh C napsal: > On Tue, 12 Sept 2023 at 14:39, Peter Eisentraut > 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. > Please, see my reply to Peter. I think his comment is very valuable (and I am for it), but I don't think it is a practical API for this feature. Regards Pavel > > Regards, > Vignesh >
Re: proposal: psql: show current user in prompt
út 12. 9. 2023 v 9:46 odesílatel Peter Eisentraut napsal: > 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. > Introducing this GUC, mainly when the usage will be limited to connection string, makes sense to me. Unfortunately for usage in psql it is not practical. * For secure usage this GUC should be session immutable - probably you don't want to disable reporting for search_path, ... inside session * Enhancing list requires more work - reading current state, parsing (theoretically the GUC "report these variables" can be marked as GUC_REPORT, so I can see this value on client side, but still there is parsing. I can imagine enhancing the SET command to style SET GUC += value or SET GUC -= value * SET statement is transactional - that means it cannot be used when a transaction is broken, but it can be solved by some protocol based command for setting GUC without dependency on state of transaction (if it is possible, my functions changes just flag of some existing GUC, so there is not necessary memory allocation, changing the value can be different story). I can imagine both access - special GUC allowed only in connection string - that can work as protection against unwanted remove of GUC_REPORT too, and dedicated functions for enabling, disabling GUC_REPORT. It can very nicely work together. Then we don't need any other changes. There is no necessity for protocol SET, there is no necessity for more user friendly work with lists in SET statements. And with connect settings for reporting, proxies can easily detect and get the values of GUC. Regards Pavel
Re: proposal: psql: show current user in prompt
Hi
čt 11. 1. 2024 v 12:12 odesílatel Jelte Fennema-Nio
napsal:
> On Tue, 12 Sept 2023 at 09:46, Peter Eisentraut
> 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
I read your patch, and I see some advantages and some disadvantages.
1. this doesn't need new protocol API just for this feature, what is nice
2. using GUC for all reported GUC looks not too readable. Maybe it should
be used just for customized reporting, not for default
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.
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.
Regards
Pavel
Re: proposal: psql: show current user in prompt
Hi
I starting work on this patch - start with rebase
Regards
Pavel
From c7bbdd67898b397143fa314152f32d5ca935c082 Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Sun, 21 Jan 2024 14:23:06 +0100
Subject: [PATCH 1/4] Protocol ReportGUC message
This patch implements dynamic reporting changes of GUC to client side.
The flags per GUC can be changed by functions SetGUCOptionFlag and UsetGUCOptionFlag.
---
doc/src/sgml/protocol.sgml | 44 +
src/backend/tcop/postgres.c | 33 ++
src/backend/utils/misc/guc.c| 31
src/include/libpq/protocol.h| 1 +
src/include/utils/guc.h | 2 ++
src/interfaces/libpq/exports.txt| 2 ++
src/interfaces/libpq/fe-exec.c | 36 ++-
src/interfaces/libpq/fe-protocol3.c | 1 -
src/interfaces/libpq/fe-trace.c | 12
src/interfaces/libpq/libpq-fe.h | 3 ++
src/interfaces/libpq/libpq-int.h| 4 ++-
11 files changed, 166 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 6c3e8a631d..990eefc8b5 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1030,6 +1030,13 @@ SELCT 1/0;
CloseComplete, or NoData messages.
+
+
+The ReportGUC message allows to set/unset REPORT flag to any configuration
+variable. The response is a CommandComplete message (status string is SET
+or UNSET). An error causes ErrorResponse. When the REPORT flag is succesfully
+assigned, then ParameterStatus message is sent before CommandComplete message.
+
@@ -5506,6 +5513,43 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
+
+ReportGUC (F)
+
+
+
+ Byte1('r')
+
+
+ Identifies the message type. ReportGUC is sent by
+ frontend when the changes of specified GUC option
+ should be (or should not be) reported to state parameter.
+
+
+
+
+
+ Byte1
+
+
+ 't' when reporting should be enables, 'f' if reporting should be
+ disabled.
+
+
+
+
+
+ String
+
+
+ The name of GUC option.
+
+
+
+
+
+
+
RowDescription (B)
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 1a34bd3715..57ea37410b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -427,6 +427,7 @@ SocketBackend(StringInfo inBuf)
case PqMsg_Describe:
case PqMsg_Execute:
case PqMsg_Flush:
+ case PqMsg_ReportGUC:
maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
doing_extended_query_message = true;
break;
@@ -4851,6 +4852,38 @@ PostgresMain(const char *dbname, const char *username)
send_ready_for_query = true;
break;
+ case PqMsg_ReportGUC:
+{
+ const char *name;
+ const char *status;
+ intis_set;
+
+ is_set = pq_getmsgbyte(_message);
+ name = pq_getmsgstring(_message);
+ pq_getmsgend(_message);
+
+ if (is_set == 't')
+ {
+ SetGUCOptionFlag(name, GUC_REPORT);
+ status = "SET";
+ }
+ else if (is_set == 'f')
+ {
+ UnsetGUCOptionFlag(name, GUC_REPORT);
+ status = "UNSET";
+ }
+ else
+ ereport(ERROR,
+(errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("invalid argument of REPORTGUC message %c",
+ is_set)));
+
+ pq_puttextmessage(PqMsg_CommandComplete, status);
+
+ valgrind_report_error_query("ReportGUC message");
+}
+break;
+
/*
* 'X' means that the frontend is closing down the socket. EOF
* means unexpected loss of frontend connection. Either way,
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8f65ef3d89..f6888269b6 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2562,6 +2562,37 @@ BeginReportingGUCOptions(void)
}
}
+/*
+ * Allow to set / unset dynamicaly flags to GUC variables
+ */
+void
+SetGUCOptionFlag(const char *name, int flag)
+{
+ struct config_generic *conf;
+
+ /* only GUC_REPORT flag is supported now */
+ Assert(flag == GUC_REPORT);
+
+ conf = find_option(name, false, false, ERROR);
+ conf->flags |= flag;
+
+ if (flag == GUC_REPORT)
+ /* force transmit value of related option to client Parameter Status */
+ ReportGUCOption(conf);
+}
+
+void
+UnsetGUCOptionFlag(const char *name, int flag)
+{
+ struct config_generic *conf;
+
+ /* only GUC_REPORT flag is supported now */
+ Assert(flag == GUC_REPORT);
+
+ conf = find_option(name, false, false, ERROR);
+ conf->flags &= ~flag;
+}
+
/*
* ReportChangedGUCOptions: report recently-changed GUC_REPORT variables
*
diff --git a/src/include/libpq/protocol.h b/src/include/libpq/protocol.h
index 4b8d440365..abcfbf2bb5 100644
--- a/src/include/libpq/protocol.h
+++
Re: proposal: psql: show current user in prompt
On Tue, 12 Sept 2023 at 09:46, Peter Eisentraut 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
Re: proposal: psql: show current user in prompt
On Tue, 12 Sept 2023 at 14:39, Peter Eisentraut 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
Re: proposal: psql: show current user in prompt
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.
Re: proposal: psql: show current user in prompt
po 11. 9. 2023 v 13:24 odesílatel Jelte Fennema napsal:
> On Fri, 8 Sept 2023 at 21:08, Pavel Stehule
> 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.
>
I did proposed change
Regards
Pavel
From 2f432cbb6d38569759e12a2cbe80ec9dce4b2f25 Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Mon, 24 Jul 2023 20:18:16 +0200
Subject: [PATCH 4/4] Implementation of %N prompt placeholder
It is based on forcing reporting feature"role" GUC to client.
---
doc/src/sgml/ref/psql-ref.sgml | 19 ++-
src/bin/psql/command.c | 13 +
src/bin/psql/prompt.c | 27 +++
src/bin/psql/settings.h| 1 +
src/bin/psql/startup.c | 32
5 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d94e3cacfc..8b267a6da6 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4568,7 +4568,24 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is set to the database session user name.
+
+
+
+ This substitution requires PostgreSQL
+ version 16 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index bcd8eb3538..bad0fdf415 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3883,6 +3883,7 @@ SyncVariables(void)
{
char vbuf[32];
const char *server_version;
+ PGresult *result;
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
@@ -3912,6 +3913,18 @@ SyncVariables(void)
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
+
+ /* link role GUC when it is needed for prompt */
+ if (pset.prompt_shows_role)
+ result = PQlinkParameterStatus(pset.db, "role");
+ else
+ result = PQunlinkParameterStatus(pset.db, "role");
+
+ if (PQresultStatus(result) != PGRES_COMMAND_OK)
+ pg_log_info("cannot set REPORT flag on configuration variable \"role\": %s",
+ PQerrorMessage(pset.db));
+
+ PQclear(result);
}
/*
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..566b47d814 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,33 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
if (pset.db)
strlcpy(buf, session_username(), sizeof(buf));
break;
+ /* DB server user role */
+case 'N':
+ if (pset.db)
+ {
+ const char *rolename = NULL;
+
+ /*
+ * This feature requires GUC "role" to be marked
+ * by GUC_REPORT flag. This is done by PQlinkParameterStatus
+ * function. This function requires protocol 3.1 (ReportGUC
+ * message). Fallback is empty string.
+ */
+ if (PQprotocolVersionFull(pset.db) >= PQmakeProtocolVersionFull(3,1))
+ {
+ rolename = PQparameterStatus(pset.db, "role");
+
+ /* fallback when role is not set yet */
+ if (rolename && strcmp(rolename, "none") == 0)
+rolename = session_username();
+ }
+
+ if (rolename)
+ strlcpy(buf, rolename, sizeof(buf));
+ else
+ buf[0] = '\0';
+ }
+ break;
/* backend pid
Re: proposal: psql: show current user in prompt
On Mon, 11 Sept 2023 at 13:59, Jelte Fennema wrote: > I think that would make the client code even simpler than it is now. To be clear, I'm not saying we should do this. There's benefits to using a dedicated new message type too (e.g. clearer errors if a proxy like pgbouncer does not understand it).
Re: proposal: psql: show current user in prompt
@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 And users might have others that they want to preserve others too. On Fri, 8 Sept 2023 at 21:08, Pavel Stehule wrote: >> 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? To clarify what I meant: I meant instead of introducing a new top level message type (i.e. your newly introduced ReportGUC message), I suggested adding a new subtype that is understood by the Describe and Close messages. In addition to being able to use Describe and Close for statements and portals with the S and P subtypes respectively, like you currently can, we could add a new subtype G which would start and stop GUC reporting (with the Describe and Close message respectively). I think that would make the client code even simpler than it is now.
Re: proposal: psql: show current user in prompt
On Fri, 8 Sept 2023 at 21:08, Pavel Stehule 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.
Re: proposal: psql: show current user in prompt
pá 8. 9. 2023 v 21:07 odesílatel Pavel Stehule
napsal:
> 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"
>
I fixed this issue. The problem was in the missing setting
`doing_extended_query_message`.
> 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
>
Regards
Pavel
>
>
From 4a7c8b30f297189f6801076556a984baa51daa4f Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Sun, 3 Sep 2023 19:14:24 +0200
Subject: [PATCH 3/4] - allow to connect to server with major protocol version
3, minor version is ignored
- allow to read minor protocol version
---
doc/src/sgml/libpq.sgml | 17 +
src/include/libpq/pqcomm.h | 2 +-
src/interfaces/libpq/exports.txt| 1 +
src/interfaces/libpq/fe-connect.c | 12 +++-
src/interfaces/libpq/fe-protocol3.c | 21 +
src/interfaces/libpq/libpq-fe.h | 1 +
6 files changed, 48 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index a52baa27d5..d9e5502d09 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2576,6 +2576,23 @@ int PQprotocolVersion(const PGconn *conn);
+
+ PQprotocolVersionFullPQprotocolVersionFull
+
+
+
+ Returns complete frontend/backend protocol number.
+
+int PQprotocolVersionFull(const PGconn *conn);
+
+ The frontend/backend version protocol number is an value, that composites.
+ major protocol number and minor protocol number. These numbers can be
+ separated by using macros PG_PROTOCOL_MAJOR and
+ PG_PROTOCOL_MINOR.
+
+
+
+
PQserverVersionPQserverVersion
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index 46a0946b8b..4ea4538157 100644
--- a/src/include/libpq/pqcomm.h
+++ b/src/include/libpq/pqcomm.h
@@ -94,7 +94,7 @@ is_unixsock_path(const char *path)
*/
#define PG_PROTOCOL_EARLIEST PG_PROTOCOL(3,0)
-#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,0)
+#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,1)
typedef uint32 ProtocolVersion; /* FE/BE protocol version number */
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 7e101368d5..595a4808f2 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -193,3 +193,4 @@ PQsendClosePrepared 190
PQsendClosePortal 191
PQlinkParameterStatus 192
Re: proposal: psql: show current user in prompt
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
Re: proposal: psql: show current user in prompt
út 5. 9. 2023 v 13:29 odesílatel Jelte Fennema napsal:
> On Tue, 5 Sept 2023 at 05:50, Pavel Stehule
> 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.
>
ok changed - there is minor problem - almost all PQ functions are of int
type, but ProtocolVersion is uint
Using different mapping to int can be problematic - can be messy if we
cannot to use PG_PROTOCOL macro.
> >> + /* 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)
>
>
done
Regards
Pavel
>
> 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.
>
From a43a620d0ab8dfa01d82baf5c1752a2ba3bb8666 Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Mon, 24 Jul 2023 20:18:16 +0200
Subject: [PATCH 4/4] Implementation of %N prompt placeholder
It is based on forcing reporting feature"role" GUC to client.
---
doc/src/sgml/ref/psql-ref.sgml | 19 ++-
src/bin/psql/command.c | 13 +
src/bin/psql/prompt.c | 27 +++
src/bin/psql/settings.h| 1 +
src/bin/psql/startup.c | 32
5 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d94e3cacfc..8b267a6da6 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4568,7 +4568,24 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is set to the database session user name.
+
+
+
+ This substitution requires PostgreSQL
+ version 16 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index bcd8eb3538..bad0fdf415 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3883,6 +3883,7 @@ SyncVariables(void)
{
char vbuf[32];
const char *server_version;
+ PGresult *result;
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
@@ -3912,6 +3913,18 @@ SyncVariables(void)
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
+
+ /* link role GUC when it is needed for prompt */
+ if (pset.prompt_shows_role)
+ result = PQlinkParameterStatus(pset.db, "role");
+ else
+ result = PQunlinkParameterStatus(pset.db, "role");
+
+ if (PQresultStatus(result) != PGRES_COMMAND_OK)
+ pg_log_info("cannot set REPORT flag on configuration variable \"role\": %s",
+ PQerrorMessage(pset.db));
+
+ PQclear(result);
}
/*
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..c1bc81c8dd 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,33 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
if (pset.db)
strlcpy(buf, session_username(), sizeof(buf));
break;
+ /* DB server user role */
+case 'N':
+ if (pset.db)
+ {
+ const char *rolename = NULL;
+
+ /*
+ * This feature requires GUC "role" to be marked
+ * by GUC_REPORT flag. This is done by PQlinkParameterStatus
+ * function. This function requires protocol 3.1 (ReportGUC
+
Re: proposal: psql: show current user in prompt
On Tue, 5 Sept 2023 at 05:50, Pavel Stehule 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.
Re: proposal: psql: show current user in prompt
po 4. 9. 2023 v 14:24 odesílatel Jelte Fennema napsal:
> On Sun, 3 Sept 2023 at 20:58, Pavel Stehule
> 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.**
>
This point is open. I'll wait for a reply from others.
>
> + /* 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
From f1975b784627fda06b23303c4f1fb6972d80a0a0 Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Mon, 24 Jul 2023 20:18:16 +0200
Subject: [PATCH 4/4] Implementation of %N prompt placeholder
It is based on forcing reporting feature"role" GUC to client.
---
doc/src/sgml/ref/psql-ref.sgml | 19 ++-
src/bin/psql/command.c | 13 +
src/bin/psql/prompt.c | 28
src/bin/psql/settings.h| 1 +
src/bin/psql/startup.c | 32
5 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d94e3cacfc..8b267a6da6 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4568,7 +4568,24 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is set to the database session user name.
+
+
+
+ This substitution requires PostgreSQL
+ version 16 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index bcd8eb3538..bad0fdf415 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3883,6 +3883,7 @@ SyncVariables(void)
{
char vbuf[32];
const char *server_version;
+ PGresult *result;
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
@@ -3912,6 +3913,18 @@ SyncVariables(void)
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
+
+ /* link role GUC when it is needed for prompt */
+ if (pset.prompt_shows_role)
+ result = PQlinkParameterStatus(pset.db, "role");
+ else
+ result = PQunlinkParameterStatus(pset.db, "role");
+
+ if (PQresultStatus(result) != PGRES_COMMAND_OK)
+ pg_log_info("cannot set REPORT flag on configuration variable \"role\": %s",
+ PQerrorMessage(pset.db));
+
+ PQclear(result);
}
/*
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..d306c91d19 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,34 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
if (pset.db)
strlcpy(buf, session_username(), sizeof(buf));
break;
+ /* DB server user role */
+case 'N':
+ if (pset.db)
+ {
+ const char *rolename = NULL;
+
+ /*
+ * This feature requires GUC "role" to be marked
+ * by GUC_REPORT flag. This is done by PQlinkParameterStatus
+ * function. This function requires protocol 3.1 (ReportGUC
+ * message). Fallback is empty string.
+ */
+ if (PQprotocolVersion(pset.db) == 3 &&
+ PQprotocolSubversion(pset.db) >= 1)
+ {
+ rolename = PQparameterStatus(pset.db, "role");
+
+ /* fallback when role is not set yet */
+ if (rolename && strcmp(rolename, "none") == 0)
+rolename = session_username();
+ }
+
+ if (rolename)
+ strlcpy(buf, rolename, sizeof(buf));
+ else
+ buf[0] = '\0';
+
Re: proposal: psql: show current user in prompt
po 4. 9. 2023 v 14:24 odesílatel Jelte Fennema napsal: > On Sun, 3 Sept 2023 at 20:58, Pavel Stehule > 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.** > Both versions look a little bit strange to me. I have not strong opinion about it, but I am not sure if is best to change contract after 20 years ago commit from Jun 2003 efc3a25bb02ada63158fe7006673518b005261ba 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. Reards Pavel > + /* 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. >
Re: proposal: psql: show current user in prompt
On Sun, 3 Sept 2023 at 20:58, Pavel Stehule 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.
Re: proposal: psql: show current user in prompt
Hi
ne 3. 9. 2023 v 9:59 odesílatel Jelte Fennema napsal:
> On Sun, 3 Sept 2023 at 08:24, Pavel Stehule
> 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.
>
ok
here is an try
Regards
Pavel
From 80a076e28c293aef96f455a25e967f2c9baf8eac Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Sun, 3 Sep 2023 19:14:24 +0200
Subject: [PATCH 3/4] - allow to connect to server with major protocol version
3, minor version is ignored
- allow to read minor protocol version
---
src/include/libpq/pqcomm.h | 2 +-
src/interfaces/libpq/exports.txt| 1 +
src/interfaces/libpq/fe-connect.c | 12 +++-
src/interfaces/libpq/fe-protocol3.c | 13 ++---
src/interfaces/libpq/libpq-fe.h | 1 +
5 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index 46a0946b8b..4ea4538157 100644
--- a/src/include/libpq/pqcomm.h
+++ b/src/include/libpq/pqcomm.h
@@ -94,7 +94,7 @@ is_unixsock_path(const char *path)
*/
#define PG_PROTOCOL_EARLIEST PG_PROTOCOL(3,0)
-#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,0)
+#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,1)
typedef uint32 ProtocolVersion; /* FE/BE protocol version number */
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 7e101368d5..71e180cd57 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -193,3 +193,4 @@ PQsendClosePrepared 190
PQsendClosePortal 191
PQlinkParameterStatus 192
PQunlinkParameterStatus 193
+PQprotocolSubversion 194
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index bf83a9b569..5e628578b4 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2777,7 +2777,7 @@ keep_going: /* We will come back to here until there is
* must persist across individual connection attempts, but we must
* reset them when we start to consider a new server.
*/
- conn->pversion = PG_PROTOCOL(3, 0);
+ conn->pversion = PG_PROTOCOL(3, 1);
conn->send_appname = true;
#ifdef USE_SSL
/* initialize these values based on SSL mode */
@@ -7234,6 +7234,16 @@ PQprotocolVersion(const PGconn *conn)
return PG_PROTOCOL_MAJOR(conn->pversion);
}
+int
+PQprotocolSubversion(const PGconn *conn)
+{
+ if (!conn)
+ return 0;
+ if (conn->status == CONNECTION_BAD)
+ return 0;
+ return PG_PROTOCOL_MINOR(conn->pversion);
+}
+
int
PQserverVersion(const PGconn *conn)
{
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 90d4e17e6f..05afa70694 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -1441,9 +1441,16 @@ pqGetNegotiateProtocolVersion3(PGconn *conn)
}
if (their_version < conn->pversion)
- libpq_append_conn_error(conn, "protocol version not supported by server: client uses %u.%u, server supports up to %u.%u",
-PG_PROTOCOL_MAJOR(conn->pversion), PG_PROTOCOL_MINOR(conn->pversion),
-PG_PROTOCOL_MAJOR(their_version), PG_PROTOCOL_MINOR(their_version));
+ {
+ /* The protocol 3.0 is required */
+ if (PG_PROTOCOL_MAJOR(their_version) == 3)
+ conn->pversion = their_version;
+ else
+ libpq_append_conn_error(conn, "protocol version not supported by server: client uses %u.%u, server supports up to %u.%u",
+ PG_PROTOCOL_MAJOR(conn->pversion), PG_PROTOCOL_MINOR(conn->pversion),
+ PG_PROTOCOL_MAJOR(their_version), PG_PROTOCOL_MINOR(their_version));
+ }
+
if (num > 0)
{
appendPQExpBuffer(>errorMessage,
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index ba3ad7e0aa..904e611670 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -347,6 +347,7 @@ extern PGTransactionStatusType PQtransactionStatus(const PGconn *conn);
extern const char *PQparameterStatus(const PGconn *conn,
const char *paramName);
extern int PQprotocolVersion(const PGconn *conn);
+extern int PQprotocolSubversion(const PGconn *conn);
extern int PQserverVersion(const PGconn *conn);
extern char *PQerrorMessage(const PGconn *conn);
extern int PQsocket(const PGconn *conn);
--
2.41.0
From bd59c5318c09d09b764da81533d2712aa6d658d4 Mon Sep 17 00:00:00
Re: proposal: psql: show current user in prompt
On Sun, 3 Sept 2023 at 08:24, Pavel Stehule 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.
Re: proposal: psql: show current user in prompt
út 29. 8. 2023 v 14:11 odesílatel Jelte Fennema napsal: > On Mon, 28 Aug 2023 at 15:00, Pavel Stehule > 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. > I checked this part and this part of the code, and it looks like current libpq doesn't allow optional requirements of higher protocol version number. With the current state of NegotiateProtocolVersion handling I am not able to connect to any older servers, because there is no fallback implementation. 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. But maybe I don't see some possibility? Regards Pavel >
Re: proposal: psql: show current user in prompt
On Mon, 28 Aug 2023 at 15:00, Pavel Stehule 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.
+
+ReportGUC (F)
We'll need some docs on the "Message Flow" page too:
https://www.postgresql.org/docs/current/protocol-flow.html
Specifically what response is expected, if any.
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.
Re: proposal: psql: show current user in prompt
po 28. 8. 2023 v 13:58 odesílatel Pavel Stehule
napsal:
> 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
>
with protocol test
Regards
Pavel
>
> Regards
>
> Pavel
>
>
>>
>> Regards
>>
>> Pavel
>>
>>
>
From 4567473ebd7fec9dc836793e8e80244b22e5c96b Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Mon, 24 Jul 2023 20:18:16 +0200
Subject: [PATCH 2/3] Implementation of %N prompt placeholder
It is based on forcing reporting feature"role" GUC to client.
---
doc/src/sgml/ref/psql-ref.sgml | 19 +-
src/bin/psql/command.c | 13 +
src/bin/psql/prompt.c | 35 ++
src/bin/psql/settings.h| 1 +
src/bin/psql/startup.c | 32 +++
5 files changed, 99 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 182e58353f..b5da83013e 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4567,7 +4567,24 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is set to the database session user name.
+
+
+
+ This substitution requires PostgreSQL
+ version 16 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 1300869d79..bf67febc11 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3861,6 +3861,7 @@ SyncVariables(void)
{
char vbuf[32];
const char *server_version;
+ PGresult *result;
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
@@ -3890,6 +3891,18 @@ SyncVariables(void)
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
+
+ /* link role GUC when it is needed for prompt */
+ if (pset.prompt_shows_role)
+ result = PQlinkParameterStatus(pset.db, "role");
+ else
+ result = PQunlinkParameterStatus(pset.db, "role");
+
+ if (PQresultStatus(result) != PGRES_COMMAND_OK)
+ pg_log_info("cannot set REPORT flag on configuration variable \"role\": %s",
+ PQerrorMessage(pset.db));
+
+ PQclear(result);
}
/*
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..b0f5158c59 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,41 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
if (pset.db)
strlcpy(buf, session_username(), sizeof(buf));
break;
+ /* DB server user role */
+case 'N':
+ if (pset.db)
+ {
+ int minServerMajor;
+ int serverMajor;
+ const char *rolename = NULL;
+
+ /*
+ * This feature requires GUC "role" to be marked
+ * as GUC_REPORT. Without it is hard to specify fallback
+ * result. Returning empty value can be messy, returning
+ * PQuser like session_username can be messy too.
+ * Exec query is not too practical too, because it doesn't
+ * work when session is not in transactional state, and
+ * CURRENT_ROLE returns different result when role is not
+ * explicitly specified by SET ROLE.
+ */
+ minServerMajor = 1600;
+ serverMajor = PQserverVersion(pset.db) / 100;
+ if (serverMajor >= minServerMajor)
+ {
+ rolename = PQparameterStatus(pset.db, "role");
+
+ /* fallback when role is not set yet */
+ if (rolename && strcmp(rolename, "none") == 0)
+rolename = session_username();
+ }
+
+ if (rolename)
+ strlcpy(buf, rolename, sizeof(buf));
+ else
+ buf[0] = '\0';
+ }
+ break;
/* backend pid */
case 'p':
if (pset.db)
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 1106954236..cb7c12bd1d 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -154,6 +154,7 @@ typedef struct _psqlSettings
PGVerbosity verbosity; /* current error verbosity level */
bool show_all_results;
PGContextVisibility show_context; /* current context display level */
+ bool prompt_shows_role;
} PsqlSettings;
extern PsqlSettings pset;
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 5a28b6f713..0dac396525 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@
Re: proposal: psql: show current user in prompt
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
>
>
From 4567473ebd7fec9dc836793e8e80244b22e5c96b Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Mon, 24 Jul 2023 20:18:16 +0200
Subject: [PATCH 2/2] Implementation of %N prompt placeholder
It is based on forcing reporting feature"role" GUC to client.
---
doc/src/sgml/ref/psql-ref.sgml | 19 +-
src/bin/psql/command.c | 13 +
src/bin/psql/prompt.c | 35 ++
src/bin/psql/settings.h| 1 +
src/bin/psql/startup.c | 32 +++
5 files changed, 99 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 182e58353f..b5da83013e 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4567,7 +4567,24 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is set to the database session user name.
+
+
+
+ This substitution requires PostgreSQL
+ version 16 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 1300869d79..bf67febc11 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3861,6 +3861,7 @@ SyncVariables(void)
{
char vbuf[32];
const char *server_version;
+ PGresult *result;
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
@@ -3890,6 +3891,18 @@ SyncVariables(void)
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
+
+ /* link role GUC when it is needed for prompt */
+ if (pset.prompt_shows_role)
+ result = PQlinkParameterStatus(pset.db, "role");
+ else
+ result = PQunlinkParameterStatus(pset.db, "role");
+
+ if (PQresultStatus(result) != PGRES_COMMAND_OK)
+ pg_log_info("cannot set REPORT flag on configuration variable \"role\": %s",
+ PQerrorMessage(pset.db));
+
+ PQclear(result);
}
/*
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..b0f5158c59 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,41 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
if (pset.db)
strlcpy(buf, session_username(), sizeof(buf));
break;
+ /* DB server user role */
+case 'N':
+ if (pset.db)
+ {
+ int minServerMajor;
+ int serverMajor;
+ const char *rolename = NULL;
+
+ /*
+ * This feature requires GUC "role" to be marked
+ * as GUC_REPORT. Without it is hard to specify fallback
+ * result. Returning empty value can be messy, returning
+ * PQuser like session_username can be messy too.
+ * Exec query is not too practical too, because it doesn't
+ * work when session is not in transactional state, and
+ * CURRENT_ROLE returns different result when role is not
+ * explicitly specified by SET ROLE.
+ */
+ minServerMajor = 1600;
+ serverMajor = PQserverVersion(pset.db) / 100;
+ if (serverMajor >= minServerMajor)
+ {
+ rolename = PQparameterStatus(pset.db, "role");
+
+ /* fallback when role is not set yet */
+ if (rolename && strcmp(rolename, "none") == 0)
+rolename = session_username();
+ }
+
+ if (rolename)
+ strlcpy(buf, rolename, sizeof(buf));
+ else
+ buf[0] = '\0';
+ }
+ break;
/* backend pid */
case 'p':
if (pset.db)
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 1106954236..cb7c12bd1d 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -154,6 +154,7 @@ typedef struct _psqlSettings
PGVerbosity verbosity; /* current error verbosity level */
bool show_all_results;
PGContextVisibility show_context; /* current context display level */
+ bool prompt_shows_role;
} PsqlSettings;
extern PsqlSettings pset;
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 5a28b6f713..0dac396525 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -1094,10 +1094,40 @@ histcontrol_hook(const char *newval)
return true;
}
+static void
+prompt_needs_role_parameter_status(void)
+{
Re: proposal: psql: show current user in prompt
čt 10. 8. 2023 v 16:31 odesílatel Jelte Fennema napsal: > On Thu, 10 Aug 2023 at 14:44, Pavel Stehule > wrote: > > čt 10. 8. 2023 v 14:05 odesílatel Jelte Fennema > 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 > yes > > 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). > I have to recheck it Regards Pavel
Re: proposal: psql: show current user in prompt
On Thu, 10 Aug 2023 at 14:44, Pavel Stehule wrote: > čt 10. 8. 2023 v 14:05 odesílatel Jelte Fennema 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).
Re: proposal: psql: show current user in prompt
čt 10. 8. 2023 v 14:05 odesílatel Jelte Fennema napsal: > On Tue, 8 Aug 2023 at 07:20, Pavel Stehule > 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; > surely not. \set is client side setting, and it is not transactional. Attention - "\set" and "set" commands are absolutely different creatures. > 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) > Postgres does it, but not on the client side. What I know, almost all environments don't support transactions on the client side. Postgres is not special in this direction. > > 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. > Maybe we are talking about different features. Now, I have no idea how the proposed feature can be useful for pgbouncer? Sure If I implement a new flow, then pgbouncer cannot forward. But it is not too hard to implement redirection of new flow to pgbouncer. > 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? > I try to get some consistency. psql setting and some features like formatting doesn't depend on transactional state. It depends just on connection. This is the reason why I don't want to inject dependency on transaction state. Without this dependency, I don't need to check transaction state, and I can execute prompt settings immediately. If I implement this feature as transactional, then I need to wait to idle or to make a new transaction (and this I have not under my control). I try to be consistent with current psql behaviour. It Would be strange (can be very messy) if I had a message like "cannot set a prompt, because you should do ROLLBACK first" When this feature should be implemented as an injected message, then I have another problem. Which SQL command I have to send to the server, when the user wants to set a prompt? And then I don't need to implement a new message, and I can just implement the SQL function pg_catalog.report_config(...).
Re: proposal: psql: show current user in prompt
On Tue, 8 Aug 2023 at 07:20, Pavel Stehule 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?
Re: proposal: psql: show current user in prompt
po 31. 7. 2023 v 17:46 odesílatel Jelte Fennema napsal: > On Mon, 24 Jul 2023 at 21:16, Pavel Stehule > 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 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. Proposed message can be injected to other flows too, I think, but for the proposed psql feature it is not practical. 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. Regards Pavel > > > 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. >
Re: proposal: psql: show current user in prompt
On Mon, 24 Jul 2023 at 21:16, Pavel Stehule 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.
Re: proposal: psql: show current user in prompt
Hi
po 8. 5. 2023 v 14:22 odesílatel Jelte Fennema napsal:
> 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.
>
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.
For example, PQsetClientEncoding does a PQexec call, which is much more
expensive. 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.
The API can be changed from PQlinkParameterStatus(PGconn *conn, const char
*paramName)
to
PQlinkParameterStatus(PGconn *conn, int nParamNames, const char *paramNames)
What do you think?
Regards
Pavel
>
> 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
> wrote:
> >
> >
> >
> > čt 27. 4. 2023 v 7:39 odesílatel Pavel Stehule
> napsal:
> >>
> >> Hi
> >>
> >> rebased version + fix warning possibly uninitialized variable
> >
> >
> > fix not unique function id
> >
> > Regards
> >
> > Pavel
> >
> >>
> >> Regards
> >>
> >> Pavel
> >>
>
Re: proposal: psql: show current user in prompt
Hi
pá 28. 4. 2023 v 7:00 odesílatel Pavel Stehule
napsal:
>
>
> čt 27. 4. 2023 v 7:39 odesílatel Pavel Stehule
> napsal:
>
>> Hi
>>
>> rebased version + fix warning possibly uninitialized variable
>>
>
> fix not unique function id
>
> Regards
>
> Pavel
>
only rebase
>
>
>> Regards
>>
>> Pavel
>>
>>
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index b11d9a6ba3..f774ffa310 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -5401,6 +5401,43 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
+
+ReportGUC (F)
+
+
+
+ Byte1('r')
+
+
+ Identifies the message type. ReportGUC is sent by
+ frontend when the changes of specified GUC option
+ should be (or should not be) reported to state parameter.
+
+
+
+
+
+ String
+
+
+ The name of GUC option.
+
+
+
+
+
+ Int16
+
+
+ 1 if reporting should be enables, 0 if reporting should be
+ disabled.
+
+
+
+
+
+
+
RowDescription (B)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 35aec6d3ce..036d34f412 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4545,7 +4545,24 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is set to the database session user name.
+
+
+
+ This substitution requires PostgreSQL
+ version 16 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 36cc99ec9c..0df46514bf 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -451,6 +451,11 @@ SocketBackend(StringInfo inBuf)
doing_extended_query_message = false;
break;
+ case 'r':/* report GUC */
+ maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
+ doing_extended_query_message = false;
+ break;
+
default:
/*
@@ -4828,6 +4833,24 @@ PostgresMain(const char *dbname, const char *username)
pq_flush();
break;
+ case 'r': /* report GUC */
+{
+ const char *name;
+ intcreate_flag;
+
+ name = pq_getmsgstring(_message);
+ create_flag = pq_getmsgint(_message, 2);
+ pq_getmsgend(_message);
+
+ if (create_flag)
+ SetGUCOptionFlag(name, GUC_REPORT);
+ else
+ UnsetGUCOptionFlag(name, GUC_REPORT);
+
+ send_ready_for_query = true;
+}
+break;
+
case 'S': /* sync */
pq_getmsgend(_message);
finish_xact_command();
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 5308896c87..e09cbbb414 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2532,6 +2532,37 @@ BeginReportingGUCOptions(void)
}
}
+/*
+ * Allow to set / unset dynamicaly flags to GUC variables
+ */
+void
+SetGUCOptionFlag(const char *name, int flag)
+{
+ struct config_generic *conf;
+
+ /* only GUC_REPORT flag is supported now */
+ Assert(flag == GUC_REPORT);
+
+ conf = find_option(name, false, true, ERROR);
+ conf->flags |= flag;
+
+ if (flag == GUC_REPORT)
+ /* force transmit value of related option to client Parameter Status */
+ ReportGUCOption(conf);
+}
+
+void
+UnsetGUCOptionFlag(const char *name, int flag)
+{
+ struct config_generic *conf;
+
+ /* only GUC_REPORT flag is supported now */
+ Assert(flag == GUC_REPORT);
+
+ conf = find_option(name, false, true, ERROR);
+ conf->flags &= ~flag;
+}
+
/*
* ReportChangedGUCOptions: report recently-changed GUC_REPORT variables
*
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 511debbe81..a1cf434187 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3859,6 +3859,7 @@ SyncVariables(void)
{
char vbuf[32];
const char *server_version;
+ int res;
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
@@ -3888,6 +3889,15 @@ SyncVariables(void)
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
+
+ /* link role GUC when it is needed for prompt */
+ if (pset.prompt_shows_role)
+ res = PQlinkParameterStatus(pset.db, "role");
+ else
+ res = PQunlinkParameterStatus(pset.db, "role");
+
+ if (res < 0)
+ pg_log_info("%s", PQerrorMessage(pset.db));
}
/*
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..b0f5158c59 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6
Re: proposal: psql: show current user in prompt
Hi
po 8. 5. 2023 v 14:22 odesílatel Jelte Fennema napsal:
> 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}
>
>
Thank you for your comments, I'll finish refactoring plpgsql_check, and
I'll start refactoring this patch with your comments.
Regards
Pavel
>
> On Fri, 28 Apr 2023 at 07:00, Pavel Stehule
> wrote:
> >
> >
> >
> > čt 27. 4. 2023 v 7:39 odesílatel Pavel Stehule
> napsal:
> >>
> >> Hi
> >>
> >> rebased version + fix warning possibly uninitialized variable
> >
> >
> > fix not unique function id
> >
> > Regards
> >
> > Pavel
> >
> >>
> >> Regards
> >>
> >> Pavel
> >>
>
Re: proposal: psql: show current user in prompt
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 wrote:
>
>
>
> čt 27. 4. 2023 v 7:39 odesílatel Pavel Stehule
> napsal:
>>
>> Hi
>>
>> rebased version + fix warning possibly uninitialized variable
>
>
> fix not unique function id
>
> Regards
>
> Pavel
>
>>
>> Regards
>>
>> Pavel
>>
Re: proposal: psql: show current user in prompt
čt 27. 4. 2023 v 7:39 odesílatel Pavel Stehule
napsal:
> Hi
>
> rebased version + fix warning possibly uninitialized variable
>
fix not unique function id
Regards
Pavel
> Regards
>
> Pavel
>
>
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index b11d9a6ba3..f774ffa310 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -5401,6 +5401,43 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
+
+ReportGUC (F)
+
+
+
+ Byte1('r')
+
+
+ Identifies the message type. ReportGUC is sent by
+ frontend when the changes of specified GUC option
+ should be (or should not be) reported to state parameter.
+
+
+
+
+
+ String
+
+
+ The name of GUC option.
+
+
+
+
+
+ Int16
+
+
+ 1 if reporting should be enables, 0 if reporting should be
+ disabled.
+
+
+
+
+
+
+
RowDescription (B)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc422373d6..569f50fe7d 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4551,7 +4551,24 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is set to the database session user name.
+
+
+
+ This substitution requires PostgreSQL
+ version 16 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 01b6cc1f7d..54a3f5c1e9 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -451,6 +451,11 @@ SocketBackend(StringInfo inBuf)
doing_extended_query_message = false;
break;
+ case 'r':/* report GUC */
+ maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
+ doing_extended_query_message = false;
+ break;
+
default:
/*
@@ -4823,6 +4828,24 @@ PostgresMain(const char *dbname, const char *username)
pq_flush();
break;
+ case 'r': /* report GUC */
+{
+ const char *name;
+ intcreate_flag;
+
+ name = pq_getmsgstring(_message);
+ create_flag = pq_getmsgint(_message, 2);
+ pq_getmsgend(_message);
+
+ if (create_flag)
+ SetGUCOptionFlag(name, GUC_REPORT);
+ else
+ UnsetGUCOptionFlag(name, GUC_REPORT);
+
+ send_ready_for_query = true;
+}
+break;
+
case 'S': /* sync */
pq_getmsgend(_message);
finish_xact_command();
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 53d1d9a06a..b023b1a020 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2531,6 +2531,37 @@ BeginReportingGUCOptions(void)
}
}
+/*
+ * Allow to set / unset dynamicaly flags to GUC variables
+ */
+void
+SetGUCOptionFlag(const char *name, int flag)
+{
+ struct config_generic *conf;
+
+ /* only GUC_REPORT flag is supported now */
+ Assert(flag == GUC_REPORT);
+
+ conf = find_option(name, false, true, ERROR);
+ conf->flags |= flag;
+
+ if (flag == GUC_REPORT)
+ /* force transmit value of related option to client Parameter Status */
+ ReportGUCOption(conf);
+}
+
+void
+UnsetGUCOptionFlag(const char *name, int flag)
+{
+ struct config_generic *conf;
+
+ /* only GUC_REPORT flag is supported now */
+ Assert(flag == GUC_REPORT);
+
+ conf = find_option(name, false, true, ERROR);
+ conf->flags &= ~flag;
+}
+
/*
* ReportChangedGUCOptions: report recently-changed GUC_REPORT variables
*
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 97f7d97220..5276da54fc 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3859,6 +3859,7 @@ SyncVariables(void)
{
char vbuf[32];
const char *server_version;
+ int res;
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
@@ -3888,6 +3889,15 @@ SyncVariables(void)
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
+
+ /* link role GUC when it is needed for prompt */
+ if (pset.prompt_shows_role)
+ res = PQlinkParameterStatus(pset.db, "role");
+ else
+ res = PQunlinkParameterStatus(pset.db, "role");
+
+ if (res < 0)
+ pg_log_info("%s", PQerrorMessage(pset.db));
}
/*
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..b0f5158c59 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,41 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
if (pset.db)
strlcpy(buf,
Re: proposal: psql: show current user in prompt
Hi
rebased version + fix warning possibly uninitialized variable
Regards
Pavel
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index b11d9a6ba3..f774ffa310 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -5401,6 +5401,43 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
+
+ReportGUC (F)
+
+
+
+ Byte1('r')
+
+
+ Identifies the message type. ReportGUC is sent by
+ frontend when the changes of specified GUC option
+ should be (or should not be) reported to state parameter.
+
+
+
+
+
+ String
+
+
+ The name of GUC option.
+
+
+
+
+
+ Int16
+
+
+ 1 if reporting should be enables, 0 if reporting should be
+ disabled.
+
+
+
+
+
+
+
RowDescription (B)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc422373d6..569f50fe7d 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4551,7 +4551,24 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is set to the database session user name.
+
+
+
+ This substitution requires PostgreSQL
+ version 16 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 01b6cc1f7d..54a3f5c1e9 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -451,6 +451,11 @@ SocketBackend(StringInfo inBuf)
doing_extended_query_message = false;
break;
+ case 'r':/* report GUC */
+ maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
+ doing_extended_query_message = false;
+ break;
+
default:
/*
@@ -4823,6 +4828,24 @@ PostgresMain(const char *dbname, const char *username)
pq_flush();
break;
+ case 'r': /* report GUC */
+{
+ const char *name;
+ intcreate_flag;
+
+ name = pq_getmsgstring(_message);
+ create_flag = pq_getmsgint(_message, 2);
+ pq_getmsgend(_message);
+
+ if (create_flag)
+ SetGUCOptionFlag(name, GUC_REPORT);
+ else
+ UnsetGUCOptionFlag(name, GUC_REPORT);
+
+ send_ready_for_query = true;
+}
+break;
+
case 'S': /* sync */
pq_getmsgend(_message);
finish_xact_command();
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 9dd624b3ae..e0dccd1fa3 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2531,6 +2531,37 @@ BeginReportingGUCOptions(void)
}
}
+/*
+ * Allow to set / unset dynamicaly flags to GUC variables
+ */
+void
+SetGUCOptionFlag(const char *name, int flag)
+{
+ struct config_generic *conf;
+
+ /* only GUC_REPORT flag is supported now */
+ Assert(flag == GUC_REPORT);
+
+ conf = find_option(name, false, true, ERROR);
+ conf->flags |= flag;
+
+ if (flag == GUC_REPORT)
+ /* force transmit value of related option to client Parameter Status */
+ ReportGUCOption(conf);
+}
+
+void
+UnsetGUCOptionFlag(const char *name, int flag)
+{
+ struct config_generic *conf;
+
+ /* only GUC_REPORT flag is supported now */
+ Assert(flag == GUC_REPORT);
+
+ conf = find_option(name, false, true, ERROR);
+ conf->flags &= ~flag;
+}
+
/*
* ReportChangedGUCOptions: report recently-changed GUC_REPORT variables
*
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 97f7d97220..5276da54fc 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3859,6 +3859,7 @@ SyncVariables(void)
{
char vbuf[32];
const char *server_version;
+ int res;
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
@@ -3888,6 +3889,15 @@ SyncVariables(void)
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
+
+ /* link role GUC when it is needed for prompt */
+ if (pset.prompt_shows_role)
+ res = PQlinkParameterStatus(pset.db, "role");
+ else
+ res = PQunlinkParameterStatus(pset.db, "role");
+
+ if (res < 0)
+ pg_log_info("%s", PQerrorMessage(pset.db));
}
/*
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..b0f5158c59 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,41 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
if (pset.db)
strlcpy(buf, session_username(), sizeof(buf));
break;
+ /* DB server user role */
+case 'N':
+ if (pset.db)
+ {
+
Re: proposal: psql: show current user in prompt
st 5. 4. 2023 v 18:40 odesílatel Pavel Stehule
napsal:
>
>
> st 5. 4. 2023 v 17:47 odesílatel Robert Haas
> napsal:
>
>> On Wed, Apr 5, 2023 at 11:34 AM Pavel Stehule
>> 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
>
here is patch with setting GUC_REPORT on role only when it is required by
prompt
Regards
Pavel
>
> Regards
>
> Pavel
>
>
>>
>> --
>> Robert Haas
>> EDB: http://www.enterprisedb.com
>>
>
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index b11d9a6ba3..f774ffa310 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -5401,6 +5401,43 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
+
+ReportGUC (F)
+
+
+
+ Byte1('r')
+
+
+ Identifies the message type. ReportGUC is sent by
+ frontend when the changes of specified GUC option
+ should be (or should not be) reported to state parameter.
+
+
+
+
+
+ String
+
+
+ The name of GUC option.
+
+
+
+
+
+ Int16
+
+
+ 1 if reporting should be enables, 0 if reporting should be
+ disabled.
+
+
+
+
+
+
+
RowDescription (B)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index f4f25d1b07..962bf65f95 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4552,7 +4552,24 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is set to the database session user name.
+
+
+
+ This substitution requires PostgreSQL
+ version 16 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 01b6cc1f7d..54a3f5c1e9 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -451,6 +451,11 @@ SocketBackend(StringInfo inBuf)
doing_extended_query_message = false;
break;
+ case 'r':/* report GUC */
+ maxmsglen = PQ_SMALL_MESSAGE_LIMIT;
+ doing_extended_query_message = false;
+ break;
+
default:
/*
@@ -4823,6 +4828,24 @@ PostgresMain(const char *dbname, const char *username)
pq_flush();
break;
+ case 'r': /* report GUC */
+{
+ const char *name;
+ intcreate_flag;
+
+ name = pq_getmsgstring(_message);
+ create_flag = pq_getmsgint(_message, 2);
+ pq_getmsgend(_message);
+
+ if (create_flag)
+ SetGUCOptionFlag(name, GUC_REPORT);
+ else
+ UnsetGUCOptionFlag(name, GUC_REPORT);
+
+ send_ready_for_query = true;
+}
+break;
+
case 'S': /* sync */
pq_getmsgend(_message);
finish_xact_command();
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ea67cfa5e5..ee8d831e1c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2532,6 +2532,37 @@ BeginReportingGUCOptions(void)
}
}
+/*
+ * Allow to set / unset dynamicaly flags to GUC variables
+ */
+void
+SetGUCOptionFlag(const char *name, int flag)
+{
+ struct config_generic *conf;
+
+ /* only GUC_REPORT flag is supported now */
+ Assert(flag == GUC_REPORT);
+
+ conf = find_option(name, false, true, ERROR);
+ conf->flags |= flag;
+
+ if (flag == GUC_REPORT)
+ /* force transmit value of related option to client Parameter Status */
+ ReportGUCOption(conf);
+}
+
+void
+UnsetGUCOptionFlag(const char *name, int flag)
+{
+ struct config_generic *conf;
+
+ /* only GUC_REPORT flag is supported now */
+ Assert(flag == GUC_REPORT);
+
+ conf = find_option(name, false, true,
Re: proposal: psql: show current user in prompt
st 5. 4. 2023 v 17:47 odesílatel Robert Haas napsal: > On Wed, Apr 5, 2023 at 11:34 AM Pavel Stehule > 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 >
Re: proposal: psql: show current user in prompt
On Wed, Apr 5, 2023 at 11:34 AM Pavel Stehule 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
Re: proposal: psql: show current user in prompt
st 5. 4. 2023 v 16:02 odesílatel Robert Haas napsal: > On Wed, Apr 5, 2023 at 9:56 AM Tom Lane wrote: > > Robert Haas writes: > > > On Tue, Apr 4, 2023 at 12:42 PM Tom Lane 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. > > 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? Regards Pavel > > -- > Robert Haas > EDB: http://www.enterprisedb.com >
Re: proposal: psql: show current user in prompt
st 5. 4. 2023 v 15:56 odesílatel Tom Lane napsal: > Robert Haas writes: > > On Tue, Apr 4, 2023 at 12:42 PM Tom Lane 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 ... > I agree with this analyze Regards Pavel > > regards, tom lane >
Re: proposal: psql: show current user in prompt
On Wed, Apr 5, 2023 at 9:56 AM Tom Lane wrote: > Robert Haas writes: > > On Tue, Apr 4, 2023 at 12:42 PM Tom Lane 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. Maybe there's a better way, that just seemed like the obvious design. -- Robert Haas EDB: http://www.enterprisedb.com
Re: proposal: psql: show current user in prompt
Robert Haas writes: > On Tue, Apr 4, 2023 at 12:42 PM Tom Lane 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
Re: proposal: psql: show current user in prompt
On Tue, Apr 4, 2023 at 12:42 PM Tom Lane 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. I don't think we can just keep adding stuff to the set of things that gets reported for everyone. It doesn't scale. We need a really good reason to enlarge the set of values reported for all users, and I don't think this meets that bar. -- Robert Haas EDB: http://www.enterprisedb.com
Re: proposal: psql: show current user in prompt
út 4. 4. 2023 v 21:11 odesílatel Pavel Stehule
napsal:
>
>
> út 4. 4. 2023 v 20:50 odesílatel Pavel Stehule
> napsal:
>
>>
>>
>> út 4. 4. 2023 v 19:55 odesílatel Tom Lane napsal:
>>
>>> Pavel Stehule writes:
>>> > út 4. 4. 2023 v 18:42 odesílatel Tom Lane 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;
>
> ?
>
>
attached updated patch
Regards
Pavel
>
>
>
>>
>> Regards
>>
>> Pavel
>>
>>
>>
>>
>>
>>
>>>
>>> regards, tom lane
>>>
>>
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 9f72dd29d8..966cce9559 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2482,6 +2482,7 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
in_hot_standby,
is_superuser,
session_authorization,
+ role,
DateStyle,
IntervalStyle,
TimeZone,
@@ -2496,7 +2497,8 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
9.0;
default_transaction_read_only and
in_hot_standby were not reported by releases before
- 14.)
+ 14;
+ role was not reported by releases before 16;)
Note that
server_version,
server_encoding and
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 29bbec2188..98669fc18a 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4540,7 +4540,24 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is set to the database session user name.
+
+
+
+ This substitution requires PostgreSQL
+ version 16 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 8062589efd..3eec4768b3 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4174,7 +4174,7 @@ struct config_string ConfigureNamesString[] =
{"role", PGC_USERSET, UNGROUPED,
gettext_noop("Sets the current role."),
NULL,
- GUC_IS_NAME | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_REST
+ GUC_IS_NAME | GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_REST
},
_string,
"none",
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..7399bacd5f 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,41 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
if (pset.db)
strlcpy(buf, session_username(), sizeof(buf));
break;
+ /* DB server user role */
+case 'N':
+ if (pset.db)
+ {
+ int minServerMajor;
+ int serverMajor;
+ const char *rolename;
+
+ /*
+ * This feature requires GUC
Re: proposal: psql: show current user in prompt
út 4. 4. 2023 v 20:50 odesílatel Pavel Stehule napsal: > > > út 4. 4. 2023 v 19:55 odesílatel Tom Lane napsal: > >> Pavel Stehule writes: >> > út 4. 4. 2023 v 18:42 odesílatel Tom Lane 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 >> >
Re: proposal: psql: show current user in prompt
út 4. 4. 2023 v 19:55 odesílatel Tom Lane napsal: > Pavel Stehule writes: > > út 4. 4. 2023 v 18:42 odesílatel Tom Lane 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 >
Re: proposal: psql: show current user in prompt
Pavel Stehule writes: > út 4. 4. 2023 v 18:42 odesílatel Tom Lane 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
Re: proposal: psql: show current user in prompt
út 4. 4. 2023 v 18:42 odesílatel Tom Lane napsal:
> Kirk Wolak 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.
>
If I understand to next comment correctly, the overhead should not be too
big
/*
* ReportChangedGUCOptions: report recently-changed GUC_REPORT variables
*
* This is called just before we wait for a new client query.
*
* By handling things this way, we ensure that a ParameterStatus message
* is sent at most once per variable per query, even if the variable
* changed multiple times within the query. That's quite possible when
* using features such as function SET clauses. Function SET clauses
* also tend to cause values to change intraquery but eventually revert
* to their prevailing values; ReportGUCOption is responsible for avoiding
* redundant reports in such cases.
*/
>
> 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.
>
ok
>
> 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.
>
It should be a separate patch?
Updated patch attached
Regards
Pavel
>
> regards, tom lane
>
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 9f72dd29d8..966cce9559 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2482,6 +2482,7 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
in_hot_standby,
is_superuser,
session_authorization,
+ role,
DateStyle,
IntervalStyle,
TimeZone,
@@ -2496,7 +2497,8 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
9.0;
default_transaction_read_only and
in_hot_standby were not reported by releases before
- 14.)
+ 14;
+ role was not reported by releases before 16;)
Note that
server_version,
server_encoding and
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 29bbec2188..330c085329 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4540,7 +4540,26 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is none. The value is same like
+ the value of the parameter role (can be
+ displayed by SHOW.
+
+
+
+ This substitution requires PostgreSQL
+ version 15 and up. When you use older version, the empty string
+ is used instead.
+
+
+
+
+
%n
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 8062589efd..3eec4768b3 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4174,7 +4174,7 @@ struct config_string ConfigureNamesString[] =
{"role", PGC_USERSET, UNGROUPED,
gettext_noop("Sets the current role."),
NULL,
- GUC_IS_NAME | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_REST
+ GUC_IS_NAME | GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_REST
},
_string,
"none",
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..a304fe1868 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,35 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
if (pset.db)
strlcpy(buf, session_username(), sizeof(buf));
break;
+ /* DB server user role */
+case 'N':
+ if (pset.db)
+ {
+ int minServerMajor;
+ int serverMajor;
+ const char *val;
+
+ /*
+ * This feature requires GUC "role" to be marked
+ * as GUC_REPORT. Without it is hard to specify fallback
+ * result. Returning empty value can be messy, returning
+ * PQuser like session_username can be messy too.
+ * Exec query is not too practical too, because it doesn't
+ * work when session is not in transactional state, and
+ *
Re: proposal: psql: show current user in prompt
Kirk Wolak 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
Re: proposal: psql: show current user in prompt
On Sat, Feb 4, 2023 at 3:33 PM Pavel Stehule wrote: > Hi > > pá 3. 2. 2023 v 21:43 odesílatel Pavel Stehule > napsal: > >> >> >> pá 3. 2. 2023 v 21:21 odesílatel Tom Lane napsal: >> >>> Pavel Stehule 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 >>> >> I've tested this w/regards to psql. Latest commit. It works as described. 'none' is displayed for the default role. (SET ROLE DEFAULT), otherwise the specific ROLE is displayed. I tried this patch on 15.2, but guc_files.c does not exist in that version, so it did not install. I also tried applying the %T patch, but since they touch the same file, it would not install with it, without rebasing, repatching. The Docs are updated, and it's a relatively contained patch. Changed status to Ready for Committer. (100% Guessing here...) Kirk
Re: proposal: psql: show current user in prompt
Hi
pá 3. 2. 2023 v 21:43 odesílatel Pavel Stehule
napsal:
>
>
> pá 3. 2. 2023 v 21:21 odesílatel Tom Lane napsal:
>
>> Pavel Stehule 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
>>
>
From 3b42bd47db4488f320b166d4c580193bd2c02de8 Mon Sep 17 00:00:00 2001
From: "ok...@github.com"
Date: Sat, 4 Feb 2023 21:16:30 +0100
Subject: [PATCH] implementation of psql prompt substitution %N
this substitution shows used role specified by command SET ROLE.
it can show string ERRA (SQL code - feature not supported)
when the variable "role" is not reported (PostgreSQL 15 and older).
---
doc/src/sgml/libpq.sgml | 4 +++-
doc/src/sgml/ref/psql-ref.sgml | 21 +++-
src/backend/utils/misc/guc_tables.c | 2 +-
src/bin/psql/prompt.c | 37 +
4 files changed, 61 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 0e7ae70c70..6a2f92b368 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2270,6 +2270,7 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
in_hot_standby,
is_superuser,
session_authorization,
+ role,
DateStyle,
IntervalStyle,
TimeZone,
@@ -2284,7 +2285,8 @@ const char *PQparameterStatus(const PGconn *conn, const char *paramName);
9.0;
default_transaction_read_only and
in_hot_standby were not reported by releases before
- 14.)
+ 14;
+ role was not reported by releases before 16;)
Note that
server_version,
server_encoding and
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11..5e313f06ab 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4509,7 +4509,26 @@ testdb= INSERT INTO my_table VALUES (:'content');
The port number at which the database server is listening.
-
+
+%N
+
+
+ The database role name. This value is specified by command
+ SET ROLE. Until execution of this command
+ the value is none. The value is same like
+ the value of the parameter role (can be
+ displayed by SHOW.
+
+
+
+ This substitution requires PostgreSQL
+ version 15 and up. When you use older version, the error value
+ ERR0A000 is used instead.
+
+
+
+
+
%n
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index b46e3b8c55..3188fd015d 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4144,7 +4144,7 @@ struct config_string ConfigureNamesString[] =
{"role", PGC_USERSET, UNGROUPED,
gettext_noop("Sets the current role."),
NULL,
- GUC_IS_NAME | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_REST
+ GUC_IS_NAME | GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_NOT_WHILE_SEC_REST
},
_string,
"none",
diff --git a/src/bin/psql/prompt.c b/src/bin/psql/prompt.c
index 969cd9908e..ef720c8496 100644
--- a/src/bin/psql/prompt.c
+++ b/src/bin/psql/prompt.c
@@ -165,6 +165,43 @@ get_prompt(promptStatus_t status, ConditionalStack cstack)
if (pset.db)
strlcpy(buf, session_username(), sizeof(buf));
break;
+ /* DB server user role */
+case 'N':
+ if (pset.db)
+ {
+ int minServerMajor;
+ int serverMajor;
+
+ /*
+ * This feature requires GUC "role" to be marked
+ * as GUC_REPORT. Without it is hard to specify fallback
+ * result. Returning
Re: proposal: psql: show current user in prompt
pá 3. 2. 2023 v 21:21 odesílatel Tom Lane napsal: > Pavel Stehule 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 >
Re: proposal: psql: show current user in prompt
Pavel Stehule 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
Re: proposal: psql: show current user in prompt
pá 3. 2. 2023 v 20:42 odesílatel Corey Huinker napsal: > > > On Fri, Feb 3, 2023 at 9:56 AM Pavel Stehule > 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. > I was little bit lazy, I am sorry. I did it in one my experimental branch. Both patches are PoC, and there are not documentation yet. I will separate it. > 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. > yes, I agree so it can be useful - it is not my idea - and it is maybe partially deduced from some other databases. Both patches are very simple - and they use almost already prepared infrastructure. Regards Pavel
Re: proposal: psql: show current user in prompt
On Fri, Feb 3, 2023 at 9:56 AM Pavel Stehule 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.
