Symbolic names for the values of typalign and typstorage
While looking at Tomas' ALTER TYPE patch, I got annoyed by the fact
that all of the backend writes constants of type alignment and type
storage values as literal characters, such as 'i' and 'x'. This is
not our style for most other "poor man's enum" catalog columns, and
it makes it really hard to grep for relevant code. Hence, attached
is a proposed patch to invent #define names for those values.
As is our custom for other similar catalog columns, I only used the
macros in C code. There are some references in SQL code too,
particularly in the regression tests, but the difficulty of replacing
symbolic references in SQL code seems more than it's worth to fix.
One thing that I'm not totally happy about, as this stands, is that
we have to #include "catalog/pg_type.h" in various places we did
not need to before (although only a fraction of the files I touched
need that). Part of the issue is that I used the TYPALIGN_XXX
macros in tupmacs.h, but did not #include pg_type.h there because
I was concerned about macro inclusion bloat. Plausible alternatives
to the way I did it here include
* just bite the bullet and #include pg_type.h in tupmacs.h;
* keep using the hard-coded values in tupmacs.h (with a comment
as to why);
* put the TYPALIGN_XXX #defines somewhere else (not clear where,
but there might be a case for postgres.h, since so much of the
backend has some interest in alignment).
Thoughts? Anybody want to say that this is more code churn
than it's worth?
regards, tom lane
Attachments:
macros-for-alignment-and-storage-constants-1.patchtext/x-diff; charset=us-ascii; name=macros-for-alignment-and-storage-constants-1.patchDownload+339-307
On 2020-Mar-02, Tom Lane wrote:
While looking at Tomas' ALTER TYPE patch, I got annoyed by the fact
that all of the backend writes constants of type alignment and type
storage values as literal characters, such as 'i' and 'x'. This is
not our style for most other "poor man's enum" catalog columns, and
it makes it really hard to grep for relevant code. Hence, attached
is a proposed patch to invent #define names for those values.
Makes sense.
As is our custom for other similar catalog columns, I only used the
macros in C code. There are some references in SQL code too,
particularly in the regression tests, but the difficulty of replacing
symbolic references in SQL code seems more than it's worth to fix.
Agreed.
One thing that I'm not totally happy about, as this stands, is that
we have to #include "catalog/pg_type.h" in various places we did
not need to before (although only a fraction of the files I touched
need that). Part of the issue is that I used the TYPALIGN_XXX
macros in tupmacs.h, but did not #include pg_type.h there because
I was concerned about macro inclusion bloat. Plausible alternatives
to the way I did it here include* just bite the bullet and #include pg_type.h in tupmacs.h;
I like this one the most -- better than the alternative in the patch --
because it's the most honest IMO, except that there seems to be
altogether too much cruft in pg_type.h that should be elsewhere
(particularly nodes/nodes.h, which includes a large number of other
headers).
If we think that pg_type.h is the header to handle access to the pg_type
catalog, then I would think that the function declarations at the bottom
should be in some "internal" header file; then we can get rid of most
the #includes in pg_type.h.
Thoughts? Anybody want to say that this is more code churn
than it's worth?
It seems worthy cleanup to me.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2020-Mar-02, Tom Lane wrote:
One thing that I'm not totally happy about, as this stands, is that
we have to #include "catalog/pg_type.h" in various places we did
not need to before (although only a fraction of the files I touched
need that).
If we think that pg_type.h is the header to handle access to the pg_type
catalog, then I would think that the function declarations at the bottom
should be in some "internal" header file; then we can get rid of most
the #includes in pg_type.h.
Well, aside from indirect inclusions, pg_type.h also brings in a bunch
of type OID macros, which I feel we don't want to broadcast everywhere.
One argument in favor of sticking these new macros somewhere "more
central" is that they apply to both pg_type and pg_attribute (that
is, attalign and attstorage also use them). That's not a strong
argument, maybe, but it's something.
regards, tom lane
I wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2020-Mar-02, Tom Lane wrote:
One thing that I'm not totally happy about, as this stands, is that
we have to #include "catalog/pg_type.h" in various places we did
not need to before (although only a fraction of the files I touched
need that).
If we think that pg_type.h is the header to handle access to the pg_type
catalog, then I would think that the function declarations at the bottom
should be in some "internal" header file; then we can get rid of most
the #includes in pg_type.h.
Well, aside from indirect inclusions, pg_type.h also brings in a bunch
of type OID macros, which I feel we don't want to broadcast everywhere.
I realized that a possible compromise position is to have tupmacs.h
include pg_type_d.h, not the whole pg_type.h header, thus dodging the
indirect inclusions. That still brings in the type-OID macros, but
it's a lot less header scope creep than I was first fearing.
regards, tom lane
On 2020-Mar-03, Tom Lane wrote:
I realized that a possible compromise position is to have tupmacs.h
include pg_type_d.h, not the whole pg_type.h header, thus dodging the
indirect inclusions. That still brings in the type-OID macros, but
it's a lot less header scope creep than I was first fearing.
WFM.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2020-Mar-03, Tom Lane wrote:
I realized that a possible compromise position is to have tupmacs.h
include pg_type_d.h, not the whole pg_type.h header, thus dodging the
indirect inclusions. That still brings in the type-OID macros, but
it's a lot less header scope creep than I was first fearing.
WFM.
OK, I'll look harder at doing it that way.
regards, tom lane
I wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2020-Mar-03, Tom Lane wrote:
I realized that a possible compromise position is to have tupmacs.h
include pg_type_d.h, not the whole pg_type.h header, thus dodging the
indirect inclusions. That still brings in the type-OID macros, but
it's a lot less header scope creep than I was first fearing.
WFM.
OK, I'll look harder at doing it that way.
Yeah, that works out very nicely: there's now only one place besides
tupmacs.h that needs a new #include.
I did a little more polishing, and consider the attached committable,
unless anyone has objections.
regards, tom lane
Attachments:
macros-for-alignment-and-storage-constants-2.patchtext/x-diff; charset=us-ascii; name=macros-for-alignment-and-storage-constants-2.patchDownload+341-309
On Tue, Mar 03, 2020 at 04:45:51PM -0500, Tom Lane wrote:
Yeah, that works out very nicely: there's now only one place besides
tupmacs.h that needs a new #include.I did a little more polishing, and consider the attached committable,
unless anyone has objections.
Nice. I have looked at the patch and it seems to me that no spots
have been missed.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Tue, Mar 03, 2020 at 04:45:51PM -0500, Tom Lane wrote:
Yeah, that works out very nicely: there's now only one place besides
tupmacs.h that needs a new #include.
I did a little more polishing, and consider the attached committable,
unless anyone has objections.
Nice. I have looked at the patch and it seems to me that no spots
have been missed.
Pushed, thanks for reviewing.
regards, tom lane
Hi,
On 2020-03-02 17:52:17 -0500, Tom Lane wrote:
While looking at Tomas' ALTER TYPE patch, I got annoyed by the fact
that all of the backend writes constants of type alignment and type
storage values as literal characters, such as 'i' and 'x'. This is
not our style for most other "poor man's enum" catalog columns, and
it makes it really hard to grep for relevant code. Hence, attached
is a proposed patch to invent #define names for those values.
Independent of the patch, why aren't we using proper enums for some of
these? There's plenty code that tries to handle all variants for various
such "poor man's enum"s - the current compiler doesn't allow the
compiler to help defend against forgotten values. And I think there's
plenty cases where we *did* forget updating places for new values,
e.g. around the partitioned table reltype.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2020-03-02 17:52:17 -0500, Tom Lane wrote:
While looking at Tomas' ALTER TYPE patch, I got annoyed by the fact
that all of the backend writes constants of type alignment and type
storage values as literal characters, such as 'i' and 'x'. This is
not our style for most other "poor man's enum" catalog columns, and
it makes it really hard to grep for relevant code. Hence, attached
is a proposed patch to invent #define names for those values.
Independent of the patch, why aren't we using proper enums for some of
these?
I did think about that, but since the underlying storage needs to be
a "char", I'm not sure that using an enum for the values would really
be all that helpful. We might get warnings from pickier compilers,
and we wouldn't necessarily get the warnings we actually want.
regards, tom lane