From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Nathan Bossart <nathandbossart(at)gmail(dot)com> |
Cc: | Aleksander Alekseev <aleksander(at)timescale(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: [PATCH] Refactor pqformat.{c,h} and protocol.h |
Date: | 2024-07-16 16:48:40 |
Message-ID: | 3356418.1721148520@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Nathan Bossart <nathandbossart(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | David G. Johnston | 2024-07-16 17:11:29 | Re: Things I don't like about \du's "Attributes" column |
Previous Message | Nathan Bossart | 2024-07-16 16:30:17 | Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal |