Thread: Using defines for protocol characters

Using defines for protocol characters

From
Dave Cramer
Date:
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.


Dave Cramer
Attachment

Re: Using defines for protocol characters

From
Alvaro Herrera
Date:
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.



Re: Using defines for protocol characters

From
Dave Cramer
Date:


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 

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

Re: Using defines for protocol characters

From
Nathan Bossart
Date:
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



Re: Using defines for protocol characters

From
Tatsuo Ishii
Date:
> 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



Re: Using defines for protocol characters

From
Dave Cramer
Date:


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

Re: Using defines for protocol characters

From
Dave Cramer
Date:


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
 

Dave

Re: Using defines for protocol characters

From
Tatsuo Ishii
Date:
> 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



Re: Using defines for protocol characters

From
Dave Cramer
Date:



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
Attachment

Re: Using defines for protocol characters

From
Tatsuo Ishii
Date:
> 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



Re: Using defines for protocol characters

From
Alvaro Herrera
Date:
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)



Re: Using defines for protocol characters

From
Dave Cramer
Date:


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
Attachment

Re: Using defines for protocol characters

From
Peter Smith
Date:
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].

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

Kind Regards,
Peter Smith
Fujitsu Australia



Re: Using defines for protocol characters

From
Alvaro Herrera
Date:
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/



Re: Using defines for protocol characters

From
Dave Cramer
Date:


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 
--
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Re: Using defines for protocol characters

From
Robert Haas
Date:
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



Re: Using defines for protocol characters

From
Tom Lane
Date:
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



Re: Using defines for protocol characters

From
Nathan Bossart
Date:
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], I think we should also consider putting the
definitions in pqcomm.h.

[0] https://postgr.es/m/20230803185356.GA1144430%40nathanxps13

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



Re: Using defines for protocol characters

From
Robert Haas
Date:
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



Re: Using defines for protocol characters

From
Dave Cramer
Date:


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 

Re: Using defines for protocol characters

From
Tom Lane
Date:
Dave Cramer <davecramer@gmail.com> writes:
> On Mon, 7 Aug 2023 at 12:59, Robert Haas <robertmhaas@gmail.com> wrote:
>> 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 ?

I'm good with Robert's proposal above.

            regards, tom lane



Re: Using defines for protocol characters

From
Nathan Bossart
Date:
On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote:
> Dave Cramer <davecramer@gmail.com> writes:
>> On Mon, 7 Aug 2023 at 12:59, Robert Haas <robertmhaas@gmail.com> wrote:
>>> 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 ?
> 
> I'm good with Robert's proposal above.

+1

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



Re: Using defines for protocol characters

From
Peter Smith
Date:
+#ifndef _PROTOCOL_H
+#define _PROTOCOL_H
+
+#define PQMSG_REQ_BIND              'B'
+#define PQMSG_REQ_CLOSE             'C'
+#define PQMSG_REQ_DESCRIBE          'D'
+#define PQMSG_REQ_EXECUTE           'E'
+#define PQMSG_REQ_FUNCTION_CALL     'F'
+#define PQMSG_REQ_FLUSH_DATA        'H'
+#define PQMSG_REQ_BACKEND_KEY_DATA  'K'
+#define PQMSG_REQ_PARSE             'P'
+#define PQMSG_REQ_AUTHENTICATION    'R'
+#define PQMSG_REQ_SYNC_DATA         'S'
+#define PQMSG_REQ_SIMPLE_QUERY      'Q'
+#define PQMSG_REQ_TERMINATE         'X'
+#define PQMSG_REQ_COPY_FAIL         'f'
+#define PQMSG_REQ_COPY_DONE         'c'
+#define PQMSG_REQ_COPY_DATA         'd'
+#define PQMSG_REQ_COPY_PROGRESS     'p'
+#define PQMSG_REQ_PREPARED          'S'
+#define PQMSG_REQ_PORTAL            'P'
+
+
+/*
+Responses
+*/
+#define PQMSG_RESP_PARSE_COMPLETE   '1'
+#define PQMSG_RESP_BIND_COMPLETE    '2'
+#define PQMSG_RESP_CLOSE_COMPLETE   '3'
+#define PQMSG_RESP_NOTIFY           'A'
+#define PQMSG_RESP_COMMAND_COMPLETE 'C'
+#define PQMSG_RESP_DATA_ROW         'D'
+#define PQMSG_RESP_ERROR            'E'
+#define PQMSG_RESP_COPY_IN          'G'
+#define PQMSG_RESP_COPY_OUT         'H'
+#define PQMSG_RESP_EMPTY_QUERY      'I'
+#define PQMSG_RESP_NOTICE           'N'
+#define PQMSG_RESP_PARALLEL_PROGRESS 'P'
+#define PQMSG_RESP_FUNCTION_CALL    'V'
+#define PQMSG_RESP_PARAMETER_STATUS 'S'
+#define PQMSG_RESP_ROW_DESCRIPTION  'T'
+#define PQMSG_RESP_COPY_BOTH        'W'
+#define PQMSG_RESP_READY_FOR_QUERY  'Z'
+#define PQMSG_RESP_NO_DATA          'n'
+#define PQMSG_RESP_PASSWORD         'p'
+#define PQMSG_RESP_PORTAL_SUSPENDED 's'
+#define PQMSG_RESP_PARAMETER_DESCRIPTION 't'
+#define PQMSG_RESP_NEGOTIATE_PROTOCOL    'v'
+#endif

