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-07-12T14:29:26Z
Lists: pgsql-hackers 
On 11.07.23 20:17, Alvaro Herrera wrote:
> 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.

I have committed these first three.  I'll leave it at that for now.

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

While working through the storage and compression handling, which look 
similar, I was momentarily puzzled by this.  While attcompression can be 
0 to mean, use default, this is not possible/allowed for attstorage, but 
it took looking around three corners to verify this.  It could be more 
explicit, I thought.

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