Обсуждение: Use CompactAttribute more often, when possible

5983a4cff added CompactAttribute to TupleDesc to allow commonly
accessed fields from the FormData_pg_attribute array to be accessed
more quickly by having to load fewer cachelines from memory. That
commit also went around and changed many locations to use
CompactAttribute, but not all locations.

The attached aims to adjust the remaining locations that I'm
comfortable changing. I've avoided doing anything in tablecmds.c as
that's where TupleDescs tend to get changed and I think there's a bit
of undue risk using CompactAttribute there, just in case it gets
accessed after the TupleDesc is changed and before flushing the
pending changes to CompactAttribute in populate_compact_attribute().

Also, I'm aware that the code around the population of the
CompactAttribute has changed a bit since it was added. attgenerated no
longer mirrors its equivalent pg_attribute column. If that was made to
mirror that column again, then there are a few extra locations that
could be made to use CompactAttribute. There's also some more
complexity around CompactAttribute.attnotnull that's crept in. I think
I roughly understand the need for that, but the intent of
CompactAttribute mirroring commonly used pg_attribute fields is made
no longer true by those changes as it now contains extra information
that's unavailable in pg_attribute. It'd be nice if pg_attribute also
had the various additional states of attnotnull that CompactAttribute
now has. </rant>

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.

David

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.

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.

@@ -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?
--
Michael

> On Oct 20, 2025, at 12:46, David Rowley <dgrowleyml@gmail.com> wrote:
>
> 5983a4cff added CompactAttribute to TupleDesc to allow commonly
> accessed fields from the FormData_pg_attribute array to be accessed
> more quickly by having to load fewer cachelines from memory. That
> commit also went around and changed many locations to use
> CompactAttribute, but not all locations.
>
> The attached aims to adjust the remaining locations that I'm
> comfortable changing. I've avoided doing anything in tablecmds.c as
> that's where TupleDescs tend to get changed and I think there's a bit
> of undue risk using CompactAttribute there, just in case it gets
> accessed after the TupleDesc is changed and before flushing the
> pending changes to CompactAttribute in populate_compact_attribute().
>
> Also, I'm aware that the code around the population of the
> CompactAttribute has changed a bit since it was added. attgenerated no
> longer mirrors its equivalent pg_attribute column. If that was made to
> mirror that column again, then there are a few extra locations that
> could be made to use CompactAttribute. There's also some more
> complexity around CompactAttribute.attnotnull that's crept in. I think
> I roughly understand the need for that, but the intent of
> CompactAttribute mirroring commonly used pg_attribute fields is made
> no longer true by those changes as it now contains extra information
> that's unavailable in pg_attribute. It'd be nice if pg_attribute also
> had the various additional states of attnotnull that CompactAttribute
> now has. </rant>
>
> 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.
>
> David
> <v1-0001-Use-CompactAttribute-more-often-when-possible.patch>

It’s good to learn. I wasn’t aware of CompactAttribute before.

I search for “TupleDescAttr”, and found more occurrences to replace. See the attached diff file.

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

On Mon, 20 Oct 2025 at 21:34, Chao Li <li.evan.chao@gmail.com> wrote:
> I search for “TupleDescAttr”, and found more occurrences to replace. See the attached diff file.

Thanks for looking.  There was a bunch that I didn't do and tried to
convey that in the commit message.

The diff you sent seems to contain a mix of my ones and your
additional ones. If you can sort that out so it's just yours and
subtract the following, which I've already decided not to do.

* MergeConstraintsIntoExisting -- I don't want to touch these for ALTER TABLE.
* record_recv -- Not done. First loop could be done but 2nd loop needs
atttypid, so Form_pg_attribute needs accessed regardless
* record_send -- Not done. First loop could be done but 2nd loop needs
atttypid, so Form_pg_attribute needs accessed regardless
* ATExecAddColumn -- I don't want to touch these for ALTER TABLE.
* CatalogTupleCheckConstraints -- Not done. This is just for Asserts
* ATRewriteTable -- I don't want to touch these for ALTER TABLE.
* ATAddForeignKeyConstraint -- I don't want to touch these for ALTER TABLE.
* set_attnotnull -- I don't want to touch these for ALTER TABLE.
* FreeTupleDesc -- Not done. I'd rather not use CompactAttribute here
just in case someone forgets to flush changes before freeing the
TupleDesc
* get_sql_insert -- Not done.  There are two loops, only 1 can use
CompactAttribute.