Was ordering-by-value intended here? If yes, then FYI both of those
groups of #defines are very nearly, but not quite, in that order.

------
Kind Regards,
Peter Smith.
Fujitsu Australia



Re: Using defines for protocol characters

From
Tatsuo Ishii
Date:
> On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote:
>> Dave Cramer <davecramer@gmail.com> writes:
>>> On Mon, 7 Aug 2023 at 12:59, Robert Haas <robertmhaas@gmail.com> wrote:
>>>> 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 ?
>> 
>> I'm good with Robert's proposal above.
> 
> +1

+1.

Also we need to decide what to do with them:

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

If we go "PqMsgEmptyQueryResponse", probably we should go something
like for these?

#define PqMsgReqPrepared 'S'
#define PqMsgReqPortal 'P'

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



Re: Using defines for protocol characters

From
Dave Cramer
Date:


On Mon, 7 Aug 2023 at 16:50, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
> On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote:
>> Dave Cramer <davecramer@gmail.com> writes:
>>> On Mon, 7 Aug 2023 at 12:59, Robert Haas <robertmhaas@gmail.com> wrote:
>>>> 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 ?
>>
>> I'm good with Robert's proposal above.
>
> +1

+1.

Also we need to decide what to do with them:

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

If we go "PqMsgEmptyQueryResponse", probably we should go something
like for these?

#define PqMsgReqPrepared 'S'
#define PqMsgReqPortal 'P'

I went with PqMsgPortalSubCommand and PqMsgPreparedSubCommand 

See attached patch

Dave
Attachment

Re: Using defines for protocol characters

From
Tatsuo Ishii
Date:
> On Mon, 7 Aug 2023 at 16:50, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
> 
>> > On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote:
>> >> Dave Cramer <davecramer@gmail.com> writes:
>> >>> On Mon, 7 Aug 2023 at 12:59, Robert Haas <robertmhaas@gmail.com>
>> wrote:
>> >>>> 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 ?
>> >>
>> >> I'm good with Robert's proposal above.
>> >
>> > +1
>>
>> +1.
>>
>> Also we need to decide what to do with them:
>>
>> > #define PQMSG_REQ_PREPARED 'S'
>> > #define PQMSG_REQ_PORTAL 'P'
>>
>> If we go "PqMsgEmptyQueryResponse", probably we should go something
>> like for these?
>>
>> #define PqMsgReqPrepared 'S'
>> #define PqMsgReqPortal 'P'
>>
> 
> I went with PqMsgPortalSubCommand and PqMsgPreparedSubCommand
> 
> See attached patch

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



Re: Using defines for protocol characters

From
Alvaro Herrera
Date:
On 2023-Aug-07, Nathan Bossart wrote:

> On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote:
> > Dave Cramer <davecramer@gmail.com> writes:

> >> Can we vote or whatever it takes to decide on a naming pattern that is
> >> acceptable ?
> > 
> > I'm good with Robert's proposal above.
> 
> +1

WFM as well.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Every machine is a smoke machine if you operate it wrong enough."
https://twitter.com/libseybieda/status/1541673325781196801



Re: Using defines for protocol characters

From
Peter Eisentraut
Date:
1. As was discussed, these definitions should go into 
src/include/libpq/pqcomm.h, not a new file.

2. I would prefer an underscore after PgMsg, like PqMsg_DescribeRequest, 
so it's easier to visually locate the start of the actual message name.

3. IMO, the names of the protocol messages in protocol.sgml are 
canonical.  Your patch appends "Request" and "Response" in cases where 
that is not part of the actual name.  Also, some messages are documented 
to go both ways, so this separation doesn't make sense strictly 
speaking.  Please use the names as in protocol.sgml without augmenting them.




Re: Using defines for protocol characters

