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.
