Using defines for protocol characters

On 2023-Aug-03, Dave Cramer wrote:

Greetings,

Attached is a patch which introduces a file protocol.h. Instead of using
the actual characters everywhere in the code this patch names the
characters and removes the comments beside each usage.

I saw this one last week. I think it's a very idea (and I fact I had
thought of doing it when I last messed with libpq code).

I don't really like the name pattern you've chosen though; I think we
need to have a common prefix in the defines. Maybe prepending PQMSG_ to
each name would be enough. And maybe turn the _RESPONSE and _REQUEST
suffixes you added into prefixes as well, so instead of PARSE_REQUEST
you could make it PQMSG_REQ_PARSE, PQMSG_RESP_BIND_COMPLETE and so
on.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.

On Thu, 3 Aug 2023 at 11:59, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2023-Aug-03, Dave Cramer wrote:

Greetings,

Attached is a patch which introduces a file protocol.h. Instead of using
the actual characters everywhere in the code this patch names the
characters and removes the comments beside each usage.

I saw this one last week. I think it's a very idea (and I fact I had
thought of doing it when I last messed with libpq code).

I don't really like the name pattern you've chosen though; I think we
need to have a common prefix in the defines. Maybe prepending PQMSG_ to
each name would be enough. And maybe turn the _RESPONSE and _REQUEST
suffixes you added into prefixes as well, so instead of PARSE_REQUEST
you could make it PQMSG_REQ_PARSE, PQMSG_RESP_BIND_COMPLETE and so
on.

That becomes trivial to do now that the names are defined. I presumed
someone would object to the names.
I'm fine with the names you propose, but I suggest we wait to see if anyone
objects.

Dave

On Thu, Aug 03, 2023 at 12:07:21PM -0600, Dave Cramer wrote:

On Thu, 3 Aug 2023 at 11:59, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I don't really like the name pattern you've chosen though; I think we
need to have a common prefix in the defines. Maybe prepending PQMSG_ to
each name would be enough. And maybe turn the _RESPONSE and _REQUEST
suffixes you added into prefixes as well, so instead of PARSE_REQUEST
you could make it PQMSG_REQ_PARSE, PQMSG_RESP_BIND_COMPLETE and so
on.

That becomes trivial to do now that the names are defined. I presumed
someone would object to the names.
I'm fine with the names you propose, but I suggest we wait to see if anyone
objects.

I'm okay with the proposed names as well.

+ * src/include/protocol.h

Could we put these definitions in an existing header such as
src/include/libpq/pqcomm.h? I see that's where the authentication request
codes live today.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Greetings,

Attached is a patch which introduces a file protocol.h. Instead of using
the actual characters everywhere in the code this patch names the
characters and removes the comments beside each usage.

+#define DESCRIBE_PREPARED 'S'
+#define DESCRIBE_PORTAL 'P'

You use these for Close message as well. I don't like the idea because
Close is different message from Describe message.

What about adding following for Close too use them instead?

#define CLOSE_PREPARED 'S'
#define CLOSE_PORTAL 'P'

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

Greetings,

Attached is a patch which introduces a file protocol.h. Instead of using
the actual characters everywhere in the code this patch names the
characters and removes the comments beside each usage.

+#define DESCRIBE_PREPARED 'S'
+#define DESCRIBE_PORTAL 'P'

You use these for Close message as well. I don't like the idea because
Close is different message from Describe message.

What about adding following for Close too use them instead?

#define CLOSE_PREPARED 'S'
#define CLOSE_PORTAL 'P'

Good catch.
I recall when writing this it was a bit hacky.
What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND instead
of duplicating them ?

Dave

On Thu, 3 Aug 2023 at 15:22, Dave Cramer <davecramer@gmail.com> wrote:

On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

Greetings,

Attached is a patch which introduces a file protocol.h. Instead of using
the actual characters everywhere in the code this patch names the
characters and removes the comments beside each usage.

+#define DESCRIBE_PREPARED 'S'
+#define DESCRIBE_PORTAL 'P'

You use these for Close message as well. I don't like the idea because
Close is different message from Describe message.

