Feedback-ID: ia2694551:Fastmail
Date: Fri, 4 Aug 2023 11:44:53 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Dave Cramer <davecramer@gmail.com>
Cc: Tatsuo Ishii <ishii@sraoss.co.jp>, pgsql-hackers@lists.postgresql.org,
	nathandbossart@gmail.com
Subject: Re: Using defines for protocol characters
Message-ID: <20230804094453.kobqc52uxq72cqyc@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CADK3HH+-=08kd-aNCOP9s7fCZHUxrUBvVx5cyBJZzHD49wzPQA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230804094453.kobqc52uxq72cqyc%40alvherre.pgsql>
Precedence: bulk

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)