Re: [PATCH] Refactor pqformat.{c,h} and protocol.h - Mailing list pgsql-hackers

From Tom Lane
Subject Re: [PATCH] Refactor pqformat.{c,h} and protocol.h
Date
Msg-id 3356418.1721148520@sss.pgh.pa.us
Whole thread Raw
In response to Re: [PATCH] Refactor pqformat.{c,h} and protocol.h  (Nathan Bossart <nathandbossart@gmail.com>)
Responses Re: [PATCH] Refactor pqformat.{c,h} and protocol.h
List pgsql-hackers
Nathan Bossart <nathandbossart@gmail.com> writes:
> This was briefly brought up in the discussion that ultimately led to
> protocol.h [0].  I don't have a particularly strong opinion on the matter,
> but I will note that protocol.h was intended to be easily usable in
> third-party code, and changing it from characters to an enum from v17 to
> v18 might cause some extra code churn.

We could avoid that issue by back-patching into 17; I don't think
it's quite too late for that, and the header is new as of 17.

However, I'm generally -1 on the proposal independently of that,
because I think it is the wrong thing from an ABI standpoint.
The message type codes in the protocol are chars, full stop.
If we declare the data type as an enum, we have basically zero
control over how wide the compiler will choose to make that ---
although it's pretty likely that it *won't* choose char width.
So you could not represent the message layout accurately with
an enum.  Perhaps nobody is doing that, but it seems to me like
a foot-gun of roughly the same caliber as not using an enum.
Also, going in this direction would require adding casts between
char and enum in assorted places, which might be error-prone or
warning-inducing.

So on the whole, "leave it alone" seems like the right answer.

(This applies only to the s/char/enum/ proposal; I've not read
the patchset further than that.)

            regards, tom lane



pgsql-hackers by date:

Previous
From: Nathan Bossart
Date:
Subject: Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
Next
From: "David G. Johnston"
Date:
Subject: Re: Things I don't like about \du's "Attributes" column