What about adding following for Close too use them instead?

#define CLOSE_PREPARED 'S'
#define CLOSE_PORTAL 'P'

Good catch.
I recall when writing this it was a bit hacky.
What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND instead
of duplicating them ?

While reviewing this I found a number of mistakes where I used
DATA_ROW_RESPONSE instead of DESCRIBE_REQUEST.

I can provide a patch now or wait until we resolve the above

On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

Greetings,

Attached is a patch which introduces a file protocol.h. Instead of using
the actual characters everywhere in the code this patch names the
characters and removes the comments beside each usage.

+#define DESCRIBE_PREPARED 'S'
+#define DESCRIBE_PORTAL 'P'

You use these for Close message as well. I don't like the idea because
Close is different message from Describe message.

What about adding following for Close too use them instead?

#define CLOSE_PREPARED 'S'
#define CLOSE_PORTAL 'P'

Good catch.
I recall when writing this it was a bit hacky.
What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND instead
of duplicating them ?

Nice. Looks good to me.

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

On Thu, 3 Aug 2023 at 16:59, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

Greetings,

Attached is a patch which introduces a file protocol.h. Instead of

using

the actual characters everywhere in the code this patch names the
characters and removes the comments beside each usage.

+#define DESCRIBE_PREPARED 'S'
+#define DESCRIBE_PORTAL 'P'

You use these for Close message as well. I don't like the idea because
Close is different message from Describe message.

What about adding following for Close too use them instead?

#define CLOSE_PREPARED 'S'
#define CLOSE_PORTAL 'P'

Good catch.
I recall when writing this it was a bit hacky.
What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND

instead

of duplicating them ?

Nice. Looks good to me.

New patch attached which uses PREPARED_SUB_COMMAND and
PORTAL_SUB_COMMAND instead
and uses
PQMSG_REQ_* and PQMSG_RESP_* as per Alvaro's suggestion

Dave Cramer

On Thu, 3 Aug 2023 at 16:59, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

Greetings,

Attached is a patch which introduces a file protocol.h. Instead of

using

the actual characters everywhere in the code this patch names the
characters and removes the comments beside each usage.

+#define DESCRIBE_PREPARED 'S'
+#define DESCRIBE_PORTAL 'P'

You use these for Close message as well. I don't like the idea because
Close is different message from Describe message.

What about adding following for Close too use them instead?

#define CLOSE_PREPARED 'S'
#define CLOSE_PORTAL 'P'

Good catch.
I recall when writing this it was a bit hacky.
What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND

instead

of duplicating them ?

Nice. Looks good to me.

New patch attached which uses PREPARED_SUB_COMMAND and
PORTAL_SUB_COMMAND instead
and uses
PQMSG_REQ_* and PQMSG_RESP_* as per Alvaro's suggestion

Looks good to me.

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

On 2023-Aug-03, Dave Cramer wrote:

New patch attached which uses PREPARED_SUB_COMMAND and
PORTAL_SUB_COMMAND instead

Hmm, I would keep the prefix in this case and make the message type a
second prefix, with the subtype last -- PQMSG_CLOSE_PREPARED,
PQMSG_DESCRIBE_PORTAL and so on.

You define PASSWORD and GSS both with 'p', which I think is bogus; in
some place that isn't doing GSS, you've replaced 'p' with the GSS one
(CheckSASLAuth). I think it'd be better to give 'p' a single name, and
not try to distinguish which is PASSWORD and which is GSS, because
ultimately it's not important.

There are some unpatched places, such as basebackup_copy.c -- there are
several matches for /'.'/ that correspond to protocol chars in that file.
Also CopySendEndOfRow has one 'd', and desc.c has two 'C'.

I think fe-trace.c will require further adjustment of the comments for
each function. We could change them to be the symbol for each char, like so:

/* PQMSG_RESP_READY_FOR_QUERY */
static void
pqTraceOutputZ(FILE *f, const char *message, int *cursor)

Thanks

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Someone said that it is at least an order of magnitude more work to do
production software than a prototype. I think he is wrong by at least
an order of magnitude." (Brian Kernighan)