From
Dave Cramer
Date:


On Wed, 9 Aug 2023 at 09:19, Peter Eisentraut <peter@eisentraut.org> wrote:
1. As was discussed, these definitions should go into
src/include/libpq/pqcomm.h, not a new file.

I'm ambivalent, this is very easy to do.  

2. I would prefer an underscore after PgMsg, like PqMsg_DescribeRequest,
so it's easier to visually locate the start of the actual message name.

3. IMO, the names of the protocol messages in protocol.sgml are
canonical.  Your patch appends "Request" and "Response" in cases where
that is not part of the actual name.  Also, some messages are documented
to go both ways, so this separation doesn't make sense strictly
speaking.  Please use the names as in protocol.sgml without augmenting them.


I've changed this a number of times. I do not mind changing it again, but can we reach a consensus ?

Dave 

Re: Using defines for protocol characters

From
Tom Lane
Date:
Dave Cramer <davecramer@gmail.com> writes:
> On Wed, 9 Aug 2023 at 09:19, Peter Eisentraut <peter@eisentraut.org> wrote:
>> 3. IMO, the names of the protocol messages in protocol.sgml are
>> canonical.  Your patch appends "Request" and "Response" in cases where
>> that is not part of the actual name.  Also, some messages are documented
>> to go both ways, so this separation doesn't make sense strictly
>> speaking.  Please use the names as in protocol.sgml without augmenting
>> them.

> I've changed this a number of times. I do not mind changing it again, but
> can we reach a consensus ?

I agree with Peter: let's use the names in the protocol document
with a single prefix.  I've got mixed feelings about whether that prefix
should have an underscore, though.

            regards, tom lane



Re: Using defines for protocol characters

From
Dave Cramer
Date:


On Wed, 9 Aug 2023 at 10:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Dave Cramer <davecramer@gmail.com> writes:
> On Wed, 9 Aug 2023 at 09:19, Peter Eisentraut <peter@eisentraut.org> wrote:
>> 3. IMO, the names of the protocol messages in protocol.sgml are
>> canonical.  Your patch appends "Request" and "Response" in cases where
>> that is not part of the actual name.  Also, some messages are documented
>> to go both ways, so this separation doesn't make sense strictly
>> speaking.  Please use the names as in protocol.sgml without augmenting
>> them.

> I've changed this a number of times. I do not mind changing it again, but
> can we reach a consensus ?

I agree with Peter: let's use the names in the protocol document
with a single prefix.  I've got mixed feelings about whether that prefix
should have an underscore, though.

Well, we're getting closer :)

Dave

Re: Using defines for protocol characters

From
Nathan Bossart
Date:
On Wed, Aug 09, 2023 at 10:44:42AM -0600, Dave Cramer wrote:
> On Wed, 9 Aug 2023 at 10:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I agree with Peter: let's use the names in the protocol document
>> with a single prefix.  I've got mixed feelings about whether that prefix
>> should have an underscore, though.
> 
> Well, we're getting closer :)

I'm +0.5 for the underscore.

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



Re: Using defines for protocol characters

From
Alvaro Herrera
Date:
On 2023-Aug-09, Nathan Bossart wrote:

> On Wed, Aug 09, 2023 at 10:44:42AM -0600, Dave Cramer wrote:
> > On Wed, 9 Aug 2023 at 10:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I agree with Peter: let's use the names in the protocol document
> >> with a single prefix.  I've got mixed feelings about whether that prefix
> >> should have an underscore, though.
> > 
> > Well, we're getting closer :)
> 
> I'm +0.5 for the underscore.

