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
