Horton

Thread

Commits

  1. Introduce macros for typalign and typstorage constants.

  1. Symbolic names for the values of typalign and typstorage

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-02T22:52:17Z

    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

  2. Re: Symbolic names for the values of typalign and typstorage

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-03T01:31:07Z

    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

  3. Re: Symbolic names for the values of typalign and typstorage

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-03T03:22:04Z

    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

  4. Re: Symbolic names for the values of typalign and typstorage

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-03T15:11:28Z

    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

  5. Re: Symbolic names for the values of typalign and typstorage

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-03-03T18:29:26Z

    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

  6. Re: Symbolic names for the values of typalign and typstorage

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-03T18:35:19Z

    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

  7. Re: Symbolic names for the values of typalign and typstorage

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-03T21:45:51Z

    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

  8. Re: Symbolic names for the values of typalign and typstorage

    Michael Paquier <michael@paquier.xyz> — 2020-03-04T05:25:25Z

    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

  9. Re: Symbolic names for the values of typalign and typstorage

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-04T15:35:16Z

    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

  10. Re: Symbolic names for the values of typalign and typstorage

    Andres Freund <andres@anarazel.de> — 2020-03-06T18:18:26Z

    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

  11. Re: Symbolic names for the values of typalign and typstorage

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-06T19:10:17Z

    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