Re: tablecmds.c/MergeAttributes() cleanup

On 2023-Jun-28, Peter Eisentraut wrote:

> The MergeAttributes() and related code in and around tablecmds.c is huge and
> ancient, with many things bolted on over time, and difficult to deal with.
> I took some time to make careful incremental updates and refactorings to
> make the code easier to follow, more compact, and more modern in appearance.
> I also found several pieces of obsolete code along the way.  This resulted
> in the attached long patch series.  Each patch tries to make a single change
> and can be considered incrementally.  At the end, the code is shorter,
> better factored, and I hope easier to understand.  There shouldn't be any
> change in behavior.

I spent a few minutes doing a test merge of this to my branch with NOT
NULL changes.  Here's a quick review.

> Subject: [PATCH 01/17] Remove obsolete comment about OID support

Obvious, trivial.  +1

> Subject: [PATCH 02/17] Remove ancient special case code for adding oid columns

LGTM; deletes dead code.

> Subject: [PATCH 03/17] Remove ancient special case code for dropping oid
>  columns

Hmm, interesting.  Yay for more dead code removal.  Didn't verify it.

> Subject: [PATCH 04/17] Make more use of makeColumnDef()

Good idea, +1.  Some lines (almost all makeColumnDef callsites) end up
too long.  This is the first patch that actually conflicts with the NOT
NULLs one, and the conflicts are easy to solve, so I won't complain if
you want to get it pushed soon.

> Subject: [PATCH 05/17] Clean up MergeAttributesIntoExisting()

I don't disagree with this in principle, but this one has more
conflicts than the previous ones.


> Subject: [PATCH 06/17] Clean up MergeCheckConstraint()

Looks a reasonable change as far as this patch goes.

However, reading it I noticed that CookedConstraint->inhcount is int
and is tested for wraparound, but pg_constraint.coninhcount is int16.
This is probably bogus already.  ColumnDef->inhcount is also int.  These
should be narrowed to match the catalog definitions.


> Subject: [PATCH 07/17] MergeAttributes() and related variable renaming

I think this makes sense, but there's a bunch of conflicts to NOT NULLs.
I guess we can come back to this one later.

> Subject: [PATCH 08/17] Improve some catalog documentation
> 
> Point out that typstorage and attstorage are never '\0', even for
> fixed-length types.  This is different from attcompression.  For this
> reason, some of the handling of these columns in tablecmds.c etc. is
> different.  (catalogs.sgml already contained this information in an
> indirect way.)

I don't understand why we must point out that they're never '\0'.  I
mean, if we're doing that, why not say that they can never be \xFF?
The valid values are already listed.  The other parts of this patch look
okay.

> Subject: [PATCH 09/17] Remove useless if condition
> 
> This is useless because these fields are not set anywhere before, so
> we can assign them unconditionally.  This also makes this more
> consistent with ATExecAddColumn().

Makes sense.

> Subject: [PATCH 10/17] Remove useless if condition
> 
> We can call GetAttributeCompression() with a NULL argument.  It
> handles that internally already.  This change makes all the callers of
> GetAttributeCompression() uniform.

I agree, +1.

> From 2eda6bc9897d0995a5112e2851c51daf0c35656e Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter@eisentraut.org>
> Date: Wed, 14 Jun 2023 17:51:31 +0200
> Subject: [PATCH 11/17] Refactor ATExecAddColumn() to use
>  BuildDescForRelation()
> 
> BuildDescForRelation() has all the knowledge for converting a
> ColumnDef into pg_attribute/tuple descriptor.  ATExecAddColumn() can
> make use of that, instead of duplicating all that logic.  We just pass
> a one-element list of ColumnDef and we get back exactly the data
> structure we need.  Note that we don't even need to touch
> BuildDescForRelation() to make this work.

Hmm, crazy.  I'm not sure I like this, because it seems much too clever.
The number of lines that are deleted is alluring, though.

Maybe it'd be better to create a separate routine that takes a single
ColumnDef and returns the Form_pg_attribute element for it; then use
that in both BuildDescForRelation and ATExecAddColumn.

> Subject: [PATCH 12/17] Push attidentity and attgenerated handling into
>  BuildDescForRelation()
> 
> Previously, this was handled by the callers separately, but it can be
> trivially moved into BuildDescForRelation() so that it is handled in a
> central place.

Looks reasonable.



I think the last few patches are the more innovative (interesting,
useful) of the bunch.  Let's get the first few out of the way.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/