Re: tablecmds.c/MergeAttributes() cleanup

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.