Thread: Using defines for protocol characters
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
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
--
Á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, 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
Dave
> 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
Attachment
> 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
Attachment
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
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
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
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], 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
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
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
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
+#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
> 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
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
> 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
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
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.
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
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
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
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
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/
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
> 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
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
> 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
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
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/
> 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
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
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
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
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
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
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
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