On Mon, 20 Oct 2025 at 21:15, Michael Paquier <michael@paquier.xyz> 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
