On Mon, 20 Oct 2025 at 21:15, Michael Paquier <[email protected]> wrote: > > On Mon, Oct 20, 2025 at 05:46:33PM +1300, David Rowley wrote: > > I don't think the attached is very interesting to look at, but l'll > > leave it here for a bit just in case anyone wants to look. Otherwise, > > I plan to just treat it as a follow-up of 5983a4cff. > > Still I've looked at it. I like reading code.
Thanks!, and good! :) > The change in validateDomainCheckConstraint()@typecmds.c looks > independent to what you are suggesting here. Grouping that is fine, > just noting that the intent is not the same. Yeah, maybe shouldn't have included that with this patch. It felt minor enough to include it. I should likely mention it in the commit message. Otherwise, happy to do it separately, I just thought it was a bit too trivial. > @@ -5146,10 +5146,6 @@ ReorderBufferToastReplace(ReorderBuffer *rb, > ReorderBufferTXN *txn, > dlist_iter it; > Size data_done = 0; > > - /* system columns aren't toasted */ > - if (attr->attnum < 0) > - continue; > > Er, why this removal? It's a good question. I only remembered to write about that after I hit send on the email... We don't put system attributes in TupleDescs so I put that check down to misguided programming. TupleDesc's header comment says: * Note that only user attributes, not system attributes, are mentioned in * TupleDesc. plus there are so many places where we assume that TupleDesc[Compact]Attr(..., attnum - 1) returns the attribute details for attnum, so if the 0th element of the compact_attrs[] and subsequent attrs[] array wasn't attnum==1, we'd be in big trouble. David
