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.