Subject: Re: Wrong usage of pqMsg_Close message code?

From: Tatsuo Ishii <[email-masked]>
Date: Tue, 29 Aug 2023 06:12:00 +0900 (JST)

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

---

From: Michael Paquier <[email-masked]>
Date: Tue, 29 Aug 2023 07:53:37 +0900

> 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

---

From: Tatsuo Ishii <[email-masked]>
Date: Tue, 29 Aug 2023 10:04:24 +0900 (JST)

> we should have a code for it as well as part of the parallel query

I did not know this. Why is this not explained in the frontend/backend
protocol document?

> fe-connect.c and parallel.c.  However, protocol.h documents it as a

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,

---

From: Michael Paquier <[email-masked]>
Date: Tue, 29 Aug 2023 16:39:51 +0900

> I did not know this. Why is this not explained in the frontend/backend

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.

> started to use 'X' in backend should have added that in the

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

---

From: Tom Lane <[email-masked]>
Date: Tue, 29 Aug 2023 10:01:47 -0400

> 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

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 29 Aug 2023 09:15:55 -0700

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.

> 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

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 29 Aug 2023 14:11:06 -0700

> identical protocol characters.

I plan to commit the attached patch shortly.

>From 3a895c081fb26a2f818d07c7d28a54b94c278391 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 29 Aug 2023 14:01:18 -0700
Subject: [PATCH v2 1/1] Fix misuse of PqMsg_Close.

EndCommand() and EndReplicationCommand() should use
PqMsg_CommandComplete instead.  Oversight in commit f4b54e1ed9.

Reported-by: Pavel Stehule, Tatsuo Ishii
Author: Pavel Stehule
Reviewed-by: Aleksander Alekseev, Michael Paquier
---
 src/backend/tcop/dest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/tcop/dest.c b/src/backend/tcop/dest.c
index 06d1872b9a..bc8494ee7d 100644
--- 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(PqMsg_Close, completionTag, len + 1);
+			pq_putmessage(PqMsg_CommandComplete, completionTag, len + 1);
 
 		case DestNone:
 		case DestDebug:
@@ -200,7 +200,7 @@ EndCommand(const QueryCompletion *qc, CommandDest dest, bool force_undecorated_o
 void
 EndReplicationCommand(const char *commandTag)
 {
-	pq_putmessage(PqMsg_Close, commandTag, strlen(commandTag) + 1);
+	pq_putmessage(PqMsg_CommandComplete, commandTag, strlen(commandTag) + 1);
 }
 
 /* ----------------
-- 
2.25.1

---

From: Michael Paquier <[email-masked]>
Date: Wed, 30 Aug 2023 07:56:33 +0900

> I plan to commit the attached patch shortly.

WFM.
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 29 Aug 2023 18:35:06 -0700

>> I plan to commit the attached patch shortly.

Committed.