Horton

Re: tablecmds.c/MergeAttributes() cleanup

Peter Eisentraut <peter@eisentraut.org>

View thread View on postgr.es
From: Peter Eisentraut <peter@eisentraut.org>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2023-12-06T08:23:07Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Fix segmentation fault in MergeInheritedAttribute()

  2. Improve compression and storage support with inheritance

  3. Split some code out from MergeAttributes()

  4. MergeAttributes code deduplication

  5. Add a const decoration

  6. MergeAttributes: convert pg_attribute back to ColumnDef before comparing

  7. Refactor ATExecAddColumn() to use BuildDescForRelation()

  8. Move BuildDescForRelation() from tupdesc.c to tablecmds.c

  9. Push attcompression and attstorage handling into BuildDescForRelation()

  10. Push attidentity and attgenerated handling into BuildDescForRelation()

  11. Add TupleDescGetDefault()

  12. MergeAttributes() and related variable renaming

  13. Clean up MergeCheckConstraint()

  14. Clean up MergeAttributesIntoExisting()

  15. Remove useless if condition

  16. Make more use of makeColumnDef()

  17. Add some const decorations

  18. Remove ancient special case code for dropping oid columns

  19. Remove ancient special case code for adding oid columns

  20. Remove obsolete comment about OID support