Horton

Re: tablecmds.c/MergeAttributes() cleanup

Alvaro Herrera <alvherre@alvh.no-ip.org>

View thread View on postgr.es
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Peter Eisentraut <peter@eisentraut.org>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2024-01-11T12:41:49Z
Lists: pgsql-hackers 
On 2023-Dec-06, Peter Eisentraut wrote:

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

Hmm.  Well, if this state of affairs is useful to you, then I withdraw
my objection, because with this patch we're not really adding any new
weirdness, just moving around already-existing weirdness.  So let's
press ahead with 0001.  (I didn't look at 0002 this time, since
apparently you'd like to process the other patch first and then come
back here.)


If you look closely at InsertPgAttributeTuples and accompanying code, it
all looks a bit archaic.  They seem to be treating TupleDesc as a
glorified array of Form_pg_attribute elements in a convenient packaging.
It's probably cleaner to change these APIs so that they deal with a
Form_pg_attribute array, and not TupleDesc anymore.  But we can hack on
that some other day.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Pensar que el espectro que vemos es ilusorio no lo despoja de espanto,
sólo le suma el nuevo terror de la locura" (Perelandra, C.S. Lewis)

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