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]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f4b54e1ed98:
```
--- 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_olen = 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@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_olen = 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
Show quoted text
--
Best regards,
Aleksander Alekseev
Attachments:
fix_EmdCommandMessage.patchtext/x-patch; charset=US-ASCII; name=fix_EmdCommandMessage.patchDownload
diff --git a/src/backend/tcop/dest.c b/src/backend/tcop/dest.c
index 06d1872b9a..bd6085b7ed 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:
Hi,
here is a patch - all tests passed
LGTM and added to the nearest CF just in case [1]https://commitfest.postgresql.org/44/4523/.
[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_olen = 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=f4b54e1ed98here 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@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
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
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
Attachments:
v2-0001-Fix-misuse-of-PqMsg_Close.patchtext/x-diff; charset=us-asciiDownload
From 3a895c081fb26a2f818d07c7d28a54b94c278391 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
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
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