Thanks

David

On 2025-Oct-20, David Rowley wrote:

> There's also some more
> complexity around CompactAttribute.attnotnull that's crept in. I think
> I roughly understand the need for that, but the intent of
> CompactAttribute mirroring commonly used pg_attribute fields is made
> no longer true by those changes as it now contains extra information
> that's unavailable in pg_attribute. It'd be nice if pg_attribute also
> had the various additional states of attnotnull that CompactAttribute
> now has. </rant>

Making CompactAttribute not match attnotnull exactly was a quite
deliberate decision, and thoroughly explained in the thread that led to
that development.  Feel free to propose changing attnotnull to match
what we have in CompactAttribute, but 1. I don't think it serves any
purpose, 2. it'd probably be complicated(*) and 3. it might be quite an
uphill battle, as the breakage you'd cause to external software looking
at attnotnull is going to be enormous.

(*) Actually it might not be all that complicated, as I think the
patches submitted by Rushabh at some point implemented the various
states directly in pg_attribute, mirroring what appears in
pg_constraint.

It's possible, of course, to argue that pg_attribute.attnotnull is just
an implementation detail and that client applications shouldn't be
looking at that or that they should be prepared to handle breakage.  We
already had that discussion and I think the prevailing conclusion was
that it's a little bit too entrenched in too many ancient apps and
frameworks, and for too long a time for that to work.  I didn't want
having to revert my not-null constraints patch because of it.

Anyway it's always been the case that certain features are subverted/
modified/evolved in ways that the original author didn't envision.
That's not a bug of the development process, I think, but rather one of
its best features.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"The saddest aspect of life right now is that science gathers knowledge faster
 than society gathers wisdom."  (Isaac Asimov)

> On Oct 20, 2025, at 16:49, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Mon, 20 Oct 2025 at 21:34, Chao Li <li.evan.chao@gmail.com> wrote:
>> I search for “TupleDescAttr”, and found more occurrences to replace. See the attached diff file.
>
> Thanks for looking.  There was a bunch that I didn't do and tried to
> convey that in the commit message.
>
> The diff you sent seems to contain a mix of my ones and your
> additional ones. If you can sort that out so it's just yours and
> subtract the following, which I've already decided not to do.
>

Sorry for the stupid mistake. I initially applied your patch to a dirty branch, and I found a new occurrence to
replace.Then I switched to a new branch, but forgot to “git am” your patch, that’s why my diff included your changes. 

I have removed those ones that you don’t want in v2 diff.

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Mon, 20 Oct 2025 at 22:33, Chao Li <li.evan.chao@gmail.com> wrote:
> I have removed those ones that you don’t want in v2 diff.

Looks like it's just the rowtypes.c ones that you have that I didn't
list. get_sql_insert() and FreeTupleDesc() were in my reject list.

The extra ones you've found seem to match a similar pattern to other
ones I did reject. The reason I left out the ones that could access
the CompactAttribute in one part of the function, but also *need* to
access the Form_pg_attribute in another part, was that I wasn't
certain it was going to be a win. One way it could be a net loss is
that now you need to load cachelines for both structs. It looks like
your changes to record_cmp(), record_eq() and record_image_eq() might
be more optimal in cases where the record has many dropped columns, as
that would allow us to skip fetching the Form_pg_attribute memory for
dropped columns. IMO, it doesn't seem like a good thing to optimise
for in exchange for additional overhead when there are no or few
dropped columns.

David

On Mon, 20 Oct 2025 at 21:43, David Rowley <dgrowleyml@gmail.com> wrote:
>
> 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:
> > @@ -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?

> We don't put system attributes in TupleDescs so I put that check down
> to misguided programming.

Thank you for reviewing this. Now pushed.

David