Re: Wrong usage of pqMsg_Close message code?

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

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

Hi

po 28. 8. 2023 v 14:00 odesílatel Aleksander Alekseev <
aleksander(at)timescale(dot)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
>

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

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

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

> 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

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

Michael Paquier <michael(at)paquier(dot)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

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(at)paquier(dot)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

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

On Tue, Aug 29, 2023 at 02:11:06PM -0700, Nathan Bossart wrote:
> I plan to commit the attached patch shortly.

WFM.
--
Michael

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