Thread
Commits
-
Fix misuse of PqMsg_Close.
- 6475e663b3c6 17.0 landed
-
Wrong usage of pqMsg_Close message code?
Pavel Stehule <pavel.stehule@gmail.com> — 2023-08-28T11:38:57Z
Hi I workin with protocol and reading related code. I found in routine EndCommand one strange thing void EndCommand(const QueryCompletion *qc, CommandDest dest, bool force_undecorated_output) { <-->char<--><-->completionTag[COMPLETION_TAG_BUFSIZE]; <-->Size<--><-->len; <-->switch (dest) <-->{ <--><-->case DestRemote: <--><-->case DestRemoteExecute: <--><-->case DestRemoteSimple: <--><--><-->len = BuildQueryCompletionString(completionTag, qc, <--><--><--><--><--><--><--><--><--><--><--> force_undecorated_output); <--><--><-->pq_putmessage(PqMsg_Close, completionTag, len + 1); <--><-->case DestNone: There is message PqMsgClose, but this should be used from client side. Here should be used PqMsg_CommandComplete instead? Regards Pavel -
Re: Wrong usage of pqMsg_Close message code?
Aleksander Alekseev <aleksander@timescale.com> — 2023-08-28T12:00:26Z
Hi Pavel, > There is message PqMsgClose, but this should be used from client side. Here should be used PqMsg_CommandComplete instead? It seems so. This change was introduced in f4b54e1ed98 [1]: ``` --- a/src/backend/tcop/dest.c +++ b/src/backend/tcop/dest.c @@ -176,7 +176,7 @@ EndCommand(const QueryCompletion *qc, CommandDest dest, bool force_undecorated_o len = BuildQueryCompletionString(completionTag, qc, force_undecorated_output); - pq_putmessage('C', completionTag, len + 1); + pq_putmessage(PqMsg_Close, completionTag, len + 1); case DestNone: case DestDebug ``` It should have been PqMsg_CommandComplete. [1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f4b54e1ed98 -- Best regards, Aleksander Alekseev -
Re: Wrong usage of pqMsg_Close message code?
Pavel Stehule <pavel.stehule@gmail.com> — 2023-08-28T14:01:29Z
Hi po 28. 8. 2023 v 14:00 odesílatel Aleksander Alekseev < aleksander@timescale.com> napsal: > Hi Pavel, > > > There is message PqMsgClose, but this should be used from client side. > Here should be used PqMsg_CommandComplete instead? > > It seems so. This change was introduced in f4b54e1ed98 [1]: > > ``` > --- a/src/backend/tcop/dest.c > +++ b/src/backend/tcop/dest.c > @@ -176,7 +176,7 @@ EndCommand(const QueryCompletion *qc, CommandDest > dest, bool force_undecorated_o > > len = BuildQueryCompletionString(completionTag, qc, > > force_undecorated_output); > - pq_putmessage('C', completionTag, len + 1); > + pq_putmessage(PqMsg_Close, completionTag, len + 1); > > case DestNone: > case DestDebug > ``` > > It should have been PqMsg_CommandComplete. > > [1]: > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f4b54e1ed98 here is a patch - all tests passed Regards Pavel > > > -- > Best regards, > Aleksander Alekseev > -
Re: Wrong usage of pqMsg_Close message code?
Aleksander Alekseev <aleksander@timescale.com> — 2023-08-28T14:26:29Z
Hi, > here is a patch - all tests passed LGTM and added to the nearest CF just in case [1]. [1]: https://commitfest.postgresql.org/44/4523/ -- Best regards, Aleksander Alekseev
-
Re: Wrong usage of pqMsg_Close message code?
Tatsuo Ishii <ishii@sraoss.co.jp> — 2023-08-28T21:12:00Z
>> Hi Pavel, >> >> > There is message PqMsgClose, but this should be used from client side. >> Here should be used PqMsg_CommandComplete instead? >> >> It seems so. This change was introduced in f4b54e1ed98 [1]: >> >> ``` >> --- a/src/backend/tcop/dest.c >> +++ b/src/backend/tcop/dest.c >> @@ -176,7 +176,7 @@ EndCommand(const QueryCompletion *qc, CommandDest >> dest, bool force_undecorated_o >> >> len = BuildQueryCompletionString(completionTag, qc, >> >> force_undecorated_output); >> - pq_putmessage('C', completionTag, len + 1); >> + pq_putmessage(PqMsg_Close, completionTag, len + 1); >> >> case DestNone: >> case DestDebug >> ``` >> >> It should have been PqMsg_CommandComplete. >> >> [1]: >> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f4b54e1ed98 > > > here is a patch - all tests passed I think EndReplicationCommand needs to be fixed as well. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp -
Re: Wrong usage of pqMsg_Close message code?
Michael Paquier <michael@paquier.xyz> — 2023-08-28T22:53:37Z
On Tue, Aug 29, 2023 at 06:12:00AM +0900, Tatsuo Ishii wrote: > I think EndReplicationCommand needs to be fixed as well. Yeah, both of you are right here. Anyway, it seems to me that there is a bit more going on in protocol.h. I have noticed two more things that are incorrect: - HandleParallelMessage is missing a message for 'P', but I think that we should have a code for it as well as part of the parallel query protocol. - PqMsg_Terminate can be sent by the frontend *and* the backend, see fe-connect.c and parallel.c. However, protocol.h documents it as a frontend-only code. -- Michael
-
Re: Wrong usage of pqMsg_Close message code?
Tatsuo Ishii <ishii@sraoss.co.jp> — 2023-08-29T01:04:24Z
> Yeah, both of you are right here. Anyway, it seems to me that there > is a bit more going on in protocol.h. I have noticed two more things > that are incorrect: > - HandleParallelMessage is missing a message for 'P', but I think that > we should have a code for it as well as part of the parallel query > protocol. I did not know this. Why is this not explained in the frontend/backend protocol document? > - PqMsg_Terminate can be sent by the frontend *and* the backend, see > fe-connect.c and parallel.c. However, protocol.h documents it as a > frontend-only code. I do not blame protocol.h because our frontend/backend protocol document also stats that it's a frontend only message. Someone who started to use 'X' in backend should have added that in the documentation. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
-
Re: Wrong usage of pqMsg_Close message code?
Michael Paquier <michael@paquier.xyz> — 2023-08-29T07:39:51Z
On Tue, Aug 29, 2023 at 10:04:24AM +0900, Tatsuo Ishii wrote: >> Yeah, both of you are right here. Anyway, it seems to me that there >> is a bit more going on in protocol.h. I have noticed two more things >> that are incorrect: >> - HandleParallelMessage is missing a message for 'P', but I think that >> we should have a code for it as well as part of the parallel query >> protocol. > > I did not know this. Why is this not explained in the frontend/backend > protocol document? Hmm. Thinking more about it, I am actually not sure that we need to do that in this case, so perhaps things are OK as they stand for this one. >> - PqMsg_Terminate can be sent by the frontend *and* the backend, see >> fe-connect.c and parallel.c. However, protocol.h documents it as a >> frontend-only code. > > I do not blame protocol.h because our frontend/backend protocol > document also stats that it's a frontend only message. Someone who > started to use 'X' in backend should have added that in the > documentation. Actually, this may be OK as well as it stands. One can also say that the parallel processing is out of this scope, being used only internally. I cannot keep wondering whether we should put more efforts in documenting the parallel worker/leader protocol. That's internal to the backend and out of the scope of this thread, still.. -- Michael
-
Re: Wrong usage of pqMsg_Close message code?
Tom Lane <tgl@sss.pgh.pa.us> — 2023-08-29T14:01:47Z
Michael Paquier <michael@paquier.xyz> writes: > Actually, this may be OK as well as it stands. One can also say that > the parallel processing is out of this scope, being used only > internally. I cannot keep wondering whether we should put more > efforts in documenting the parallel worker/leader protocol. That's > internal to the backend and out of the scope of this thread, still.. Yeah. I'm not sure whether the leader/worker protocol needs better documentation, but the parts of it that are not common with the frontend protocol should NOT be documented in protocol.sgml. That would just confuse authors of frontend code. I don't mind having constants for the leader/worker protocol in protocol.h, as long as they're in a separate section that's clearly marked as relevant only for server-internal parallelism. regards, tom lane
-
Re: Wrong usage of pqMsg_Close message code?
Nathan Bossart <nathandbossart@gmail.com> — 2023-08-29T16:15:55Z
Hi everyone, Thanks for the report. I'll get this fixed up. My guess is that this was leftover from an earlier version of the patch that used the same macro for identical protocol characters. On Tue, Aug 29, 2023 at 10:01:47AM -0400, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: >> Actually, this may be OK as well as it stands. One can also say that >> the parallel processing is out of this scope, being used only >> internally. I cannot keep wondering whether we should put more >> efforts in documenting the parallel worker/leader protocol. That's >> internal to the backend and out of the scope of this thread, still.. > > Yeah. I'm not sure whether the leader/worker protocol needs better > documentation, but the parts of it that are not common with the > frontend protocol should NOT be documented in protocol.sgml. > That would just confuse authors of frontend code. > > I don't mind having constants for the leader/worker protocol in > protocol.h, as long as they're in a separate section that's clearly > marked as relevant only for server-internal parallelism. +1, I left the parallel stuff (and a couple other things) out in the first round to avoid prolonging the naming discussion, but we can continue to add to protocol.h. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
-
Re: Wrong usage of pqMsg_Close message code?
Nathan Bossart <nathandbossart@gmail.com> — 2023-08-29T21:11:06Z
On Tue, Aug 29, 2023 at 09:15:55AM -0700, Nathan Bossart wrote: > Thanks for the report. I'll get this fixed up. My guess is that this was > leftover from an earlier version of the patch that used the same macro for > identical protocol characters. I plan to commit the attached patch shortly. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
-
Re: Wrong usage of pqMsg_Close message code?
Michael Paquier <michael@paquier.xyz> — 2023-08-29T22:56:33Z
On Tue, Aug 29, 2023 at 02:11:06PM -0700, Nathan Bossart wrote: > I plan to commit the attached patch shortly. WFM. -- Michael
-
Re: Wrong usage of pqMsg_Close message code?
Nathan Bossart <nathandbossart@gmail.com> — 2023-08-30T01:35:06Z
On Wed, Aug 30, 2023 at 07:56:33AM +0900, Michael Paquier wrote: > On Tue, Aug 29, 2023 at 02:11:06PM -0700, Nathan Bossart wrote: >> I plan to commit the attached patch shortly. > > WFM. Committed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com