We use CamelCase_With_UnderScores in other places (PgStat_Counter,
AtEOXact_PgStat_Database).  It's not pretty, and at times it's not
comfortable to type, but I think it will be less ugly here than in those
other places (particularly because there'll be a single underscore), and
I also agree that readability is better with the underscore than
without.

So, I'm also +0.5 on having them, much as it displeases Robert.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/



Re: Using defines for protocol characters

From
Nathan Bossart
Date:
I tried to address all the feedback in v5 of the patch, which is attached.
I limited the patch to only the characters that have names in the "Message
Formats" section of protocol.sgml instead of trying to invent names for
unnamed characters.

I'm aware of one inconsistency.  While I grouped all the authentication
request messages ('R') into PqMsg_AuthenticationRequest, I added separate
macros for all the different meanings of 'p', i.e., GSSResponse,
PasswordMessage, SASLInitialResponse, and SASLResponse.  I wanted to list
everything in protocol.sgml (even the duplicateѕ) to ease greppability, but
the code is structure such that adding macros for all the different
authentication messages would be kind of pointless.  Plus, there is a
separate set of authentication request codes just below the added macros.
So, this is where I landed...

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

Attachment

Re: Using defines for protocol characters

From
Tatsuo Ishii
Date:
> I tried to address all the feedback in v5 of the patch, which is attached.
> I limited the patch to only the characters that have names in the "Message
> Formats" section of protocol.sgml instead of trying to invent names for
> unnamed characters.
>
> I'm aware of one inconsistency.  While I grouped all the authentication
> request messages ('R') into PqMsg_AuthenticationRequest, I added separate
> macros for all the different meanings of 'p', i.e., GSSResponse,
> PasswordMessage, SASLInitialResponse, and SASLResponse.  I wanted to list
> everything in protocol.sgml (even the duplicate ) to ease greppability, but
> the code is structure such that adding macros for all the different
> authentication messages would be kind of pointless.  Plus, there is a
> separate set of authentication request codes just below the added macros.
> So, this is where I landed...

Is it possible to put the new define staff into protocol.h then let
pqcomm.h include protocol.h? This makes Pgpool-II and other middle
ware/drivers written in C easier to use the defines so that they only
include protocol.h to use the defines.

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



Re: Using defines for protocol characters

From
Nathan Bossart
Date:
On Tue, Aug 15, 2023 at 02:46:24PM +0900, Tatsuo Ishii wrote:
> Is it possible to put the new define staff into protocol.h then let
> pqcomm.h include protocol.h? This makes Pgpool-II and other middle
> ware/drivers written in C easier to use the defines so that they only
> include protocol.h to use the defines.

It is possible, of course, but are there any reasons such programs couldn't
include pqcomm.h?  It looks relatively inexpensive to me.  That being said,
I'm fine with the approach you describe if the folks in this thread agree.

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



Re: Using defines for protocol characters

From
Tatsuo Ishii
Date:
> On Tue, Aug 15, 2023 at 02:46:24PM +0900, Tatsuo Ishii wrote:
>> Is it possible to put the new define staff into protocol.h then let
>> pqcomm.h include protocol.h? This makes Pgpool-II and other middle
>> ware/drivers written in C easier to use the defines so that they only
>> include protocol.h to use the defines.
> 
> It is possible, of course, but are there any reasons such programs couldn't
> include pqcomm.h?  It looks relatively inexpensive to me.  That being said,
> I'm fine with the approach you describe if the folks in this thread agree.

Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But
what about other middleware?

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



Re: Using defines for protocol characters

From
Michael Paquier
Date:
On Wed, Aug 16, 2023 at 06:25:09AM +0900, Tatsuo Ishii wrote:
> Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But
> what about other middleware?

Why do you need to include directly c.h?  There are definitions in
there that are not intended to be exposed.
--
Michael

Attachment

Re: Using defines for protocol characters

From
Alvaro Herrera
Date:
On 2023-Aug-16, Michael Paquier wrote:

> On Wed, Aug 16, 2023 at 06:25:09AM +0900, Tatsuo Ishii wrote:
> > Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But
> > what about other middleware?
> 
> Why do you need to include directly c.h?  There are definitions in
> there that are not intended to be exposed.

What this argument says is that these new defines should be in a
separate file, not in pqcomm.h.  IMO that makes sense, precisely because
these defines should be usable by third parties.

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



Re: Using defines for protocol characters

From
Tatsuo Ishii
Date:
> On Wed, Aug 16, 2023 at 06:25:09AM +0900, Tatsuo Ishii wrote:
>> Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But
>> what about other middleware?
> 
> Why do you need to include directly c.h?  There are definitions in
> there that are not intended to be exposed.

pqcomm.h has this:

#define UNIXSOCK_PATH(path, port, sockdir) \
       (AssertMacro(sockdir), \
        AssertMacro(*(sockdir) != '\0'), \
        snprintf(path, sizeof(path), "%s/.s.PGSQL.%d", \
                 (sockdir), (port)))

AssertMacro is defined in c.h.

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



Re: Using defines for protocol characters

From
Nathan Bossart
Date:
On Tue, Aug 15, 2023 at 11:40:07PM +0200, Alvaro Herrera wrote:
> On 2023-Aug-16, Michael Paquier wrote:
> 
>> On Wed, Aug 16, 2023 at 06:25:09AM +0900, Tatsuo Ishii wrote:
>> > Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But
>> > what about other middleware?
>> 
>> Why do you need to include directly c.h?  There are definitions in
>> there that are not intended to be exposed.
> 
> What this argument says is that these new defines should be in a
> separate file, not in pqcomm.h.  IMO that makes sense, precisely because
> these defines should be usable by third parties.

I moved the definitions out to a separate file in v6.

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

Attachment

Re: Using defines for protocol characters

From
Michael Paquier
Date:
On Wed, Aug 16, 2023 at 12:29:56PM -0700, Nathan Bossart wrote:
> I moved the definitions out to a separate file in v6.

Looks sensible seen from here.

This patch is missing the installation of protocol.h in
src/tools/msvc/Install.pm for MSVC.  For pqcomm.h, we are doing that:
lcopy('src/include/libpq/pqcomm.h', $target . '/include/internal/libpq/')
    || croak 'Could not copy pqcomm.h';

So adding two similar lines for protocol.h should be enough (I assume,
did not test).

In fe-exec.c, we still have a few things for the type of objects to
work on:
- 'S' for statement.
- 'P' for portal.
Should these be added to protocol.h?  They are part of the extended
protocol.

The comment at the top of PQsendTypedCommand() mentions 'C' and 'D',
but perhaps these should be updated to the object names instead?

pqFunctionCall3(), for PQfn(), has a few more hardcoded characters for
its status codes.  I'm OK to do things incrementally so it's fine by
me to not add them now, just noticing on the way what could be added
to this new header.
--
Michael

Attachment

Re: Using defines for protocol characters

From
Nathan Bossart
Date:
On Thu, Aug 17, 2023 at 09:31:55AM +0900, Michael Paquier wrote:
> Looks sensible seen from here.

Thanks for taking a look.

> This patch is missing the installation of protocol.h in
> src/tools/msvc/Install.pm for MSVC.  For pqcomm.h, we are doing that:
> lcopy('src/include/libpq/pqcomm.h', $target . '/include/internal/libpq/')
>     || croak 'Could not copy pqcomm.h';
> 
> So adding two similar lines for protocol.h should be enough (I assume,
> did not test).

I added those lines in v7.

> In fe-exec.c, we still have a few things for the type of objects to
> work on:
> - 'S' for statement.
> - 'P' for portal.
> Should these be added to protocol.h?  They are part of the extended
> protocol.

IMHO they should be added, but I've intentionally restricted this first
patch to only codes with existing names in protocol.sgml.  I figured we
could work on naming other things in a follow-up discussion.

> The comment at the top of PQsendTypedCommand() mentions 'C' and 'D',
> but perhaps these should be updated to the object names instead?

Done.

> pqFunctionCall3(), for PQfn(), has a few more hardcoded characters for
> its status codes.  I'm OK to do things incrementally so it's fine by
> me to not add them now, just noticing on the way what could be added
> to this new header.

Cool, thanks.

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

Attachment

Re: Using defines for protocol characters

From
Robert Haas
Date:
On Thu, Aug 17, 2023 at 12:13 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
> On Thu, Aug 17, 2023 at 09:31:55AM +0900, Michael Paquier wrote:
> > Looks sensible seen from here.
>
> Thanks for taking a look.

I think this is going to be a major improvement in terms of readability!

I'm pretty happy with this version, too. We may be running out of
things to argue about -- and no, I don't want to argue about
underscores more!

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



Re: Using defines for protocol characters

From
Dave Cramer
Date:


On Thu, Aug 17, 2023 at 9:22 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Aug 17, 2023 at 12:13 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
> On Thu, Aug 17, 2023 at 09:31:55AM +0900, Michael Paquier wrote:
> > Looks sensible seen from here.
>
> Thanks for taking a look.

I think this is going to be a major improvement in terms of readability!

That was the primary goal 

Dave 

--
Dave Cramer

Re: Using defines for protocol characters

From
Nathan Bossart
Date:
On Thu, Aug 17, 2023 at 12:22:24PM -0400, Robert Haas wrote:
> I think this is going to be a major improvement in terms of readability!
> 
> I'm pretty happy with this version, too. We may be running out of
> things to argue about -- and no, I don't want to argue about
> underscores more!

Awesome.  If we are indeed out of things to argue about, I'll plan on
committing this sometime next week.

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



Re: Using defines for protocol characters

From
Nathan Bossart
Date:
On Thu, Aug 17, 2023 at 09:52:03AM -0700, Nathan Bossart wrote:
> On Thu, Aug 17, 2023 at 12:22:24PM -0400, Robert Haas wrote:
>> I think this is going to be a major improvement in terms of readability!
>> 
>> I'm pretty happy with this version, too. We may be running out of
>> things to argue about -- and no, I don't want to argue about
>> underscores more!
> 
> Awesome.  If we are indeed out of things to argue about, I'll plan on
> committing this sometime next week.

Committed.

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