On Fri, 4 Aug 2023 at 03:44, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2023-Aug-03, Dave Cramer wrote:

New patch attached which uses PREPARED_SUB_COMMAND and
PORTAL_SUB_COMMAND instead

Hmm, I would keep the prefix in this case and make the message type a
second prefix, with the subtype last -- PQMSG_CLOSE_PREPARED,
PQMSG_DESCRIBE_PORTAL and so on.

I used

#define PQMSG_REQ_PREPARED 'S'
#define PQMSG_REQ_PORTAL 'P'

Duplicating them for CLOSE PREPARED|PORTAL seems awkward

You define PASSWORD and GSS both with 'p', which I think is bogus; in
some place that isn't doing GSS, you've replaced 'p' with the GSS one
(CheckSASLAuth). I think it'd be better to give 'p' a single name, and
not try to distinguish which is PASSWORD and which is GSS, because
ultimately it's not important.

Done

There are some unpatched places, such as basebackup_copy.c -- there are
several matches for /'.'/ that correspond to protocol chars in that file.
Also CopySendEndOfRow has one 'd', and desc.c has two 'C'.

I think fe-trace.c will require further adjustment of the comments for
each function. We could change them to be the symbol for each char, like
so:

/* PQMSG_RESP_READY_FOR_QUERY */
static void
pqTraceOutputZ(FILE *f, const char *message, int *cursor)

Thanks for reviewing see attached for fixes

Dave

Hi, I wondered if any consideration was given to using an enum instead
of all the #defines.

I guess, your patch would not be much different; you can still have
all the nice names and assign the appropriate values to the enum
values same as now, but using an enum you might also gain
type-checking in the code and also get warnings for the "switch"
statements if there are any cases accidentally omitted.

For example, see typedef enum LogicalRepMsgType [1]https://github.com/postgres/postgres/blob/eeb4eeea2c525c51767ffeafda0070b946f26ae8/src/include/replication/logicalproto.h#L57C31-L57C31.

------
[1]: https://github.com/postgres/postgres/blob/eeb4eeea2c525c51767ffeafda0070b946f26ae8/src/include/replication/logicalproto.h#L57C31-L57C31

Kind Regards,
Peter Smith
Fujitsu Australia

On 2023-Aug-07, Peter Smith wrote:

I guess, your patch would not be much different; you can still have
all the nice names and assign the appropriate values to the enum
values same as now, but using an enum you might also gain
type-checking in the code and also get warnings for the "switch"
statements if there are any cases accidentally omitted.

Hmm, I think omitting a 'default' clause (which is needed when you want
warnings for missing clauses) in a switch that handles protocol traffic
is not great, because the switch would misbehave when the network
counterpart sends a broken message. I'm not sure we want to do that.
It could become a serious security problem if confronted with a
malicious libpq.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

On Mon, 7 Aug 2023 at 03:10, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2023-Aug-07, Peter Smith wrote:

I guess, your patch would not be much different; you can still have
all the nice names and assign the appropriate values to the enum
values same as now, but using an enum you might also gain
type-checking in the code and also get warnings for the "switch"
statements if there are any cases accidentally omitted.

Hmm, I think omitting a 'default' clause (which is needed when you want
warnings for missing clauses) in a switch that handles protocol traffic
is not great, because the switch would misbehave when the network
counterpart sends a broken message. I'm not sure we want to do that.
It could become a serious security problem if confronted with a
malicious libpq.

Any other changes required ?

Dave

On Mon, Aug 7, 2023 at 1:19 PM Dave Cramer <davecramer@gmail.com> wrote:

Any other changes required ?

