Re: Use CompactAttribute more often, when possible

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