Re: tablecmds.c/MergeAttributes() cleanup

On 05.10.23 17:49, Peter Eisentraut wrote:
> On 19.09.23 15:11, Peter Eisentraut wrote:
>> Here is an updated version of this patch set.  I resolved some
>> conflicts and addressed this comment of yours.  I also dropped the one
>> patch with some catalog header edits that people didn't seem to
>> particularly like.
>>
>> The patches that are now 0001 through 0004 were previously reviewed
>> and just held for the not-null constraint patches, I think, so I'll
>> commit them in a few days if there are no objections.
>>
>> Patches 0005 through 0007 are also ready in my opinion, but they
>> haven't really been reviewed, so this would be something for reviewers
>> to focus on.  (0005 and 0007 are trivial, but they go to together with
>> 0006.)
>>
>> The remaining 0008 and 0009 were still under discussion and
>> contemplation.
>
> I have committed through 0007, and I'll now close this patch set as
> "Committed", and I will (probably) bring back the rest (especially 0008)
> as part of a different patch set soon.

After playing with this for, well, 2 months, and considering various
other approaches, I would like to bring back the remaining two patches
in unchanged form.

Especially the (now) first patch "Refactor ATExecAddColumn() to use
BuildDescForRelation()" would be very helpful for facilitating further
refactoring in this area, because it avoids having two essentially
duplicate pieces of code responsible for converting a ColumnDef node
into internal form.

One of your (Álvaro's) comments about this earlier was

> 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.

which was also my thought at the beginning. However, this wouldn't
quite work that way, for several reasons. For instance,
BuildDescForRelation() also needs to keep track of the has_not_null
property across all fields, and so if you split that function up, you
would have to somehow make that an output argument and have the caller
keep track of it. Also, the output of BuildDescForRelation() in
ATExecAddColumn() is passed into InsertPgAttributeTuples(), which
requires a tuple descriptor anyway, so splitting this up into a
per-attribute function would then require ATExecAddColumn() to
re-assemble a tuple descriptor anyway, so this wouldn't save anything.
Also note that ATExecAddColumn() could in theory be enhanced to add more
than one column in one go, so having this code structure in place isn't
inconsistent with that.

The main hackish thing, I suppose, is that we have to fix up the
attribute number after returning from BuildDescForRelation(). I suppose
we could address that by passing in a starting attribute number (or
alternatively maximum existing attribute number) into
BuildDescForRelation(). I think that would be okay; it would probably
be about a wash in terms of code added versus saved.

The (now) second patch is also still of interest to me, but I have since
noticed that I think [0] should be fixed first, but to make that fix
simpler, I would like the first patch from here.

[0]:
https://www.postgresql.org/message-id/flat/24656cec-d6ef-4d15-8b5b-e8dfc9c833a7%40eisentraut.org