IMHO, the correspondence between the names in the patch and the
traditional names in the documentation could be stronger. For example,
the documentation mentions EmptyQueryResponse and
NegotiateProtocolVersion, but in this patch those become
PQMSG_RESP_NEGOTIATE_PROTOCOL and PQMSG_RESP_EMPTY_QUERY. I think we
could consider directly using the names from the documentation, right
down to capitalization, perhaps with a prefix, but I'm also totally
fine with this use of uppercase letters and underscores. But why not
do a strict translation, like EmptyQueryResponse ->
PQMSG_EMPTY_QUERY_RESPONSE, NegotiateProtocolVersion ->
PQMSG_NEGOTIATE_PROTOCOL_VERSION? To me at least, the current patch is
inventing new and slightly different names for things that already
have names...

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

IMHO, the correspondence between the names in the patch and the
traditional names in the documentation could be stronger. For example,
the documentation mentions EmptyQueryResponse and
NegotiateProtocolVersion, but in this patch those become
PQMSG_RESP_NEGOTIATE_PROTOCOL and PQMSG_RESP_EMPTY_QUERY. I think we
could consider directly using the names from the documentation, right
down to capitalization, perhaps with a prefix, but I'm also totally
fine with this use of uppercase letters and underscores. But why not
do a strict translation, like EmptyQueryResponse ->
PQMSG_EMPTY_QUERY_RESPONSE, NegotiateProtocolVersion ->
PQMSG_NEGOTIATE_PROTOCOL_VERSION? To me at least, the current patch is
inventing new and slightly different names for things that already
have names...

+1. For ease of greppability, maybe even PQMSG_EmptyQueryResponse
and so on? Then one grep would find both uses of the constants and
code/docs references. Not sure if the prefix should be all-caps or
not if we go this way.

regards, tom lane

On Mon, Aug 07, 2023 at 02:24:58PM -0400, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

IMHO, the correspondence between the names in the patch and the
traditional names in the documentation could be stronger. For example,
the documentation mentions EmptyQueryResponse and
NegotiateProtocolVersion, but in this patch those become
PQMSG_RESP_NEGOTIATE_PROTOCOL and PQMSG_RESP_EMPTY_QUERY. I think we
could consider directly using the names from the documentation, right
down to capitalization, perhaps with a prefix, but I'm also totally
fine with this use of uppercase letters and underscores. But why not
do a strict translation, like EmptyQueryResponse ->
PQMSG_EMPTY_QUERY_RESPONSE, NegotiateProtocolVersion ->
PQMSG_NEGOTIATE_PROTOCOL_VERSION? To me at least, the current patch is
inventing new and slightly different names for things that already
have names...

+1. For ease of greppability, maybe even PQMSG_EmptyQueryResponse
and so on? Then one grep would find both uses of the constants and
code/docs references. Not sure if the prefix should be all-caps or
not if we go this way.

+1 for Tom's suggestion. I have no strong opinion about the prefix, but I
do think easing greppability is worthwhile.

As mentioned earlier [0]/messages/by-id/20230803185356.GA1144430@nathanxps13, I think we should also consider putting the
definitions in pqcomm.h.

[0]: /messages/by-id/20230803185356.GA1144430@nathanxps13

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, Aug 7, 2023 at 2:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

+1. For ease of greppability, maybe even PQMSG_EmptyQueryResponse
and so on? Then one grep would find both uses of the constants and
code/docs references. Not sure if the prefix should be all-caps or
not if we go this way.

PqMsgEmptyQueryResponse or something like that seems better, if we
want to keep the current capitalization. I'm not a huge fan of the way
we vary our capitalization conventions so much all over the code base,
but I think we would at least do well to keep it consistent from one
end of a certain identifier to the other.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, 7 Aug 2023 at 12:59, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Aug 7, 2023 at 2:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

+1. For ease of greppability, maybe even PQMSG_EmptyQueryResponse
and so on? Then one grep would find both uses of the constants and
code/docs references. Not sure if the prefix should be all-caps or
not if we go this way.

PqMsgEmptyQueryResponse or something like that seems better, if we
want to keep the current capitalization. I'm not a huge fan of the way
we vary our capitalization conventions so much all over the code base,
but I think we would at least do well to keep it consistent from one
end of a certain identifier to the other.

I don't have a strong preference, but before I make the changes I'd like to
get consensus.

Can we vote or whatever it takes to decide on a naming pattern that is
acceptable ?

Dave