Expanding HOT updates for expression and partial indexes

Attached find a patch that expands the cases where heap-only tuple (HOT) updates are possible without changing the basic semantics of HOT. This is accomplished by examining expression indexes for changes to determine if indexes require updating or not. A similar approach is taken for partial indexes, the predicate is evaluated and, in some cases, HOT updates are allowed. Even with this patch if any index is changed, all indexes are updated. Only in cases where none are modified will this patch allow the HOT path. Previously, an expression index on a modified column would disqualify the update from the HOT path in the heap access manager. This patch is functional, includes new tests, and passes check-world however I’m sure it will require more work after community review.

Why is this important? A growing number of Postgres users work with JSONB data. Indexes on fields within JSONB columns use expressions preventing all updates on data within JSONB columns from the HOT path. This is unnecessary and a major drawback when using Postgres.

This is not a new idea; indeed, this patch grew out of Surjective functional indexes [1] which was applied [2] and then reverted [3] after it was discovered that it caused a bug and had other quality issues. This patch is a new approach that hopefully address the identified bug and most of the other concerns raised in that and other email threads on the subject. I’m also aware of PHOT [4] and WARM [5] which allow for updating some, but not all indexes while remaining on the HOT update path, this patch does not attempt to accomplish that.

Attached you’ll find two slightly different approaches. The first (v3) patch is slightly less intrusive than the second (v4), but both apply to master (59d6c03956193f622c069a4ab985bade27384ac4). Tests have been added (heap_hot_updates.sql) that exercise this new feature set. Of the two patches I personally prefer v4 as it cleans up the summarizing index logic and removes TU_UpdateIndexes. This opens the door to future improvements by providing a way to pass a bitmap of modified indexes along to be addressed by something similar to the PHOT/WARM logic.

I have a few concerns with the patch, things I’d greatly appreciate your thoughts on:

First, I pass an EState along the update path to enable running the checks in heapam, this works but leaves me feeling as if I violated separation of concerns. If there is a better way to do this let me know or if you think the cost of creating one in the execIndexing.c ExecIndexesRequiringUpdates() is okay that’s another possibility.

Second, I’m sure that creating the rd_indexinfolist should be improved/changed and likely cached via relcache.c as this is likely part of the performance overhead mentioned below.

Third, there is overhead to this patch, it is no longer a single simple bitmap test to choose HOT or not in heap_update(). Sometimes this patch will perform expensive additional checks and ultimately not go down the HOT path, new overhead with no benefit. Some expressions are more expensive than others to evaluate, there is no logic to adjust for that. The Surjective patch/email thread had quite a bit of discussion on this without resolution. I’ve chosen to add a GUC that optionally avoids the expression evaluation. I’m open to ideas here as well, addition of another GUC or removal of the one I’ve added. I’ve tried to avoid rechecking indexes for changes when possible.

Fourth, I’d like to know which version the community prefers (v3 or v4). I think v4 moves the code in a direction that is cleaner overall, but you may disagree. I realize that the way I use the modified_indexes bitmapset is a tad overloaded (NULL means all indexes should be updated, otherwise only update the indexes in the set which may be all/some/none of the indexes) and that may violate the principal of least surprise but I feel that it is better than the TU_UpdateIndexes enum in the code today.

I’ve run two performance tests against this; a very synthetic workload that updates only non-indexed fields in a JSONB document that is small enough not to be TOASTed, and one that measures TPC-C-like workload using a MongoDB API mapped into JSONB.

The synthetic workload randomly updated non-indexed fields within the JSONB documents as fast as possible from 50 client connections. The test started pre-loaded with 10,000,000 documents within a single column with 50 expression indexes (BTREE) into fields in those documents. This test showed a dramatic increase in throughput (28-110%), reduction in per-operation latency (22-52%), and lower storage requirements all while CPU remained within 1% of the unpatched server. The tests ran for 2hrs during which time we observed about a 20-30% reduction in IOPs. On the unpatched server there were no HOT updates, on the patched server with default fillfactor HOT updates made up at least 30% and at times much more as the random-access pattern and pruneheap opened space on pages for HOT updates.

The TPC-C-like workload [7] ran [8] first with —no-execute then [8] with —no-load. This test showed HOT updates for CUSTOMER (7%), DISTRICT (48%), and WAREHOUSE (99%) but zero for STOCK and ORDERS. When compared to a non-patched server the performance with this patch was 7.8% slower than without. This was clearly not the result I expected. I believe that the lower performance may in part be due to how I build and maintain the rd_indexinfolist and the overhead of executing expressions on indexes repeatedly only to find that the update still doesn’t qualify for the HOT path. I’d be very happy to hear thoughts on how I might reduce this gap if you have suggestions.

I hope to further develop this patch into a final form acceptable to this community.

best regards,
-greg

[1] https://www.postgresql.org/message-id/flat/4d9928ee-a9e6-15f9-9c82-5981f13ffca6%40postgrespro.ru
[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c203d6cf8
[3] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=05f84605dbeb9cf8279a157234b24bbb706c5256
[4] https://www.postgresql.org/message-id/flat/2ECBBCA0-4D8D-4841-8872-4A5BBDC063D2%40amazon.com
[5] https://www.postgresql.org/message-id/flat/CABOikdMop5Rb_RnS2xFdAXMZGSqcJ-P-BY2ruMd%2BbuUkJ4iDPw%40mail.gmail.com
[6] https://www.postgresql.org/message-id/flat/CABOikdMNy6yowA%2BwTGK9RVd8iw%2BCzqHeQSGpW7Yka_4RSZ_LOQ%40mail.gmail.com
[7] https://github.com/mongodb-labs/py-tpcc
[8] python3 tpcc.py —no-load —duration 3600 —warehouses 2000 —clients 50 —stop-on-error —config gpure.config mongodb

On Thu, 2025-02-06 at 22:24 +0000, Burd, Greg wrote:
> Attached find a patch that expands the cases where heap-only tuple (HOT) updates are possible
> without changing the basic semantics of HOT. This is accomplished by examining expression
> indexes for changes to determine if indexes require updating or not. A similar approach is
> taken for partial indexes, the predicate is evaluated and, in some cases, HOT updates are
> allowed.
>
> [...]
>
> Third, there is overhead to this patch, it is no longer a single simple bitmap test to choose
> HOT or not in heap_update(). Sometimes this patch will perform expensive additional checks
> and ultimately not go down the HOT path, new overhead with no benefit. Some expressions are
> more expensive than others to evaluate, there is no logic to adjust for that. The Surjective
> patch/email thread had quite a bit of discussion on this without resolution. I’ve chosen to
> add a GUC that optionally avoids the expression evaluation. I’m open to ideas here as well,
> addition of another GUC or removal of the one I’ve added. I’ve tried to avoid rechecking
> indexes for changes when possible.

I think that the goal of this patch is interesting and desirable.

The greatest concern for me is the performance impact. I think that a switch is warranted,
but I am not sure if it should be a GUC. Wouldn't it be better to have a reloption, so that
this can be configured per table? I am not sure if a global switch is necessary, but I am
not fundamentally against it.

Yours,
Laurenz Albe

On Feb 9, 2025, at 1:14 AM, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at> wrote:
>
> I think that the goal of this patch is interesting and desirable.

Thanks for taking a look at it. Which version did you prefer, v3 or v4?

> The greatest concern for me is the performance impact.

Agreed, I’m still looking for ways to minimize it (suggestions welcome).

> I think that a switch is warranted, but I am not sure if it should be a GUC.
> Wouldn't it be better to have a reloption, so that this can be configured per table?

I can remove the GUC in favor of a reloption, that makes sense to me.

> Yours,
> Laurenz Albe

best,

-greg

On Thu, 6 Feb 2025 at 23:24, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>
> Attached find a patch that expands the cases where heap-only tuple (HOT) updates are possible without changing the basic semantics of HOT. This is accomplished by examining expression indexes for changes to determine if indexes require updating or not. A similar approach is taken for partial indexes, the predicate is evaluated and, in some cases, HOT updates are allowed. Even with this patch if any index is changed, all indexes are updated. Only in cases where none are modified will this patch allow the HOT path.

So, effectively this disables the amsummarizing-based optimizations of
https://postgr.es/c/19d8e2308 ? That sounds like a bad degradation in
behaviour.

> I’m also aware of PHOT [4] and WARM [5] which allow for updating some, but not all indexes while remaining on the HOT update path, this patch does not attempt to accomplish that.
>
> [...] This opens the door to future improvements by providing a way to pass a bitmap of modified indexes along to be addressed by something similar to the PHOT/WARM logic.

<sidetrack>

I have serious doubts about the viability of any proposal working to
implement PHOT/WARM in PostgreSQL, as they seem to have an inherent
nature of fundamentally breaking the TID lifecycle:
We won't be able to clean up dead-to-everyone TIDs that were
PHOT-updated, because some index Y may still rely on it, and we can't
remove the TID from that same index Y because there is still a live
PHOT/WARM tuple later in the chain whose values for that index haven't
changed since that dead-to-everyone tuple, and thus this PHOT/WARM
tuple is the one pointed to by that index.
For HOT, this isn't much of an issue, because there is just one TID
that's impacted (and it only occupies a single LP slot, with
LP_REDIRECT). However, with PHOT/WARM, you'd relatively easily be able
to fill a page with TIDs (or even full tuples) you can't clean up with
VACUUM until the moment a the PHOT/WARM/HOT chain is broken (due to
UPDATE leaving the page or the final entry getting DELETE-d).

Unless we are somehow are able to replace the TIDs in indexes from
"intermediate dead PHOT" to "base TID"/"latest TID" (either of which
is probably also problematic for indexes that expect a TID to appear
exactly once in the index at any point in time) I don't think the
system is viable if we maintain only a single data structure to
contain all dead TIDs. If we had a datastore for dead items per index,
that'd be more likely to work, but it also would significantly
increase the memory overhead of vacuuming tables.

</sidetrack>

> I have a few concerns with the patch, things I’d greatly appreciate your thoughts on:
>
> First, I pass an EState along the update path to enable running the checks in heapam, this works but leaves me feeling as if I violated separation of concerns. If there is a better way to do this let me know or if you think the cost of creating one in the execIndexing.c ExecIndexesRequiringUpdates() is okay that’s another possibility.

I think that doesn't have to be bad.

> Third, there is overhead to this patch, it is no longer a single simple bitmap test to choose HOT or not in heap_update().

Why can't it mostly be that simple in simple cases?

I mean, it's clear that "updated indexed column's value == non-HOT
update". And that to determine whether an updated *projected* column's
value (i.e., expression index column's value) was actually updated we
need to calculate the previous and current index value, thus execute
the projection twice. But why would we have significant additional
overhead if there are no expression indexes, or when we can know by
bitmap overlap that the only interesting cases are summarizing
indexes?

I would've implemented this with (1) two new bitmaps, one each for
normal and summarizing indexes, each containing which columns are
exclusively used in expression indexes (and which should thus be used
to trigger the (comparatively) expensive recalculation).

Then, I'd maintain a (cached) list of unique projections/expressions
found in indexes, so that 30 indexes on e.g.
((mycolumn::jsonb)->>'metadata') only extend to 1 check for
differences, rather than 30. The "new" output of these expression
evaluations would be stored to be used later as index datums, reducing
the number of per-expression evaluations down to 2 at most, rather
than 2+1 when the index needs an insertion but the expression itself
wasn't updated.

So, it'd be something like (pseudocode):

if (bms_overlap(updated_columns, hotblocking))
/* if columns only indexed through expressions were updated, do
expensive stuff. Otherwise, it's a normal non-HOT update. */
if (bms_subset_compare(updated_columns, hot_expression_columns) in
(BMS_EQUAL, BMS_SUBSET1))
expensive check for expression changes + populate index column data
else
normal_update
else if (bms_overlap(updated_columns, summarizing))
/* same as above for hotblocking, but now summarizing */
if (bms_subset_compare(updated_columns, sum_expression_columns) in
(BMS_EQUAL, BMS_SUBSET1))
expensive check for summarized expression changes + populate
summarized index column data
else
summarizing_update
else
hot_update

Note that it is relatively expensive to do check whether any one index
needs to be updated. It's generally cheaper to do all those checks at
once, where possible; using one or 2 more bitmaps would be sufficient.

Also note that this approach doesn't update specific summarizing
indexes, just all of them or none. I think that "update only
summarizing indexes that were updated" should be a separate patch from
"check if indexed expressions' values changed", potentially in the
patchset, but not as part of the main bulk.

> Fourth, I’d like to know which version the community prefers (v3 or v4). I think v4 moves the code in a direction that is cleaner overall, but you may disagree. I realize that the way I use the modified_indexes bitmapset is a tad overloaded (NULL means all indexes should be updated, otherwise only update the indexes in the set which may be all/some/none of the indexes) and that may violate the principal of least surprise but I feel that it is better than the TU_UpdateIndexes enum in the code today.

I would be hesitant to let table AMs decide which indexes to update at
that precision. Note that this API would allow the AM to update only
(say) the PK index and no other indexes, which is not allowed to
happen if index consistentcy is required (which it is).

----->8-----

Do you have any documentation on the approaches used, and the specific
differences between v3 and v4? I don't see much of that in your
initial mail, and the patches themselves also don't show much of that
in their details. I'd like at least some documentation of the new
behaviour in src/backend/access/heap/README.HOT at some point before
this got marked as RFC in the commitfest app, though preferably sooner
rather than later.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

Apologies for not being clear, this preserves the current behavior for summarizing indexes allowing for HOT updates while also updating the index. No degradation here that I’m aware of, indeed the tests that ensure that behavior are unchanged and pass.

-greg

> On Feb 10, 2025, at 12:17 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>
> So, effectively this disables the amsummarizing-based optimizations of
> https://postgr.es/c/19d8e2308 ? That sounds like a bad degradation in
> behaviour.

> On Feb 10, 2025, at 12:17 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>
>>
>> I have a few concerns with the patch, things I’d greatly appreciate your thoughts on:
>>
>> First, I pass an EState along the update path to enable running the checks in heapam, this works but leaves me feeling as if I violated separation of concerns. If there is a better way to do this let me know or if you think the cost of creating one in the execIndexing.c ExecIndexesRequiringUpdates() is okay that’s another possibility.
>
> I think that doesn't have to be bad.

Meaning that the approach I’ve taken is okay with you?

>> Third, there is overhead to this patch, it is no longer a single simple bitmap test to choose HOT or not in heap_update().
>
> Why can't it mostly be that simple in simple cases?

It can remain that simple in the cases you mention. In relcache the hot blocking attributes are nearly the same, only the summarizing attributes are removed. The first test then in heap_update() is for overlap with the modified set. When there is none, the update will proceed on the HOT path.

The presence of a summarizing index is determined in ExecIndexesRequiringUpdates() in execIndexing.c, so a slightly longer code path but not much new overhead.

> I mean, it's clear that "updated indexed column's value == non-HOT
> update". And that to determine whether an updated *projected* column's
> value (i.e., expression index column's value) was actually updated we
> need to calculate the previous and current index value, thus execute
> the projection twice. But why would we have significant additional
> overhead if there are no expression indexes, or when we can know by
> bitmap overlap that the only interesting cases are summarizing
> indexes?

You’re right, there’s not a lot of new overhead in that case except what happens in ExecIndexesRequiringUpdates() to scan over the list of IndexInfo. It is really only when there are many expressions/predicates requiring examination that there is any significant cost to this approach AFAICT (but if you see something please point it out).

> I would've implemented this with (1) two new bitmaps, one each for
> normal and summarizing indexes, each containing which columns are
> exclusively used in expression indexes (and which should thus be used
> to trigger the (comparatively) expensive recalculation).

That was one where I started, over time that became harder to work as the bitmaps contain the union of index attributes for the table not per-column. Now there is one bitmap to cover the broadest case and then a function to find the modified set of indexes where each is examined against bitmaps that contain only attributes specific to the index in question. This helped in cases where there were both expression and non-expression indexes on the same attribute.

> Then, I'd maintain a (cached) list of unique projections/expressions
> found in indexes, so that 30 indexes on e.g.
> ((mycolumn::jsonb)->>'metadata') only extend to 1 check for
> differences, rather than 30.

An optimization to avoid rechecking isn’t a bad idea. I wonder how hard it would be to surface the field (->>’metadata’) from the index expression to track for redundancy, I’ll have to look into that.

> The "new" output of these expression
> evaluations would be stored to be used later as index datums, reducing
> the number of per-expression evaluations down to 2 at most, rather
> than 2+1 when the index needs an insertion but the expression itself
> wasn't updated.

Not reforming the new index tuples is also an interesting optimization. I wonder how that can be passed from within heapam’s call into a function in execIndexing up into nodeModifiyTable and back down into execIndexing and on to the index access method? I’ll have to think about that, ideas welcome.

> So, it'd be something like (pseudocode):
>
> if (bms_overlap(updated_columns, hotblocking))
> /* if columns only indexed through expressions were updated, do
> expensive stuff. Otherwise, it's a normal non-HOT update. */
> if (bms_subset_compare(updated_columns, hot_expression_columns) in
> (BMS_EQUAL, BMS_SUBSET1))
> expensive check for expression changes + populate index column data
> else
> normal_update
> else if (bms_overlap(updated_columns, summarizing))
> /* same as above for hotblocking, but now summarizing */
> if (bms_subset_compare(updated_columns, sum_expression_columns) in
> (BMS_EQUAL, BMS_SUBSET1))
> expensive check for summarized expression changes + populate
> summarized index column data
> else
> summarizing_update
> else
> hot_update
>
> Note that it is relatively expensive to do check whether any one index
> needs to be updated. It's generally cheaper to do all those checks at
> once, where possible; using one or 2 more bitmaps would be sufficient.
>
> Also note that this approach doesn't update specific summarizing
> indexes, just all of them or none. I think that "update only
> summarizing indexes that were updated" should be a separate patch from
> "check if indexed expressions' values changed", potentially in the
> patchset, but not as part of the main bulk.
>
>> Fourth, I’d like to know which version the community prefers (v3 or v4). I think v4 moves the code in a direction that is cleaner overall, but you may disagree. I realize that the way I use the modified_indexes bitmapset is a tad overloaded (NULL means all indexes should be updated, otherwise only update the indexes in the set which may be all/some/none of the indexes) and that may violate the principal of least surprise but I feel that it is better than the TU_UpdateIndexes enum in the code today.
>
> I would be hesitant to let table AMs decide which indexes to update at
> that precision. Note that this API would allow the AM to update only
> (say) the PK index and no other indexes, which is not allowed to
> happen if index consistentcy is required (which it is).

Interesting, thanks for the feedback. I’ll think on this a bit more and provide more detail with the next update.

> ----->8-----
>
> Do you have any documentation on the approaches used, and the specific
> differences between v3 and v4? I don't see much of that in your
> initial mail, and the patches themselves also don't show much of that
> in their details. I'd like at least some documentation of the new
> behaviour in src/backend/access/heap/README.HOT at some point before
> this got marked as RFC in the commitfest app, though preferably sooner
> rather than later.

Good point, I should have updated README.HOT with the initial patchset. I’ll jump on that and update ASAP.

> Kind regards,
>
> Matthias van de Meent
> Neon (https://neon.tech)

thanks for the thoughtful reply.

-greg

On Mon, Feb 10, 2025 at 06:17:42PM +0100, Matthias van de Meent wrote:
> I have serious doubts about the viability of any proposal working to
> implement PHOT/WARM in PostgreSQL, as they seem to have an inherent
> nature of fundamentally breaking the TID lifecycle:
> We won't be able to clean up dead-to-everyone TIDs that were
> PHOT-updated, because some index Y may still rely on it, and we can't
> remove the TID from that same index Y because there is still a live
> PHOT/WARM tuple later in the chain whose values for that index haven't
> changed since that dead-to-everyone tuple, and thus this PHOT/WARM
> tuple is the one pointed to by that index.
> For HOT, this isn't much of an issue, because there is just one TID
> that's impacted (and it only occupies a single LP slot, with
> LP_REDIRECT). However, with PHOT/WARM, you'd relatively easily be able
> to fill a page with TIDs (or even full tuples) you can't clean up with
> VACUUM until the moment a the PHOT/WARM/HOT chain is broken (due to
> UPDATE leaving the page or the final entry getting DELETE-d).
>
> Unless we are somehow are able to replace the TIDs in indexes from
> "intermediate dead PHOT" to "base TID"/"latest TID" (either of which
> is probably also problematic for indexes that expect a TID to appear
> exactly once in the index at any point in time) I don't think the
> system is viable if we maintain only a single data structure to
> contain all dead TIDs. If we had a datastore for dead items per index,
> that'd be more likely to work, but it also would significantly
> increase the memory overhead of vacuuming tables.

I share your concerns, but I don't think things are as dire as you suggest.
For example, perhaps we put a limit on how long a PHOT chain can be, or
maybe we try to detect update patterns that don't work well with PHOT.
Another option could be to limit PHOT updates to only when the same set of
indexed columns are updated or when <50% of the indexed columns are
updated. These aren't fully fleshed-out ideas, of course, but I am at
least somewhat optimistic we could find appropriate trade-offs.

--
nathan

On Tue, 11 Feb 2025 at 00:20, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> On Mon, Feb 10, 2025 at 06:17:42PM +0100, Matthias van de Meent wrote:
> > I have serious doubts about the viability of any proposal working to
> > implement PHOT/WARM in PostgreSQL, as they seem to have an inherent
> > nature of fundamentally breaking the TID lifecycle:
> > [... concerns]
>
> I share your concerns, but I don't think things are as dire as you suggest.
> For example, perhaps we put a limit on how long a PHOT chain can be, or
> maybe we try to detect update patterns that don't work well with PHOT.
> Another option could be to limit PHOT updates to only when the same set of
> indexed columns are updated or when <50% of the indexed columns are
> updated. These aren't fully fleshed-out ideas, of course, but I am at
> least somewhat optimistic we could find appropriate trade-offs.

Yes, there are methods which could limit the overhead. But I'm not
sure there are cheap-enough designs which would make PHOT a
universally good choice (i.e. not tunable with guc/table option),
considering its significantly larger un-reclaimable storage overhead
vs HOT.

Kind regards,

Matthias van de Meent.

On Mon, 10 Feb 2025 at 20:11, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
> > On Feb 10, 2025, at 12:17 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
> >
> >>
> >> I have a few concerns with the patch, things I’d greatly appreciate your thoughts on:
> >>
> >> First, I pass an EState along the update path to enable running the checks in heapam, this works but leaves me feeling as if I violated separation of concerns. If there is a better way to do this let me know or if you think the cost of creating one in the execIndexing.c ExecIndexesRequiringUpdates() is okay that’s another possibility.
> >
> > I think that doesn't have to be bad.
>
> Meaning that the approach I’ve taken is okay with you?

If you mean "passing EState down through the AM so we can check if we
really need HOT updates", then Yes, that's OK. I don't think the basic
idea (executing the projections to check for differences) is bad per
se, however I do think we may need more design work on the exact shape
of how ExecIndexesRequiringUpdates() receives its information (which
includes the EState).

E.g. we could pass an opaquely typed pointer that's passed from the
executor state through the table_update method into this
ExecIndexesRequiringUpdates(). That opaque struct would then contain
the important information for index update state checking, so that the
AM can't realistically break things without bypassing the separation
of concerns, and doesn't have to know about any executor nodes.

>>> Third, there is overhead to this patch, it is no longer a single simple bitmap test to choose HOT or not in heap_update().
>>
>> Why can't it mostly be that simple in simple cases?
>
> It can remain that simple in the cases you mention. In relcache the hot blocking attributes are nearly the same, only the summarizing attributes are removed. The first test then in heap_update() is for overlap with the modified set. When there is none, the update will proceed on the HOT path.
>
> The presence of a summarizing index is determined in ExecIndexesRequiringUpdates() in execIndexing.c, so a slightly longer code path but not much new overhead.

Yes, but that's only determined at an index-by-index level, rather
than determined all at once, and that's real bad when you have
hundreds of indexes to go through (however unlikely it might be, I've
heard of cases where there are 1000s of indexes on one table). So, I
prefer limiting any O(n_indexes) operations to only the most critical
cases.

> > I mean, it's clear that "updated indexed column's value == non-HOT
> > update". And that to determine whether an updated *projected* column's
> > value (i.e., expression index column's value) was actually updated we
> > need to calculate the previous and current index value, thus execute
> > the projection twice. But why would we have significant additional
> > overhead if there are no expression indexes, or when we can know by
> > bitmap overlap that the only interesting cases are summarizing
> > indexes?
>
> You’re right, there’s not a lot of new overhead in that case except what happens in ExecIndexesRequiringUpdates() to scan over the list of IndexInfo. It is really only when there are many expressions/predicates requiring examination that there is any significant cost to this approach AFAICT (but if you see something please point it out).

See the attached approach. Evaluation of the expressions only has to
happen if there are any HOT-blocking attributes which are exclusively
hot-blockingly indexed through expressions, so if the updated
attribute numbers are a subset of hotblockingexprattrs. (substitute
hotblocking with summarizing for the summarizing approach)

> > I would've implemented this with (1) two new bitmaps, one each for
> > normal and summarizing indexes, each containing which columns are
> > exclusively used in expression indexes (and which should thus be used
> > to trigger the (comparatively) expensive recalculation).
>
> That was one where I started, over time that became harder to work as the bitmaps contain the union of index attributes for the table not per-column.

I think it's fairly easy to create, though.

> Now there is one bitmap to cover the broadest case and then a function to find the modified set of indexes where each is examined against bitmaps that contain only attributes specific to the index in question. This helped in cases where there were both expression and non-expression indexes on the same attribute.

Fair, but do we care about one expression index on (attr1->>'data')'s
value *not* changing when an index on (attr1) exists and attr1 has
changed? That index on att1 would block HOT updates regardless of the
(lack of) changes to the (att1->>'data') index, so doing those
expensive calculations seems quite wasteful.

So, in my opinion, we should also keep track of those attributes only
included in expressions of indexes, and that's fairly easy: see
attached prototype.diff.txt (might need some work, the patch was
drafted on v16's codebase, but the idea is clear).

The resulting %exprattrs bitmap contains attributes that are used only
in expressions of those index types.

> > The "new" output of these expression
> > evaluations would be stored to be used later as index datums, reducing
> > the number of per-expression evaluations down to 2 at most, rather
> > than 2+1 when the index needs an insertion but the expression itself
> > wasn't updated.
>
> Not reforming the new index tuples is also an interesting optimization. I wonder how that can be passed from within heapam’s call into a function in execIndexing up into nodeModifiyTable and back down into execIndexing and on to the index access method? I’ll have to think about that, ideas welcome.

Note that index tuple forming happens only in the index AM, it's the
Datum construction (i.e. projection from attributes/tuple to indexed
value) that I'd like to deduplicate. Though looking at the current
code, I don't think it's reasonable to have that as a requirement for
this work. It'd be a nice-to-have for sure, but not as requirement.

> > Do you have any documentation on the approaches used, and the specific
> > differences between v3 and v4? I don't see much of that in your
> > initial mail, and the patches themselves also don't show much of that
> > in their details. I'd like at least some documentation of the new
> > behaviour in src/backend/access/heap/README.HOT at some point before
> > this got marked as RFC in the commitfest app, though preferably sooner
> > rather than later.
>
> Good point, I should have updated README.HOT with the initial patchset. I’ll jump on that and update ASAP.

Thanks in advance.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Mon, 10 Feb 2025 at 19:15, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>
> Apologies for not being clear, this preserves the current behavior for summarizing indexes allowing for HOT updates while also updating the index. No degradation here that I’m aware of, indeed the tests that ensure that behavior are unchanged and pass.

Looking at the code again, while it does indeed preserve the current
behaviour, it doesn't actually improve the behavior for summarizing
indexes when that would be expected.

Example:

CREATE INDEX hotblocking ON mytab USING btree((att1->'data'));
CREATE INDEX summarizing ON mytab USING BRIN(att2);
UPDATE mytab SET att1 = att1 || '{"check": "mate"}';

In v3 (same code present in v4), I notice that in the above case we
hit the "indexed attribute updated" path (hotblocking indeed indexes
the updated attribute att1), go into ExecIndexesRequiringUpdates, and
mark index 'summarizing' as 'needs an update', even though no
attribute of that index has a new value. Then we notice that
att1->'data' hasn't changed, and so we don't need to update the
'hotblocking' index, but we do update the (unchanged) 'summarizing'
index.

This indicates that in practice (with this version of the patch) this
will improve the HOT applicability situation while summarizing indexes
don't really gain a benefit from this - they're always updated when
any indexed column is updated, even if we could detect that there were
no changes to any indexed values.

Actually, you could say we find ourselves in the counter-intuitive
situation that the addition of the 'hotblocking' index whose value
were not updated now caused index insertions into summarizing indexes.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

Matthias,

Thanks for the in-depth review, you are correct and I appreciate you uncovering that oversight with summarizing indexes. I’ll add a test case and modify the logic to prevent updates to unchanged summarizing indexes by testing their attributes against the modified set while keeping the HOT optimization when only summarizing indexes are changed.

thanks for finding this,

-greg

> On Feb 11, 2025, at 4:40 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>
> On Mon, 10 Feb 2025 at 19:15, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>>
>> Apologies for not being clear, this preserves the current behavior for summarizing indexes allowing for HOT updates while also updating the index. No degradation here that I’m aware of, indeed the tests that ensure that behavior are unchanged and pass.
>
> Looking at the code again, while it does indeed preserve the current
> behaviour, it doesn't actually improve the behavior for summarizing
> indexes when that would be expected.
>
> Example:
>
> CREATE INDEX hotblocking ON mytab USING btree((att1->'data'));
> CREATE INDEX summarizing ON mytab USING BRIN(att2);
> UPDATE mytab SET att1 = att1 || '{"check": "mate"}';
>
> In v3 (same code present in v4), I notice that in the above case we
> hit the "indexed attribute updated" path (hotblocking indeed indexes
> the updated attribute att1), go into ExecIndexesRequiringUpdates, and
> mark index 'summarizing' as 'needs an update', even though no
> attribute of that index has a new value. Then we notice that
> att1->'data' hasn't changed, and so we don't need to update the
> 'hotblocking' index, but we do update the (unchanged) 'summarizing'
> index.
>
> This indicates that in practice (with this version of the patch) this
> will improve the HOT applicability situation while summarizing indexes
> don't really gain a benefit from this - they're always updated when
> any indexed column is updated, even if we could detect that there were
> no changes to any indexed values.
>
> Actually, you could say we find ourselves in the counter-intuitive
> situation that the addition of the 'hotblocking' index whose value
> were not updated now caused index insertions into summarizing indexes.
>
> Kind regards,
>
> Matthias van de Meent
> Neon (https://neon.tech)

Attached find an updated patchset v5 that is an evolution of v4.

Changes v4 to v5 are:
* replaced GUC with table reloption called "expression_checks" (open to other name ideas)
* minimal documentation updates to README.HOT to address changes
* avoid, when possible, the expensive path that requires evaluating an estate using bitmaps
* determines the set of summarized indexes requiring updates, only updates those
* more tests in heap_hot_updates.sql (perhaps too many...)
* rebased to master, formatted, and make check-world passes

More comments in context below...

-greg

> On Feb 11, 2025, at 4:18 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>
>
> On Mon, 10 Feb 2025 at 20:11, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>>> On Feb 10, 2025, at 12:17 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>>>
>>>>
>>>> I have a few concerns with the patch, things I’d greatly appreciate your thoughts on:
>>>>
>>>> First, I pass an EState along the update path to enable running the checks in heapam, this works but leaves me feeling as if I violated separation of concerns. If there is a better way to do this let me know or if you think the cost of creating one in the execIndexing.c ExecIndexesRequiringUpdates() is okay that’s another possibility.
>>>
>>> I think that doesn't have to be bad.
>>
>> Meaning that the approach I’ve taken is okay with you?
>
> If you mean "passing EState down through the AM so we can check if we
> really need HOT updates", then Yes, that's OK. I don't think the basic
> idea (executing the projections to check for differences) is bad per
> se, however I do think we may need more design work on the exact shape
> of how ExecIndexesRequiringUpdates() receives its information (which
> includes the EState).
>
> E.g. we could pass an opaquely typed pointer that's passed from the
> executor state through the table_update method into this
> ExecIndexesRequiringUpdates(). That opaque struct would then contain
> the important information for index update state checking, so that the
> AM can't realistically break things without bypassing the separation
> of concerns, and doesn't have to know about any executor nodes.

I'm open to this idea and will attempt an implementation in v6, ideas welcome.

>>>> Third, there is overhead to this patch, it is no longer a single simple bitmap test to choose HOT or not in heap_update().
>>>
>>> Why can't it mostly be that simple in simple cases?
>>
>> It can remain that simple in the cases you mention. In relcache the hot blocking attributes are nearly the same, only the summarizing attributes are removed. The first test then in heap_update() is for overlap with the modified set. When there is none, the update will proceed on the HOT path.
>>
>> The presence of a summarizing index is determined in ExecIndexesRequiringUpdates() in execIndexing.c, so a slightly longer code path but not much new overhead.
>
> Yes, but that's only determined at an index-by-index level, rather
> than determined all at once, and that's real bad when you have
> hundreds of indexes to go through (however unlikely it might be, I've
> heard of cases where there are 1000s of indexes on one table). So, I
> prefer limiting any O(n_indexes) operations to only the most critical
> cases.

This makes sense, and I agree that avoiding O(n_indexes) operations is a good goal when possible.

>>> I mean, it's clear that "updated indexed column's value == non-HOT
>>> update". And that to determine whether an updated *projected* column's
>>> value (i.e., expression index column's value) was actually updated we
>>> need to calculate the previous and current index value, thus execute
>>> the projection twice. But why would we have significant additional
>>> overhead if there are no expression indexes, or when we can know by
>>> bitmap overlap that the only interesting cases are summarizing
>>> indexes?
>>
>> You’re right, there’s not a lot of new overhead in that case except what happens in ExecIndexesRequiringUpdates() to scan over the list of IndexInfo. It is really only when there are many expressions/predicates requiring examination that there is any significant cost to this approach AFAICT (but if you see something please point it out).
>
> See the attached approach. Evaluation of the expressions only has to
> happen if there are any HOT-blocking attributes which are exclusively
> hot-blockingly indexed through expressions, so if the updated
> attribute numbers are a subset of hotblockingexprattrs. (substitute
> hotblocking with summarizing for the summarizing approach)

I believe I've incorporated the gist of your idea in this v5 patch, let me know if I missed something.

>>> I would've implemented this with (1) two new bitmaps, one each for
>>> normal and summarizing indexes, each containing which columns are
>>> exclusively used in expression indexes (and which should thus be used
>>> to trigger the (comparatively) expensive recalculation).
>>
>> That was one where I started, over time that became harder to work as the bitmaps contain the union of index attributes for the table not per-column.
>
> I think it's fairly easy to create, though.
>
>> Now there is one bitmap to cover the broadest case and then a function to find the modified set of indexes where each is examined against bitmaps that contain only attributes specific to the index in question. This helped in cases where there were both expression and non-expression indexes on the same attribute.
>
> Fair, but do we care about one expression index on (attr1->>'data')'s
> value *not* changing when an index on (attr1) exists and attr1 has
> changed? That index on att1 would block HOT updates regardless of the
> (lack of) changes to the (att1->>'data') index, so doing those
> expensive calculations seems quite wasteful.

Agreed, when both a non-expression and an expression index exist on the same attribute then the expression checks are unnecessary and should be avoided. In this v5 patchset this case becomes two checks of bitmaps (first hot_attrs, then exclusively exp_attrs) before proceeding with a non-HOT update.

> So, in my opinion, we should also keep track of those attributes only
> included in expressions of indexes, and that's fairly easy: see
> attached prototype.diff.txt (might need some work, the patch was
> drafted on v16's codebase, but the idea is clear).

Thank you for your patch, I've included and expanded it.

> The resulting %exprattrs bitmap contains attributes that are used only
> in expressions of those index types.
>
>>> The "new" output of these expression
>>> evaluations would be stored to be used later as index datums, reducing
>>> the number of per-expression evaluations down to 2 at most, rather
>>> than 2+1 when the index needs an insertion but the expression itself
>>> wasn't updated.
>>
>> Not reforming the new index tuples is also an interesting optimization. I wonder how that can be passed from within heapam’s call into a function in execIndexing up into nodeModifiyTable and back down into execIndexing and on to the index access method? I’ll have to think about that, ideas welcome.
>
> Note that index tuple forming happens only in the index AM, it's the
> Datum construction (i.e. projection from attributes/tuple to indexed
> value) that I'd like to deduplicate. Though looking at the current
> code, I don't think it's reasonable to have that as a requirement for
> this work. It'd be a nice-to-have for sure, but not as requirement.

Agreed that it's a nice-to-have, but not a priority.

>>> Do you have any documentation on the approaches used, and the specific
>>> differences between v3 and v4? I don't see much of that in your
>>> initial mail, and the patches themselves also don't show much of that
>>> in their details. I'd like at least some documentation of the new
>>> behaviour in src/backend/access/heap/README.HOT at some point before
>>> this got marked as RFC in the commitfest app, though preferably sooner
>>> rather than later.
>>
>> Good point, I should have updated README.HOT with the initial patchset. I’ll jump on that and update ASAP.
>
> Thanks in advance.
>
> Kind regards,
>
> Matthias van de Meent
> Neon (https://neon.tech)
> <prototype.diff.txt>

On Thu, 13 Feb 2025 at 19:46, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>
> Attached find an updated patchset v5 that is an evolution of v4.
>
> Changes v4 to v5 are:
> * replaced GUC with table reloption called "expression_checks" (open to other name ideas)
> * minimal documentation updates to README.HOT to address changes
> * avoid, when possible, the expensive path that requires evaluating an estate using bitmaps
> * determines the set of summarized indexes requiring updates, only updates those
> * more tests in heap_hot_updates.sql (perhaps too many...)
> * rebased to master, formatted, and make check-world passes

Thank you for the update. Below some comments, in no particular order.

-----

I'm not a fan of how you replaced TU_UpdateIndexes with a bitmap. It
seems unergonomic and a waste of performance.

In HEAD, we don't do any expensive computations for the fast path of
"indexed attribute updated" - at most we do the bitmap compare and
then set a pointer. With this patch, the way we signal that is by
allocating a bitmap of size O(n_indexes). That's potentially quite
expensive (given 1000s of indexes), and definitely more expensive than
only a pointer assignment.
In HEAD, we have a clear indication of which classes of indexes to
update, with TU_UpdateIndexes. With this patch, we have to derive that
from the (lack of) bits in the bitmap that might be output by the
table_update procedure.

I think we can do with an additional parameter for which indexes would
be updated (or store that info in the parameter which also will hold
EState et al). I think it's cheaper that way, too - only when
update_indexes could be TU_SUMMARIZING we might need the exact
information for which indexes to insert new tuples into, and it only
really needs to be sized to the number of summarizing indexes (usually
small/nonexistent, but potentially huge).

-----

I think your patch design came from trying to include at least two
distinct optimizations:
1) to make the HOT-or-not check include whether the expressions of
indexes were updated, and
2) to only insert index tuples into indexes that got updated values
when table_tuple_update returns update_indexes=TU_Summarizing.

While they touch similar code (clearly seen here), I think those
should be implemented in different patches. For (1), the current API
surface is good enough when the EState is passed down. For (2), you'll
indeed also need an additional argument we can use to fill with the
right summarizing indexes, but I don't think that can nor should
replace the function of TU_UpdateIndexes.

If you agree with my observation of those being distinct
optimizations, could you split this patch into parts (but still within
the same series) so that these are separately reviewable?

-----

I notice that ExecIndexesRequiringUpdates() does work on all indexes,
rather than just indexes relevant to this exact phase of checking. I
think that is a waste of time, so if we sort the indexes in order of
[hotblocking without expressions, hotblocking with expressions,
summarizing], then (with stored start/end indexes) we can save time in
cases where there are comparatively few of the types we're not going
to look at.

As an extreme example: we shouldn't do the (comparatively) expensive
work evaluating expressions to determine which of 1000s of summarizing
indexes has been updated when we're still not sure if we can apply HOT
at all.

(Sidenote: Though, arguably, we could be smarter by skipping index
insertions into unmodified summarizing indexes altogether regardless
of HOT status, as long as the update is on the same page - but that's
getting ahead of ourselves and not relevant to this discussion.)

-----

I noticed you've disabled any passing of "HOT or not" in the
simple_update cases, and have done away with the various checks that
are in place to prevent corruption. I don't think that's a great idea,
it's quite likely to cause bugs.

-----

You're extracting type info from the opclass, to use in
datum_image_eq(). Couldn't you instead use the index relation's
TupleDesc and its stored attribute information instead? That saves us
from having to do further catalog lookups during execution. I'm also
fairly sure that that information is supposed to be a more accurate
representation of attributes' expression output types than the
opclass' type information (though, they probably should match).

-----

The operations applied in ExecIndexesRequiringUpdates partially
duplicate those done in index_unchanged_by_update. Can we (partially)
unify this, and pass which indexes were updated through the IndexInfo,
rather than the current bitmap?

-----

I don't see a good reason to add IndexInfo to Relation, by way of
rd_indexInfoList. It seems like an ad-hoc way of passing data around,
and I don't think that's the right way.

>>>> I mean, it's clear that "updated indexed column's value == non-HOT
>>>> update". And that to determine whether an updated *projected* column's
>>>> value (i.e., expression index column's value) was actually updated we
>>>> need to calculate the previous and current index value, thus execute
>>>> the projection twice. But why would we have significant additional
>>>> overhead if there are no expression indexes, or when we can know by
>>>> bitmap overlap that the only interesting cases are summarizing
>>>> indexes?
>>
>> See the attached approach. Evaluation of the expressions only has to
>> happen if there are any HOT-blocking attributes which are exclusively
>> hot-blockingly indexed through expressions, so if the updated
>> attribute numbers are a subset of hotblockingexprattrs. (substitute
>> hotblocking with summarizing for the summarizing approach)
>
> I believe I've incorporated the gist of your idea in this v5 patch, let me know if I missed something.

Seems about accurate.

>>>> I would've implemented this with (1) two new bitmaps, one each for
>>>> normal and summarizing indexes, each containing which columns are
>>>> exclusively used in expression indexes (and which should thus be used
>>>> to trigger the (comparatively) expensive recalculation).
>>>
>>> That was one where I started, over time that became harder to work as the bitmaps contain the union of index attributes for the table not per-column.
>>
>> I think it's fairly easy to create, though.
>>
>>> Now there is one bitmap to cover the broadest case and then a function to find the modified set of indexes where each is examined against bitmaps that contain only attributes specific to the index in question. This helped in cases where there were both expression and non-expression indexes on the same attribute.
>>
>> Fair, but do we care about one expression index on (attr1->>'data')'s
>> value *not* changing when an index on (attr1) exists and attr1 has
>> changed? That index on att1 would block HOT updates regardless of the
>> (lack of) changes to the (att1->>'data') index, so doing those
>> expensive calculations seems quite wasteful.
>
> Agreed, when both a non-expression and an expression index exist on the same attribute then the expression checks are unnecessary and should be avoided. In this v5 patchset this case becomes two checks of bitmaps (first hot_attrs, then exclusively exp_attrs) before proceeding with a non-HOT update.

> > So, in my opinion, we should also keep track of those attributes only
> > included in expressions of indexes, and that's fairly easy: see
> > attached prototype.diff.txt (might need some work, the patch was
> > drafted on v16's codebase, but the idea is clear).
>
> Thank you for your patch, I've included and expanded it.

Matthias,

First off, I can't thank you enough for taking the time to review in detail the patch. I appreciate and value your time and excellent feedback.

Second, I think that I should admit to the fact that I've also been working on making PHOT functional again. I have it rebased against master, however it is still at the proof-of-concept phase and there are some larger issues to flesh out. One of the changes in this patch would have enabled that future with PHOT, specifically the bitmap as the method for conveying what indexes need updating.

That said, I agree that this patch should simply focus on expanding HOT to expression indexes and partial indexes. With that in mind I will return to the TU_UpdateIndexes approach, even though I'm not a fan of it, and later propose the bitmap approach (or something better) as part of PHOT if and when that's ready.

Changes v5 to v6:
* reverted to TU_UpdateIndexes rather than Bitmapset
* renamed ExecIndexesRequiringUpdates to ExecIndexesExpressionsWereNotUpdated
* ExecIndexesExpressionsWereNotUpdated returns bool, exits early if possible
* simple_update paths can be HOT once again
* removed efforts to determine the subset of updated summarizing indexes
* create filtered IndexInfo list in relcache containing only indexes with expressions
* now using index TupleDesc CompactAttributes for arguments to datum_is_equal() <- did I get this one right, I'm not sure it is what you had in mind

best.

-greg

> On Feb 15, 2025, at 5:49 AM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>
> On Thu, 13 Feb 2025 at 19:46, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>
> -----
>
> I'm not a fan of how you replaced TU_UpdateIndexes with a bitmap. It
> seems unergonomic and a waste of performance.
>
> In HEAD, we don't do any expensive computations for the fast path of
> "indexed attribute updated" - at most we do the bitmap compare and
> then set a pointer. With this patch, the way we signal that is by
> allocating a bitmap of size O(n_indexes). That's potentially quite
> expensive (given 1000s of indexes), and definitely more expensive than
> only a pointer assignment.

Fair point. I'm not a fan of the TU_UpdateIndexes enum, but I appreciate your argument against the bitmapset. Under the conditions you describe (1000s of indexes) it could grow unwieldy and impact performance.

> In HEAD, we have a clear indication of which classes of indexes to
> update, with TU_UpdateIndexes. With this patch, we have to derive that
> from the (lack of) bits in the bitmap that might be output by the
> table_update procedure.

Yes, but... that "clear indication" is lacking the ability to convey more detailed information. It doesn't tell you which summarizing indexes really need updating just that as a result of being on the HOT path all summarizing indexes require updates.

> I think we can do with an additional parameter for which indexes would
> be updated (or store that info in the parameter which also will hold
> EState et al). I think it's cheaper that way, too - only when
> update_indexes could be TU_SUMMARIZING we might need the exact
> information for which indexes to insert new tuples into, and it only
> really needs to be sized to the number of summarizing indexes (usually
> small/nonexistent, but potentially huge).

Okay, yes with this patch we need only concern ourselves with all, none, or some subset of summarizing as before. I'll work on the opaque parameter next iteration.

> -----
>
> I think your patch design came from trying to include at least two
> distinct optimizations:
> 1) to make the HOT-or-not check include whether the expressions of
> indexes were updated, and
> 2) to only insert index tuples into indexes that got updated values
> when table_tuple_update returns update_indexes=TU_Summarizing.

It did. (1) for sure, but the second is more related to the PHOT work (as mentioned above). With that work almost all updates are on the HOT/PHOT path and so the bitmap of changed indexes is small. It would take an update of a table with 1000s of indexes where almost all those were modified to create a bitmap that was large, which certainly could (and somewhere likely does) exist, but it's not the common case (I'd imagine). In that case the work to update all those indexes will likely dwarf the work to build that bitmap, but I could be wrong.

But you're fundamentally right, I'm conflating two ideas and I shouldn't. I will focus the changes required for this idea without pulling in changes useful to a future ones.

> While they touch similar code (clearly seen here), I think those
> should be implemented in different patches. For (1), the current API
> surface is good enough when the EState is passed down. For (2), you'll
> indeed also need an additional argument we can use to fill with the
> right summarizing indexes, but I don't think that can nor should
> replace the function of TU_UpdateIndexes.

Okay, I'll combine the earlier v3 patch with the changes in v5. That should leave TU_UpdateIndexes in place and allow for HOT with expression indexes. I'll change the ExecIndexesRequiringUpdates() a bit (and rename it) so that it is just a boolean test for any index that spoils the HOT path and can exit early in that case potentially avoiding extra work.

> If you agree with my observation of those being distinct
> optimizations, could you split this patch into parts (but still within
> the same series) so that these are separately reviewable?

I agree, but I think that a single simple more focused patch will suffice.

> -----
>
> I notice that ExecIndexesRequiringUpdates() does work on all indexes,
> rather than just indexes relevant to this exact phase of checking. I
> think that is a waste of time, so if we sort the indexes in order of
> [hotblocking without expressions, hotblocking with expressions,
> summarizing], then (with stored start/end indexes) we can save time in
> cases where there are comparatively few of the types we're not going
> to look at.

If I plan on just having ExecIndexesRequiringUpdates() return a bool rather than a bitmap then sorting, or even just filtering the list of IndexInfo to only include indexes with expressions, makes sense. That way the only indexes in question in that function's loop will be those that may spoil the HOT path. When that list is length 0, we can skip the tests entirely.

> As an extreme example: we shouldn't do the (comparatively) expensive
> work evaluating expressions to determine which of 1000s of summarizing
> indexes has been updated when we're still not sure if we can apply HOT
> at all.

That makes sense, and your sorting idea would inform that kind of work. I'll keep that in mind if I reintroduce code that aims to only update changed summarized indexes.

> (Sidenote: Though, arguably, we could be smarter by skipping index
> insertions into unmodified summarizing indexes altogether regardless
> of HOT status, as long as the update is on the same page - but that's
> getting ahead of ourselves and not relevant to this discussion.)
>
> -----
>
> I noticed you've disabled any passing of "HOT or not" in the
> simple_update cases, and have done away with the various checks that
> are in place to prevent corruption. I don't think that's a great idea,
> it's quite likely to cause bugs.

Yes. I'll resurrect that.

> -----
>
> You're extracting type info from the opclass, to use in
> datum_image_eq(). Couldn't you instead use the index relation's
> TupleDesc and its stored attribute information instead? That saves us
> from having to do further catalog lookups during execution. I'm also
> fairly sure that that information is supposed to be a more accurate
> representation of attributes' expression output types than the
> opclass' type information (though, they probably should match).

I hadn't thought of that, I think it's a valid idea and I'll update accordingly. I think I understand what you are suggesting.

>
> -----
>
> The operations applied in ExecIndexesRequiringUpdates partially
> duplicate those done in index_unchanged_by_update. Can we (partially)
> unify this, and pass which indexes were updated through the IndexInfo,
> rather than the current bitmap?

I think I do that now, feel free to say otherwise. When the expression is checked in ExecIndexesExpressionsWereNotUpdated() I set:

/* Shortcut index_unchanged_by_update(), we know the answer. */ indexInfo->ii_CheckedUnchanged = true; indexInfo->ii_IndexUnchanged = !changed;

That prevents duplicate effort in index_unchanged_by_update().

> -----
>
> I don't see a good reason to add IndexInfo to Relation, by way of
> rd_indexInfoList. It seems like an ad-hoc way of passing data around,
> and I don't think that's the right way.

At one point I'd created a way to get this set via relcache, I will resurrect that approach but I'm not sure it is what you were hinting at. The current method avoids pulling a the lock on the index to build the list, but doing that once in relcache isn't horrible. Maybe you were suggesting using that opaque struct to pass around the list of IndexInfo? Let me know on this one if you had a specific idea. The swap I've made in v6 really just moves the IndexInfo list to a filtered list with a new name created in relcache.

Changes v6 to v7:
* Fixed documentation oversight causing build failure
* Changed how I convey attribute len/by-val in IndexInfo
* Fixed method to shortcut index_unchanged_by_update() when possible

-greg

Hello,

I've rebased and updated the patch a bit. The biggest change is that the performance penalty measured with v1 of this patch is essentially gone in v10. The overhead was due to re-creating IndexInfo information unnecessarily, which I found existed in the estate. I've added a few fields in IndexInfo that are not populated by default but necessary when checking expression indexes, those fields are populated on demand and only once limiting their overhead.

Here's what you'll find if you look into execIndexing.c where the majority of changes happened.

* assumes estate->es_result_relations[0] is the ResultRelInfo being updated
* uses ri_IndexRelationInfo[] from within estate rather than re-creating it
* augments IndexInfo only when needed for testing expressions and only once
* only creates a local old/new TupleTableSlot when not present in estate
* retains existing summarized index HOT update logic

One remaining concern stems from the assumption that estate->es_result_relations[0] is always going to be the relation being updated. This is guarded by assert()'s in the patch. It seems this is safe, all tests are passing (including TAP) and my review of the code seems to line up with that assumption. That said... opinions?

Another lingering question is under what conditions the old/new TupleTableSlots are not created and available via the ResultRelInfo found in estate. I've only seen this happen when there is an INSERT ... ON CONFLICT UPDATE ... with expression indexes. I was hopeful that in all cases I could avoid re-creating those when checking expression indexes to avoid that repeated overhead. I still avoid it when possible in this patch.

When you have time I'd appreciate any feedback.

-greg
Amazon Web Services: https://aws.amazon.com

Hi,

Sorry for the delay. This is a reply for the mail thread up to 17 Feb,
so it might be very out-of-date by now, in which case sorry for the
noise.

On Mon, 17 Feb 2025 at 20:54, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
> On Feb 15, 2025, at 5:49 AM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
> >
> > In HEAD, we have a clear indication of which classes of indexes to
> > update, with TU_UpdateIndexes. With this patch, we have to derive that
> > from the (lack of) bits in the bitmap that might be output by the
> > table_update procedure.
>
> Yes, but... that "clear indication" is lacking the ability to convey more detailed information. It doesn't tell you which summarizing indexes really need updating just that as a result of being on the HOT path all summarizing indexes require updates.

Agreed that it's not great if you want to know about which indexes
were meaningfully updated. I think that barring significant advances
in performance of update checks, we can devise a way of transfering
this info to the table_tuple_update caller once we get a need for more
detailed information (e.g. this could be transfered through the
IndexInfo* that's currently also used by index_unchanged_by_update).

> > I think we can do with an additional parameter for which indexes would
> > be updated (or store that info in the parameter which also will hold
> > EState et al). I think it's cheaper that way, too - only when
> > update_indexes could be TU_SUMMARIZING we might need the exact
> > information for which indexes to insert new tuples into, and it only
> > really needs to be sized to the number of summarizing indexes (usually
> > small/nonexistent, but potentially huge).
>
> Okay, yes with this patch we need only concern ourselves with all, none, or some subset of summarizing as before. I'll work on the opaque parameter next iteration.

Thanks!

> > -----
> >
> > I notice that ExecIndexesRequiringUpdates() does work on all indexes,
> > rather than just indexes relevant to this exact phase of checking. I
> > think that is a waste of time, so if we sort the indexes in order of
> > [hotblocking without expressions, hotblocking with expressions,
> > summarizing], then (with stored start/end indexes) we can save time in
> > cases where there are comparatively few of the types we're not going
> > to look at.
>
> If I plan on just having ExecIndexesRequiringUpdates() return a bool rather than a bitmap then sorting, or even just filtering the list of IndexInfo to only include indexes with expressions, makes sense. That way the only indexes in question in that function's loop will be those that may spoil the HOT path. When that list is length 0, we can skip the tests entirely.

Yes, exactly. Though I'm not sure it should hit that path if the
length is 0, as would mean we had expression indexes that matched the
updated columns, but somehow none are in the list?

> > You're extracting type info from the opclass, to use in
> > datum_image_eq(). Couldn't you instead use the index relation's
> > TupleDesc and its stored attribute information instead? That saves us
> > from having to do further catalog lookups during execution. I'm also
> > fairly sure that that information is supposed to be a more accurate
> > representation of attributes' expression output types than the
> > opclass' type information (though, they probably should match).
>
> I hadn't thought of that, I think it's a valid idea and I'll update accordingly. I think I understand what you are suggesting.

Thanks, that change was exactly what I meant.

> >
> > -----
> >
> > The operations applied in ExecIndexesRequiringUpdates partially
> > duplicate those done in index_unchanged_by_update. Can we (partially)
> > unify this, and pass which indexes were updated through the IndexInfo,
> > rather than the current bitmap?
>
> I think I do that now, feel free to say otherwise. When the expression is checked in ExecIndexesExpressionsWereNotUpdated() I set:
>
> /* Shortcut index_unchanged_by_update(), we know the answer. */ indexInfo->ii_CheckedUnchanged = true; indexInfo->ii_IndexUnchanged = !changed;
>
> That prevents duplicate effort in index_unchanged_by_update().

Exactly, yes.

> > -----
> >
> > I don't see a good reason to add IndexInfo to Relation, by way of
> > rd_indexInfoList. It seems like an ad-hoc way of passing data around,
> > and I don't think that's the right way.
>
> At one point I'd created a way to get this set via relcache, I will resurrect that approach but I'm not sure it is what you were hinting at.

AFAIK, we don't have IndexInfo in the relcaches currently. I'm very
hesitant to add an executor node (!) subtype to catalog caches, as
IndexInfos are also used to store temporary information about e.g.
index tuple insertion state, which (if IndexInfo is stored in
relcaches) would imply modifying relcache entries without any further
locks, and I'm not sure that's at all an OK thing to do.

> The current method avoids pulling a the lock on the index to build the list, but doing that once in relcache isn't horrible. Maybe you were suggesting using that opaque struct to pass around the list of IndexInfo? Let me know on this one if you had a specific idea. The swap I've made in v6 really just moves the IndexInfo list to a filtered list with a new name created in relcache.

My main concern is the storage of executor nodes(!) directly in the
relcache. I don't think we need that: We have relatively direct access
to the right IndexInfo** in ResultRelInfo->ri_IndexRelationInfo, which
I think should be sufficient for this purpose. (The relevant RRI is
available in table_tuple_update caller ExecUpdateAct; and could be
passed down by ExecSimpleRelationUpdate to simple_table_tuple_update,
covering both (current, core) callers of table_tuple_update). That
would then be passed down to the TableAM using an opaque pointer type;
for example (names, file locations, exact layout all bikesheddable):

/* tableam.h */
/* exact definition somewhere else, in e.g. an executor_internal.h */
typedef struct TU_UpdateIndexData TU_UpdateIndexData;

table_tuple_update(..., TU_UpdateIndexData *idxupdate, ...)

/* executor.h */

TU_UpdateIndexes
UpdateDetermineChangedIndexes(TU_UpdateIndexData *idxupdate,
TableTupleSlot *old, TableTupleSlot *new, bitmap *changed_atts, ...);

/* executor_internal.h */
struct TU_UpdateIndexData
{
EState estate;
IndexInfo **idxinfos;
...
}

-----

Looking at your later comments about RRI in patch v8, I think that
would solve and clean up the way that you currently get access to the
RRI and thus index set.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>
> Hello,
>
> I've rebased and updated the patch a bit. The biggest change is that the performance penalty measured with v1 of this patch is essentially gone in v10. The overhead was due to re-creating IndexInfo information unnecessarily, which I found existed in the estate. I've added a few fields in IndexInfo that are not populated by default but necessary when checking expression indexes, those fields are populated on demand and only once limiting their overhead.

This review is based on a light reading of patch v10. I have not read
all 90kB, and am unlikely to finish a full review soon:

> * assumes estate->es_result_relations[0] is the ResultRelInfo being updated

I'm not sure that's a valid assumption. I suspect it might be false in
cases of nested updates, like

$ UPDATE table1 SET value = other.value FROM (UPDATE table2 SET value
= 2 ) other WHERE other.id = table1.id;

If this table1 or table2 has expression indexes I suspect it may
result in this assertion failing (but I haven't spun up a server with
the patch).
Alternatively, please also check that it doesn't break if any of these
two tables is partitioned with multiple partitions (and/or has
expression indexes, etc.).

> * uses ri_IndexRelationInfo[] from within estate rather than re-creating it

As I mentioned above, I think it's safer to pass the known-correct RRI
(known by callers of table_tuple_update) down the stack.

> * augments IndexInfo only when needed for testing expressions and only once

ExecExpressionIndexesUpdated seems to always loop over all indexes,
always calling AttributeIndexInfo which always updates the fields in
the IndexInfo when the index has only !byval attributes (e.g. text,
json, or other such varlena types). You say it happens only once, have
I missed something?

I'm also somewhat concerned about the use of typecache lookups on
index->rd_opcintype[i], rather than using
TupleDescCompactAttr(index->rd_att, i); the latter of which I think
should be faster, especially when multiple wide indexes are scanned
with various column types. In hot loops of single-tuple update
statements I think this may make a few 0.1%pt difference - not a lot,
but worth considering.

> * only creates a local old/new TupleTableSlot when not present in estate

I'm not sure it's safe for us to touch that RRI's tupleslots.

> * retains existing summarized index HOT update logic

Great, thanks!

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

> On Mar 5, 2025, at 5:56 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> Sorry for the delay. This is a reply for the mail thread up to 17 Feb,
> so it might be very out-of-date by now, in which case sorry for the
> noise.

Never noise, always helpful.

> On Mon, 17 Feb 2025 at 20:54, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>> On Feb 15, 2025, at 5:49 AM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>>>
>>> In HEAD, we have a clear indication of which classes of indexes to
>>> update, with TU_UpdateIndexes. With this patch, we have to derive that
>>> from the (lack of) bits in the bitmap that might be output by the
>>> table_update procedure.
>>
>> Yes, but... that "clear indication" is lacking the ability to convey more detailed information. It doesn't tell you which summarizing indexes really need updating just that as a result of being on the HOT path all summarizing indexes require updates.
>
> Agreed that it's not great if you want to know about which indexes
> were meaningfully updated. I think that barring significant advances
> in performance of update checks, we can devise a way of transfering
> this info to the table_tuple_update caller once we get a need for more
> detailed information (e.g. this could be transfered through the
> IndexInfo* that's currently also used by index_unchanged_by_update).

One idea I had and tested a bit was to re-order the arrays of ri_IndexRelationInfo/Desc[] and then have a ri_NumModifiedIndices. This avoided allocation of a Bitmapset and was something downstream code could use or not depending on the requirements within that path. It may be, and I didn't check, that the order of indexes in that array has meaning in other contexts in the code so I put this aside. I think I'll keep focused as much as possible and consider that within the context of another patch later if needed.

>>> I think we can do with an additional parameter for which indexes would
>>> be updated (or store that info in the parameter which also will hold
>>> EState et al). I think it's cheaper that way, too - only when
>>> update_indexes could be TU_SUMMARIZING we might need the exact
>>> information for which indexes to insert new tuples into, and it only
>>> really needs to be sized to the number of summarizing indexes (usually
>>> small/nonexistent, but potentially huge).
>>
>> Okay, yes with this patch we need only concern ourselves with all, none, or some subset of summarizing as before. I'll work on the opaque parameter next iteration.
>
> Thanks!

I still haven't added the opaque parameter as suggested, but it's on my mind to give it a shot.

>>> -----
>>>
>>> I don't see a good reason to add IndexInfo to Relation, by way of
>>> rd_indexInfoList. It seems like an ad-hoc way of passing data around,
>>> and I don't think that's the right way.
>>
>> At one point I'd created a way to get this set via relcache, I will resurrect that approach but I'm not sure it is what you were hinting at.
>
> AFAIK, we don't have IndexInfo in the relcaches currently. I'm very
> hesitant to add an executor node (!) subtype to catalog caches, as
> IndexInfos are also used to store temporary information about e.g.
> index tuple insertion state, which (if IndexInfo is stored in
> relcaches) would imply modifying relcache entries without any further
> locks, and I'm not sure that's at all an OK thing to do.

This is gone in the v10 patch in favor of finding IndexInfo within the EState's ri_IndexRelationInfo[].

Thanks again for your continued support!

-greg

> On Mar 5, 2025, at 6:39 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>
> On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>>
>> Hello,
>>
>> I've rebased and updated the patch a bit. The biggest change is that the performance penalty measured with v1 of this patch is essentially gone in v10. The overhead was due to re-creating IndexInfo information unnecessarily, which I found existed in the estate. I've added a few fields in IndexInfo that are not populated by default but necessary when checking expression indexes, those fields are populated on demand and only once limiting their overhead.
>
> This review is based on a light reading of patch v10. I have not read
> all 90kB, and am unlikely to finish a full review soon:
>
>> * assumes estate->es_result_relations[0] is the ResultRelInfo being updated
>
> I'm not sure that's a valid assumption. I suspect it might be false in
> cases of nested updates, like
>
> $ UPDATE table1 SET value = other.value FROM (UPDATE table2 SET value
> = 2 ) other WHERE other.id = table1.id;
>
> If this table1 or table2 has expression indexes I suspect it may
> result in this assertion failing (but I haven't spun up a server with
> the patch).
> Alternatively, please also check that it doesn't break if any of these
> two tables is partitioned with multiple partitions (and/or has
> expression indexes, etc.).

Valid, and possible. I'll check and find a way to pass along the known-correct RRI index into that array.

>> * uses ri_IndexRelationInfo[] from within estate rather than re-creating it
>
> As I mentioned above, I think it's safer to pass the known-correct RRI
> (known by callers of table_tuple_update) down the stack.

I think passing the known-correct RRI index is the way to go as I need information from both ri_IndexRelationInfo/Desc[] arrays.

>> * augments IndexInfo only when needed for testing expressions and only once
>
> ExecExpressionIndexesUpdated seems to always loop over all indexes,
> always calling AttributeIndexInfo which always updates the fields in
> the IndexInfo when the index has only !byval attributes (e.g. text,
> json, or other such varlena types). You say it happens only once, have
> I missed something?

There's a test that avoids doing it more than once, but I'm going to rename this as BuildExpressionIndexInfo() and call it from ExecOpenIndices() if there are expressions on the index. I think that's cleaner and there's precedent for it in the form of BuildSpeculativeIndexInfo().

> I'm also somewhat concerned about the use of typecache lookups on
> index->rd_opcintype[i], rather than using
> TupleDescCompactAttr(index->rd_att, i); the latter of which I think
> should be faster, especially when multiple wide indexes are scanned
> with various column types. In hot loops of single-tuple update
> statements I think this may make a few 0.1%pt difference - not a lot,
> but worth considering.

I was just working on that. Good idea.

>> * only creates a local old/new TupleTableSlot when not present in estate
>
> I'm not sure it's safe for us to touch that RRI's tupleslots.

Me neither, that's why I mentioned it. It was my attempt to avoid the work to create/destroy temp slots over and over that led to that idea. It's working, but needs more thought.

>> * retains existing summarized index HOT update logic
>
> Great, thanks!
>
> Kind regards,
>
> Matthias van de Meent
> Neon (https://neon.tech)

I might widen this patch a bit to include support for testing equality of index tuples using custom operators when they exist for the index. In the use case I'm solving for we use a custom operator for equality that is not the same as a memcmp(). Do you have thoughts on that? It may be hard to accomplish this as the notion of an equality operator is specific to the index access method and not well-defined outside that AFAICT. If that's the case I'd have to augment the definition of an index access method to provide that information.

-greg

On Thu, 6 Mar 2025 at 13:40, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>
> > On Mar 5, 2025, at 6:39 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
> >
> > On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
> >> * augments IndexInfo only when needed for testing expressions and only once
> >
> > ExecExpressionIndexesUpdated seems to always loop over all indexes,
> > always calling AttributeIndexInfo which always updates the fields in
> > the IndexInfo when the index has only !byval attributes (e.g. text,
> > json, or other such varlena types). You say it happens only once, have
> > I missed something?
>
> There's a test that avoids doing it more than once, [...]

Is this that one?

+ if (indexInfo->ii_IndexAttrByVal)
+ return indexInfo;

I think that test doesn't work consistently: a bitmapset * is NULL
when no bits are set; and for some indexes no attribute will be byval,
thus failing this early-exit even after processing.

Another small issue with this approach is that it always calls and
tests in EEIU(), while it's quite likely we would do better if we
pre-processed _all_ indexes at once, so that we can have a path that
doesn't repeatedly get into EEIU only to exit immediately after. It'll
probably be hot enough to not matter much, but it's still cycles spent
on something that we can optimize for in code.

> >> * retains existing summarized index HOT update logic
> >
> > Great, thanks!
> >
> > Kind regards,
> >
> > Matthias van de Meent
> > Neon (https://neon.tech)
>
> I might widen this patch a bit to include support for testing equality of index tuples using custom operators when they exist for the index. In the use case I'm solving for we use a custom operator for equality that is not the same as a memcmp(). Do you have thoughts on that?

I don't think that's a very great idea. From a certain point of view,
you can see HOT as "deduplicating multiple tuple versions behind a
single TID". Btree doesn't support deduplication for types that can
have more than one representation of the same value so that e.g.
'0.0'::numeric and '0'::numeric are both displayed correctly, even
when they compare as equal according to certain equality operators.

So, I don't think that's worth investing time into right now. Maybe in
the future if there are new discoveries about what we can and cannot
deduplicate, but I don't think it should be part of an MVP or 1.0.

Kind regards,

Matthias van de Meent

Matthias,

Rebased patch attached.

Changes in v14:
* UpdateContext now the location I've stored estate, resultRelInfo, etc.
* Reuse the result from the predicate on the partial index.

-greg

> On Mar 7, 2025, at 5:47 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>
> On Thu, 6 Mar 2025 at 13:40, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>
>>
>>
>>> On Mar 5, 2025, at 6:39 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>>>
>>> On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>>>
>>>> * augments IndexInfo only when needed for testing expressions and only once
>>>
>>>
>>> ExecExpressionIndexesUpdated seems to always loop over all indexes,
>>> always calling AttributeIndexInfo which always updates the fields in
>>> the IndexInfo when the index has only !byval attributes (e.g. text,
>>> json, or other such varlena types). You say it happens only once, have
>>> I missed something?
>>
>>
>> There's a test that avoids doing it more than once, [...]
>
>
> Is this that one?
>
> + if (indexInfo->ii_IndexAttrByVal)
> + return indexInfo;
>
> I think that test doesn't work consistently: a bitmapset * is NULL
> when no bits are set; and for some indexes no attribute will be byval,
> thus failing this early-exit even after processing.
>
> Another small issue with this approach is that it always calls and
> tests in EEIU(), while it's quite likely we would do better if we
> pre-processed _all_ indexes at once, so that we can have a path that
> doesn't repeatedly get into EEIU only to exit immediately after. It'll
> probably be hot enough to not matter much, but it's still cycles spent
> on something that we can optimize for in code.

I've changed this a bit, now in ExecOpenIndices() when there are expressions or predicates I augment the IndexInfo with information necessary to perform the tests in EEIU(). I've debated adding another bool to ExecOpenIndices() to indicate that we're opening indexes for the purpose of an update to avoid building that information in cases where we are not. Similar to the bool `speculative` on ExecOpenIndices() today. Thoughts?

>> I might widen this patch a bit to include support for testing equality of index tuples using custom operators when they exist for the index. In the use case I'm solving for we use a custom operator for equality that is not the same as a memcmp(). Do you have thoughts on that?
>
>
> I don't think that's a very great idea. From a certain point of view,
> you can see HOT as "deduplicating multiple tuple versions behind a
> single TID". Btree doesn't support deduplication for types that can
> have more than one representation of the same value so that e.g.
> '0.0'::numeric and '0'::numeric are both displayed correctly, even
> when they compare as equal according to certain equality operators.

Interesting, good point. Seems like it would require a new index AM function:
bool indexed_tuple_would_change()

I'll drop this for now, it seems out of scope for this patch set.

Apologies for the noise, I overlooked a compiler warning.

fixed.

-greg

> On Mar 25, 2025, at 7:47 AM, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>
> Matthias,
>
> Rebased patch attached.
>
> Changes in v14:
> * UpdateContext now the location I've stored estate, resultRelInfo, etc.
> * Reuse the result from the predicate on the partial index.
>
> -greg
>
>
>
>> On Mar 7, 2025, at 5:47 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>>
>> On Thu, 6 Mar 2025 at 13:40, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>>
>>
>>>
>>>
>>>
>>>> On Mar 5, 2025, at 6:39 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>>>>
>>>> On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>>>>
>>>>
>>>>> * augments IndexInfo only when needed for testing expressions and only once
>>>>
>>>>
>>>>
>>>> ExecExpressionIndexesUpdated seems to always loop over all indexes,
>>>> always calling AttributeIndexInfo which always updates the fields in
>>>> the IndexInfo when the index has only !byval attributes (e.g. text,
>>>> json, or other such varlena types). You say it happens only once, have
>>>> I missed something?
>>>
>>>
>>>
>>> There's a test that avoids doing it more than once, [...]
>>
>>
>>
>> Is this that one?
>>
>> + if (indexInfo->ii_IndexAttrByVal)
>> + return indexInfo;
>>
>> I think that test doesn't work consistently: a bitmapset * is NULL
>> when no bits are set; and for some indexes no attribute will be byval,
>> thus failing this early-exit even after processing.
>>
>> Another small issue with this approach is that it always calls and
>> tests in EEIU(), while it's quite likely we would do better if we
>> pre-processed _all_ indexes at once, so that we can have a path that
>> doesn't repeatedly get into EEIU only to exit immediately after. It'll
>> probably be hot enough to not matter much, but it's still cycles spent
>> on something that we can optimize for in code.
>
>
> I've changed this a bit, now in ExecOpenIndices() when there are expressions or predicates I augment the IndexInfo with information necessary to perform the tests in EEIU(). I've debated adding another bool to ExecOpenIndices() to indicate that we're opening indexes for the purpose of an update to avoid building that information in cases where we are not. Similar to the bool `speculative` on ExecOpenIndices() today. Thoughts?
>
>
>>> I might widen this patch a bit to include support for testing equality of index tuples using custom operators when they exist for the index. In the use case I'm solving for we use a custom operator for equality that is not the same as a memcmp(). Do you have thoughts on that?
>>
>>
>>
>> I don't think that's a very great idea. From a certain point of view,
>> you can see HOT as "deduplicating multiple tuple versions behind a
>> single TID". Btree doesn't support deduplication for types that can
>> have more than one representation of the same value so that e.g.
>> '0.0'::numeric and '0'::numeric are both displayed correctly, even
>> when they compare as equal according to certain equality operators.
>
>
> Interesting, good point. Seems like it would require a new index AM function:
> bool indexed_tuple_would_change()
>
> I'll drop this for now, it seems out of scope for this patch set.
>
>
>
> <v14-0001-Expand-HOT-update-path-to-include-expression-and.patch>

I'm working again on expanding the conditions under which HOT updates are allowable, it is still a work-in-progress at this point. It has been a while since my last update to this patch set so I'll refresh everyone's memories (including myself) with an overview. Apologies in advance for a long email...

The goal is to allow HOT updates under two new conditions:
* when an indexed expression has not changed
* when possible for a partial index

Expression Indexes and HOT updates
==========================================================
Indexes on expressions such as:
CREATE INDEX test1_lower_col1_idx ON test1 (lower(col1));
CREATE INDEX people_names ON people ((first_name || ' ' || last_name));
CREATE INDEX names ON people((docs->>'user'));

These are not currently candidates for HOT updates because the attributes they reference are added to the hotblockingattrs bitmapset that is compared for overlap with the modified_attrs bitmap in heap_update() as created by HeapDetermineColumnsInfo(). Logically this exclusion makes sense because expressions used to form indexes can only contain references to functions that are IMMUTABLE. This then allows for a simple and efficient method for determining if the update is a candidate for the HOT path, a quick check for overlap of two bitmapsets and you have an answer.

But there is a common case that is overlooked. The third example above is an expression index that references a JSONB attribute 'docs' field 'name'. Imagine a simple bit of JSON stored in the 'docs' JSONB column changed by an UPDATE from:
{ "user": "scott", "password": "tiger" }
to:
{ "user": "scott", "password": "$ecret" }
this will not use the HOT path in today's code because the attribute for 'docs' will be referenced in the UPDATE statement and that attribute's content did change so it will be in the modified_attrs which will overlap with the hotblockingattrs set. The result is that it is not possible to use the HOT update path in heap_update() if there is an indexed JSONB column in that statement. This has a huge impact on performance and bloat. It's not hard to see that the index doesn't require an update, the portion of the document used to form the index tuple isn't changing.

This patch addressed this problem by evaluating the expression on the index using the current and new tuple and then comparing them. If there is no change to the index tuple then the HOT path is still an option for the update whereas before this was not the case.

There is at least one major challenge to this approach left to solve, it invokes UDFs while holding BUFFER_LOCK_EXCLUSIVE which we learn from heap_attr_equals() "we cannot safely invoke user-defined functions while holding exclusive buffer lock."

So, for performance an safety reasons it makes sense to try to limit the expressions that can be evaluated to those that can't self-deadlock trying to pin the same buffer and are roughly constant time and fast so we don't hold that lock for very long. Additionally it would be nice to ensure that we really have to do the evaluation in the first place.

I looked for ways to limit the types of expressions to evaluate so as to only take this approach when necessary, but I wasn't able to identify a good way forward. The problem is that it is possible to author a UDF similar to the json/jsonb getter functions json_extract_path() that extract a portion of a datum that is then used when creating an index. There is currently no demarcation on functions similar to "IMMUTABLE" (for example "EXTRACTOR") that would indicate that's what the UDF does nor does it strictly fit within the category of volatility as I don't think of this as information for the optimizer to use. Ideally you'd only want to evaluate the expression when any function in the expression is known to extract a portion of the datum. Were that the case you could require for HOT updates in the presence of expression indexes that at least one function in the expression be an "EXTRACTOR". I might still do this depending on reaction to it here, I'm seeking ideas.

Another idea was to change up the strategy a bit and not check expressions at all, but rather find a method to keep the attribute out of the modified_attrs set to begin with. This would mean that HeapDeterminColumnsInfo() would have to change, any maybe that's a good idea to try out too. It could even morph into something that can test equality using type-specific equality tests supporting custom types and invoke index access method-specific path extraction functions. In some ways this feels more "correct" to me in that HeapDeterminColumnsInfo()'s goal is to reduce the set of attributes in play from the interesting set to the modified set and if what is interesting is an index on a field within a document that isn't modified then it shouldn't be in the set. This comes with challenges related to that comment mentioned earlier in heap_attr_equals() and the fact that index access methods don't have a way to supply their equality op or provide a way to call a path operator should one exist. But the benefit would be that we expand HOT updates a bit further than my initial goals.

I'm not wedded to the use of a reloption to disable expression checks, nor do I love the name I choose ("expression_checks"). I'm interested to hear if I should simple remove this.

Another benefit to changing HeapDeterminColumnsInfo() is that it is what provides information to the PHOT patch during heap pruning to record which attributes were changed. As it stands now, this patch and the PHOT patch when combined result in some undesirable index scan results.

Partial Indexes and HOT updates
==========================================================

Partial indexes are a problem when it comes to HOT updates because they are part of the modified_attrs set returned by HeapDeterminColumnsInfo() even in cases where the value being set in the UPDATE does not satisfy the partial index predicate. Take for example:

CREATE TABLE example (a int, b int);
CREATE INDEX idx_a ON example(a);
CREATE INDEX idx_b ON example(b) WHERE b > 100;
INSERT INTO example (a, b) VALUES (1, 50);
UPDATE example SET b = 60 WHERE a = 1;

What happened? Was the update HOT? No. Why? Because the predicate isn't taken into account at the time the decision for HOT (or not) is made, that happens later during ExecInsertIndexTuples(). Today this update would trigger new index entries for both idx_a and idx_b when neither were required.

This patch changes this behavior by evaluating the predicate for the partial index earlier during heap_update(). When both the existing and updated tuples fall outside of the predicate the HOT update path is still an option. To limit scope creep I've not implemented the case when the existing was within the index's predicate but now the updated falls outside the predicate because that would require adding an optional delete function to the index access method (something that I think would be generally useful at some point). When that happens this patch does not allow a HOT update forcing the new tuple in the heap to not be redirected so later on index scan the CTID from the index will be ignored.

Summary
==========================================================

Attached find v17 of this patch rebased to this morning's (EDT) HEAD passing make world and formatted.

any and all feedback welcome.

-greg

On Jul 2 2025, at 2:10 pm, Greg Burd <greg(at)burd(dot)me> wrote:

> The goal is to allow HOT updates under two new conditions:
> * when an indexed expression has not changed
> * when possible for a partial index

This is still true. :)

This patch has languished for nearly 2+ months at v17. Why? Primarily
due to feedback that although the idea had merit, there was a critical
flaw in the approach that made it a non-starter. The flaw was that I'd
been executing expressions while holding both the pin and a lock on the
buffer, which is not a great idea (self dead lock, etc.). This was
pointed out to me (thanks Robert Haas!) and so I needed to re-think my approach.

I put the patch aside for a while, then this past week at PGConf.dev/NYC
I heard interest from a few people (Jeff Davis, Nathan Bossart) who
encouraged me to move the code executing the expressions to just before
acquiring the lock but after pinning the buffer. The theory being that
my new code using the old/new tts to form and test the index tuples
resulting from executing expressions was using the resultsRelInfo struct
created during plan execution, not the information found on the page,
and so was safe without the lock.

This proved tricky because I had been using the modified_attrs and
expr_attrs as a test to avoid exercising expressions when unnecessary.
Calling HeapDetermineColumnsInfo() outside the buffer lock to get
modified_attrs proved to be a problem as it examines an oldtup that is
cobbled together from the elements on the page, requiring the lock I was
trying to avoid.

After reviewing how updates work in the executor, I discovered that
during execution the new tuple slot is populated with the information
from ExecBuildUpdateProjection() and the old tuple, but that most
importantly for this use case that function created a bitmap of the
modified columns (the columns specified in the update). This bitmap
isn't the same as the one produced by HeapDetermineColumnsInfo() as the
latter excludes attributes that are not changed after testing equality
with the helper function heap_attr_equals() where as the former will
include attributes that appear in the update but are the same value as
before. This, happily, is immaterial for the purposes of my function
ExecExprIndexesRequireUpdates() which simply needs to check to see if
index tuples generated are unchanged. So I had all I needed to run the
checks ahead of acquiring the lock on the buffer.

So, this led to v18 (attached), which passes test wold including a
number of new tests for the various corner cases relative to HOT updates
for expressions.

There is much room for improvement, and your suggestions are welcome.

I'll find time to quantify the benefit of this patch for the targeted
use cases and to ensure that all other cases see no regressions.

I need to review the tests I've added to ensure that they are the
minimal set required, that they communicate effectively their purpose,
etc. For now, it's more shotgun than scalpel, .... I'll get to it.

I added a reloption "expression_checks" to disable this new code path.
Good idea or bad precedent?

In execIndexing I special case for IsolationIsSerializable() and I can't
remember why now but I do recall one isolation test failing... I'll
check on this and get back to the thread. Or maybe you know why that

From small things like naming to larger questions like hijacking the
modified columns bitmapset for use later in the heap, I need feedback
and guidance.

I'm using UpdateContext for estate/resultRelInfo and I've added to that
lockmode, these change the table am API a tad, I'm open to better ideas
that accomplish the same.

I'd like not to build, then rebuild index tuples for these expressions
but I can't think of a way to do that without a palloc(), this is
avoided today.

Example:

CREATE TABLE t (
x INT GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
y JSONB
);
CREATE INDEX t_age_idx ON t (((y->>'age')::int));

INSERT INTO t (y)
VALUES ('{"name": "John", "age": 30, "city": "New York"}');

-- Update an indexed field in the JSONB document, should not be HOT
UPDATE t
SET y = jsonb_set(y, '{age}', '31')
WHERE x = 1;

SELECT pg_stat_force_next_flush();
SELECT relname, n_tup_upd, n_tup_hot_upd FROM pg_stat_user_tables
WHERE relname = 't';

-- Update a non-indexed field in the JSONB document, new HOT update
UPDATE t
SET y = jsonb_set(y, '{city}', '"Boston"')
WHERE x = 1;

SELECT pg_stat_force_next_flush();
SELECT relname, n_tup_upd, n_tup_hot_upd FROM pg_stat_user_tables
WHERE relname = 't';

This patch is far from "commit ready", but it does accomplish the
$subject and pass tests.

I look forward to any/all constructive feedback.

best.

-greg

On Tue, Oct 07, 2025 at 05:36:11PM -0400, Greg Burd wrote:
> I put the patch aside for a while, then this past week at PGConf.dev/NYC
> I heard interest from a few people (Jeff Davis, Nathan Bossart) who
> encouraged me to move the code executing the expressions to just before
> acquiring the lock but after pinning the buffer. The theory being that
> my new code using the old/new tts to form and test the index tuples
> resulting from executing expressions was using the resultsRelInfo struct
> created during plan execution, not the information found on the page,
> and so was safe without the lock.

An open question (at least from me) is whether this is safe. I'm not
familiar enough with this area of code yet to confidently determine that.

> After reviewing how updates work in the executor, I discovered that
> during execution the new tuple slot is populated with the information
> from ExecBuildUpdateProjection() and the old tuple, but that most
> importantly for this use case that function created a bitmap of the
> modified columns (the columns specified in the update). This bitmap
> isn't the same as the one produced by HeapDetermineColumnsInfo() as the
> latter excludes attributes that are not changed after testing equality
> with the helper function heap_attr_equals() where as the former will
> include attributes that appear in the update but are the same value as
> before. This, happily, is immaterial for the purposes of my function
> ExecExprIndexesRequireUpdates() which simply needs to check to see if
> index tuples generated are unchanged. So I had all I needed to run the
> checks ahead of acquiring the lock on the buffer.

Nice.

> There is much room for improvement, and your suggestions are welcome.

A general and predictable suggestion is to find ways to break this into
smaller pieces. As-is, this patch would take me an enormous amount of time
to review in any depth. If we can break off some smaller pieces that we
can scrutinize and commit independently, we can start making forward
progress sooner. The UpdateContext and reloption stuff are examples of
things that might be possible to split into independent patches.

> I'll find time to quantify the benefit of this patch for the targeted
> use cases and to ensure that all other cases see no regressions.

Looking forward to these results. This should also help us decide whether
to set expression_checks by default.

> I added a reloption "expression_checks" to disable this new code path.
> Good idea or bad precedent?

If there are cases where the added overhead outweighs the benefits (which
seems like it must be true some of the time), then I think we must have a
way to opt-out (or maybe even opt-in). In fact, I'd advise adding a GUC to
complement the reloption so that users can configure it at higher levels.

> In execIndexing I special case for IsolationIsSerializable() and I can't
> remember why now but I do recall one isolation test failing... I'll
> check on this and get back to the thread. Or maybe you know why that

I didn't follow this.

> I'd like not to build, then rebuild index tuples for these expressions
> but I can't think of a way to do that without a palloc(), this is
> avoided today.

Is the avoidance of palloc() a strict rule? Is this discussed in the code
anywhere?

--
nathan

On Wed, 2025-10-08 at 15:48 -0500, Nathan Bossart wrote:
> > The theory being that
> > my new code using the old/new tts to form and test the index tuples
> > resulting from executing expressions was using the resultsRelInfo
> > struct
> > created during plan execution, not the information found on the
> > page,
> > and so was safe without the lock.
>
> An open question (at least from me) is whether this is safe.  I'm not
> familiar enough with this area of code yet to confidently determine
> that.

The optimization requires that the expression evaluates to the same
thing on the old and new tuples. That determination doesn't have
anything to do with a lock on the buffer, so long as the old tuple
isn't pruned away or something. And clearly it won't be pruned, because
we're in the process of updating it, so we have a snapshot that can see
it.

There might be subtleties in other parts of the proposal, but the above
determination can be made safely without a buffer lock.

>
> > I added a reloption "expression_checks" to disable this new code
> > path.
> > Good idea or bad precedent?
>
> If there are cases where the added overhead outweighs the benefits
> (which
> seems like it must be true some of the time), then I think we must
> have a
> way to opt-out (or maybe even opt-in).  In fact, I'd advise adding a
> GUC to
> complement the reloption so that users can configure it at higher
> levels.

I'll push back against this. For now I'm fine with developer options to
make testing easier, but we should find a way to make this work well
without tuning.

Regards,
Jeff Davis

On Tue, 2025-10-07 at 17:36 -0400, Greg Burd wrote:
> After reviewing how updates work in the executor, I discovered that
> during execution the new tuple slot is populated with the information
> from ExecBuildUpdateProjection() and the old tuple, but that most
> importantly for this use case that function created a bitmap of the
> modified columns (the columns specified in the update).  This bitmap
> isn't the same as the one produced by HeapDetermineColumnsInfo() as
> the
> latter excludes attributes that are not changed after testing
> equality
> with the helper function heap_attr_equals() where as the former will
> include attributes that appear in the update but are the same value
> as
> before.  This, happily, is immaterial for the purposes of my function
> ExecExprIndexesRequireUpdates() which simply needs to check to see if
> index tuples generated are unchanged.  So I had all I needed to run
> the
> checks ahead of acquiring the lock on the buffer.

You're still calling ExecExprIndexesRequireUpdates() from within
heap_update(). Can't you do that inside of ExecUpdatePrologue() or
thereabouts?

Regards,
Jeff Davis

> On Oct 8, 2025, at 4:48 PM, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> On Tue, Oct 07, 2025 at 05:36:11PM -0400, Greg Burd wrote:
>> I put the patch aside for a while, then this past week at PGConf.dev/NYC
>> I heard interest from a few people (Jeff Davis, Nathan Bossart) who
>> encouraged me to move the code executing the expressions to just before
>> acquiring the lock but after pinning the buffer. The theory being that
>> my new code using the old/new tts to form and test the index tuples
>> resulting from executing expressions was using the resultsRelInfo struct
>> created during plan execution, not the information found on the page,
>> and so was safe without the lock.

Thanks for taking a look Nathan.
>
> An open question (at least from me) is whether this is safe. I'm not
> familiar enough with this area of code yet to confidently determine that.

My read is that it is safe because we're testing the content of two
TupleTableSlots both formed in the executor. The function uses only
that information and doesn't reference data on the page at all.

>> After reviewing how updates work in the executor, I discovered that
>> during execution the new tuple slot is populated with the information
>> from ExecBuildUpdateProjection() and the old tuple, but that most
>> importantly for this use case that function created a bitmap of the
>> modified columns (the columns specified in the update). This bitmap
>> isn't the same as the one produced by HeapDetermineColumnsInfo() as the
>> latter excludes attributes that are not changed after testing equality
>> with the helper function heap_attr_equals() where as the former will
>> include attributes that appear in the update but are the same value as
>> before. This, happily, is immaterial for the purposes of my function
>> ExecExprIndexesRequireUpdates() which simply needs to check to see if
>> index tuples generated are unchanged. So I had all I needed to run the
>> checks ahead of acquiring the lock on the buffer.
>
> Nice.

Handy indeed. I'm not at all a fan of increasing the size of a plan node
but it's only by a little... and for a good cause.

>> There is much room for improvement, and your suggestions are welcome.
>
> A general and predictable suggestion is to find ways to break this into
> smaller pieces. As-is, this patch would take me an enormous amount of time
> to review in any depth. If we can break off some smaller pieces that we
> can scrutinize and commit independently, we can start making forward
> progress sooner. The UpdateContext and reloption stuff are examples of
> things that might be possible to split into independent patches.

Fair, I'll try.

>> I'll find time to quantify the benefit of this patch for the targeted
>> use cases and to ensure that all other cases see no regressions.
>
> Looking forward to these results. This should also help us decide whether
> to set expression_checks by default.

In past my test results were very positive for cases where this helped avoid
heap and index bloat and almost immeasurably small even for cases where we
were doing the work to test but ultimately unable to take the HOT path.

This will require new tests as the code has changed quite a bit.

>> I added a reloption "expression_checks" to disable this new code path.
>> Good idea or bad precedent?
>
> If there are cases where the added overhead outweighs the benefits (which
> seems like it must be true some of the time), then I think we must have a
> way to opt-out (or maybe even opt-in). In fact, I'd advise adding a GUC to
> complement the reloption so that users can configure it at higher levels.

This evolved from a GUC to a reloption and I'd rather it go away entirely.
I hear your concern, but I've yet to measure a perceptable impact and I'll
try hard to keep it that way as this matures. Assuming that's the case,
I'd like to eliminate the potentially confusing tuning knob.

>> In execIndexing I special case for IsolationIsSerializable() and I can't
>> remember why now but I do recall one isolation test failing... I'll
>> check on this and get back to the thread. Or maybe you know why that
>
> I didn't follow this.

More later if/when I can reproduce it and understand it better myself.

>> I'd like not to build, then rebuild index tuples for these expressions
>> but I can't think of a way to do that without a palloc(), this is
>> avoided today.
>
> Is the avoidance of palloc() a strict rule? Is this discussed in the code
> anywhere?

Not that I know of, just my paranoid self trying to avoid it on a path that
didn't have it before.

> --
> nathan

best.

-greg

> On Oct 9, 2025, at 3:08 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>
> On Wed, 2025-10-08 at 15:48 -0500, Nathan Bossart wrote:
>>> The theory being that
>>> my new code using the old/new tts to form and test the index tuples
>>> resulting from executing expressions was using the resultsRelInfo
>>> struct
>>> created during plan execution, not the information found on the
>>> page,
>>> and so was safe without the lock.
>>
>> An open question (at least from me) is whether this is safe. I'm not
>> familiar enough with this area of code yet to confidently determine
>> that.

Hey Jeff,

Thanks for the nudge at PGConf.dev in NYC and for the follow-up here.

> The optimization requires that the expression evaluates to the same
> thing on the old and new tuples. That determination doesn't have
> anything to do with a lock on the buffer, so long as the old tuple
> isn't pruned away or something. And clearly it won't be pruned, because
> we're in the process of updating it, so we have a snapshot that can see
> it.

Right, I test that the expression on the index evaluates to the same
value when forming an index tuple for old/new slots.

> There might be subtleties in other parts of the proposal, but the above
> determination can be made safely without a buffer lock.
>
>>
>>> I added a reloption "expression_checks" to disable this new code
>>> path.
>>> Good idea or bad precedent?
>>
>> If there are cases where the added overhead outweighs the benefits
>> (which
>> seems like it must be true some of the time), then I think we must
>> have a
>> way to opt-out (or maybe even opt-in). In fact, I'd advise adding a
>> GUC to
>> complement the reloption so that users can configure it at higher
>> levels.
>
> I'll push back against this. For now I'm fine with developer options to
> make testing easier, but we should find a way to make this work well
> without tuning.

I'm aligned with this, the reloption evolved from a GUC and I'm more of
the opinion that neither should exist and that the overhead of this be
minimized and so require no tuning or consideration by the end user.

best.

-greg

> Regards,
> Jeff Davis

> On Oct 9, 2025, at 3:27 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>
> On Tue, 2025-10-07 at 17:36 -0400, Greg Burd wrote:
>> After reviewing how updates work in the executor, I discovered that
>> during execution the new tuple slot is populated with the information
>> from ExecBuildUpdateProjection() and the old tuple, but that most
>> importantly for this use case that function created a bitmap of the
>> modified columns (the columns specified in the update). This bitmap
>> isn't the same as the one produced by HeapDetermineColumnsInfo() as
>> the
>> latter excludes attributes that are not changed after testing
>> equality
>> with the helper function heap_attr_equals() where as the former will
>> include attributes that appear in the update but are the same value
>> as
>> before. This, happily, is immaterial for the purposes of my function
>> ExecExprIndexesRequireUpdates() which simply needs to check to see if
>> index tuples generated are unchanged. So I had all I needed to run
>> the
>> checks ahead of acquiring the lock on the buffer.
>
> You're still calling ExecExprIndexesRequireUpdates() from within
> heap_update(). Can't you do that inside of ExecUpdatePrologue() or
> thereabouts?

Hey Jeff,

I'm trying to knit this into the executor layer but that is tricky because
the concept of HOT is very heap-specific, so the executor should be
ignorant of the heap's specific needs (right?). Right now, I am considering
adding a step in ExecUpdatePrologue() just after opening the indexes.

The idea I'm toying with is to have a new function on all TupleTableSlots
that examines the before/after slots for an update and the set of updated
attributes and returns a Bitmapset of the changed attributes that overlap
with indexes and so should trigger index updates in ExecUpdateEpilogue().

That way for heap we'd have something like:
Bitmapset *tts_heap_getidxattr(ResultRelInfo *info,
TupleTableSlot *updated,
TupleTableSlot *existing,
Bitmapset *updated_attrs)
{
some combo of HeapDeterminColumnsInfo() and
ExecExprIndexesRequireUpdates()

returns the set of indexed attrs that this update changed
}

So, attributes only referenced by expressions where the expression
produces the same value for the updated and existing slots would be
removed from the set.

Interestingly, summarizing indexes that don't overlap with changed
attributes won't be updated (and that's a good thing).

Problem is we're not yet accounting for what is about to happen in
ExecUpdateAct() when calling into the heap_update(). That's where
heap tries to fit the new tuple onto the same page. That might be
possible with large tuples thanks to TOAST, it's impossible to say
before getting into this function with the page locked.

So, for updates we include the modified_attrs in the UpdateContext
which is available to heap_update(). If the heap code decides to
go HOT, great unset all attributes in the modified_attrs except any
that are only summarizing. If the heap can't go HOT, fine, add
the indexed attrs back into modified_attrs which should trigger all
indexes to be updated.

This gets rid of TU_UpdateIndexes enum and allows only modified
summarizing indexes to be updated on the HOT path. Two additional
benefits IMO.

at least, that's what I'm trying out now,

-greg

> Regards,
> Jeff Davis

On Tue, 2025-10-14 at 13:46 -0400, Greg Burd wrote:
> I'm trying to knit this into the executor layer but that is tricky
> because
> the concept of HOT is very heap-specific, so the executor should be
> ignorant of the heap's specific needs (right?).

It's wrong for the executor to say "do a HOT update" but it's OK for
the executor to say "this is the set of indexes that might have a new
key after the update". If that set is empty, then the heap can choose
to do a HOT update.

> Right now, I am considering
> adding a step in ExecUpdatePrologue() just after opening the indexes.

Seems like a reasonable place.

> The idea I'm toying with is to have a new function on all
> TupleTableSlots...
>
> That way for heap we'd have something like:
> Bitmapset *tts_heap_getidxattr(ResultRelInfo *info,
> TupleTableSlot *updated,
> TupleTableSlot *existing,
> Bitmapset *updated_attrs)
> {
> some combo of HeapDeterminColumnsInfo() and
> ExecExprIndexesRequireUpdates()
>
> returns the set of indexed attrs that this update changed
> }

Why is this a generic method for all slots? Do we need to reuse it
somewhere else? I would have expected just a static method in
nodeModifyTable.c that does just what's needed.

And to be precise, it's the set of indexed attrs where the update might
have created a new key, right? The whole point is that we don't care if
the indexed attr has been changed, so long as it doesn't create a new
index key.

> Interestingly, summarizing indexes that don't overlap with changed
> attributes won't be updated (and that's a good thing).

Nice.

> Problem is we're not yet accounting for what is about to happen in
> ExecUpdateAct() when calling into the heap_update().  That's where
> heap tries to fit the new tuple onto the same page.  That might be
> possible with large tuples thanks to TOAST, it's impossible to say
> before getting into this function with the page locked.

I don't see why that's a problem. The executor can pass down the list
of indexed attrs that might have created new keys after the update,
then heap_update uses that information (along with other factors, like
if it fits on the same page) to determine whether to perform a HOT
update or not.

> So, for updates we include the modified_attrs in the UpdateContext
> which is available to heap_update().

It doesn't look like UpdateContext is currently available to
heap_update(). We might need to change the signature. But I think it's
fine to change the signature if it results in a cleaner design --
tableam extensions often need source changes when new major versions
are released.

>   If the heap code decides to
> go HOT, great unset all attributes in the modified_attrs except any
> that are only summarizing.  If the heap can't go HOT, fine, add
> the indexed attrs back into modified_attrs which should trigger all
> indexes to be updated.

IIUC, that sounds like a good plan.

> This gets rid of TU_UpdateIndexes enum and allows only modified
> summarizing indexes to be updated on the HOT path.  Two additional
> benefits IMO.

I'm not sure that I understand, but I'll look at that after we sort out
some of the other details.

Regards,
Jeff Davis

Hello again.

This idea started over a year ago for me while working on a project that
used JSONB and had horrible "bloat" with update times that were not
fantastic. The root cause, expression indexes prevent HOT updates and
all indexes on JSONB are by definition, expressions. That's the backstory.

The idea for the solution came from a patch [1] applied [2], then later
reverted [3], that basically evaluated the before/after tuple
expressions in heap_update() at the point where newbuff == buffer just
before deciding to use_hot_update or not. When the evaluated
expressions produced equal results using a binary comparison the index
didn't need to be updated. While this approach sorta worked, but it was
reverted for a few reasons, here's Tom's summary: "The problem here is
that [the code that checks for equality] thinks that the type of the
index column is identical to the type of the source datum for it, which
is not true for any opclass making use of the opckeytype property. [4]"

Still, expanding the domain of what might go HOT seems like a good goal
to me and it was at the heart of the issues I was facing on the project
using JSONB, so I kept at it.

The patches I've sent on this thread have evolved from that first idea.
First they addressed the specific issue raised by Tom. Then they
expanded to include partial indexes as well. This worked, and I helped
to ship a fork of Postgres used this approach and solved customer issues
with the JSONB use case. Customer experience was better, no unnecessary
index updates when indexed data wasn't modified meant no more heap/index
bloat and faster update times. Vacuum could finally keep up and
performance and storage overhead was much better.

For this patch set v1 through v17 were essentially work that
refined/reworked that original approach, but there was a serious flaw
that I didn't fully appreciate until around v16/17. I was evaluating
the expressions while holding a lock on the buffer page which a) expands
the time the lock is held, and b) opens the door to self-deadlock. No bueno.

Then there was v18, a quick work-around for that. I moved the call that
invokes the executor to the beginning of heap_update() before taking the
lock on the page. To do this I had to find the set of updated
attributes, which I discovered was available in the executor as the
updated tuple is created. This was a viable fix, but didn't really go
far enough and was a bit hackish IMO. Jeff Davis and others challenged
me to move the work to identify what's changed into the executor and
clean it up. I'm a sucker for a challenge.

More generally, this idea made sense. While Postgres has many places
where logic is tightly coupled to the way the heap works this didn't
have to be one. To make this work I needed the update path in the
executor to be interested in:

a) knowing what columns were specified in the UPDATE statement and those
impacted by before/after triggers,
b) reducing that set to those attributes known to be both indexed and to
have changed value,
c) finding which of those (and possibly other) attributes that force new
index updates.

Why? We'll, that code already exists in a few places and in some cases
is replicated; for (a) there is ExecGetAllUpdatedCols(), for (b)
HeapDetermineColumnsInfo() and index_unchanged_by_update().

An interesting thing to note is that HeapDetermineColumnsInfo() might
return a set that includes columns not returned by
ExecGetAllUpdatedCols() because HeapDetermineColumnsInfo() iterates over
all indexed attributes looking for changes and that might find an
indexed attribute that was changed by heap_modify_tuple() but not
knowable by ExecGetAllUpdatedCols(). This happens in tsvector code, see
tsvector_op.c tsvector_update_trigger() where if (update_needed)
heap_modify_tuple_by_cols(). That column isn't known to ExecGetAllUpdatedCols().

HeapDetermineColumnsInfo() is also critical when modifying catalog
tuples. Catalog tuples are modified using either Form/GETSTRUCT or
values/nulls/replaces then using heap_modify_tuple() and calling into
CatalogTupleUpdate() which calls simple_heap_update() that calls
heap_update() where we find HeapDetermineColumnsInfo(). The interesting
thing here is that when modifying catalog tuples there is knowledge of
what attributes are changed, but that knowledge isn't preserved and
passed into CatalogTupleUpdate(), rather it is re-discovered in
HeapDetermineColumnsInfo(). That's how catalog tuples are able to take
the HOT path, they re-use that same logic. There is a fix for that [5]
too (and I really hope that lands in master ASAP), but that's not the
subject of this thread.

HeapDetermineColumnsInfo() also helps inform a few other decisions in
heap_update(), but these have to happen after taking the buffer lock and
are very heap-specific, namely:

1. Do either the replica identity key attributes overlap with the
modified index attributes or do they need to be stored externally, this
is passed on to ExtractReplicaIdentity() to find out if we must augment
the WAL log or not.

2. Are there any modified indexed attributes that intersect with the
primary keys of the relation, if not lower the lock mode to enable
multixact to work.

HeapDetermineColumnsInfo() also takes a pragmatic approach to testing
for equality when looking for modified indexed attributes, it uses
datumIsEqual() which boils down to a simple memcmp() of the before/after
HeapTuple datum. This is fine in most cases, but limits the scope of
what can be HOT.

Interestingly, this requirement of binary equality has leaked into other
parts of the code, namely nbtree's deduplication of TIDs on page split.
That code uses binary equality as well. A nbtree index with collation
for case insensitive must store both "A" and "a" despite those being
type-equal because they are not binary equivalent. More on this later.

At this point, I had my sights set on HeapDetermineColumnsInfo(). I
felt that what it was doing should move into the executor, well as much
of that work as possible, and outside of the buffer lock. This would
also open the door for removal of redundant code. My thought was that
the table AM update API should have an additional argument, the
"modified indexed attributes" or "mix_attrs", passed in.

So, here we are at the door of v19... let's begin.

0001 - Reorganize heap update logic

This is preparatory work for the larger goal in that heap_update()
serves two masters: CatalogTupleUpdate()/simple_heap_update() and
heap_tuple_update() and in reality, they were different but needed most
of the same logic that happens at the start of heap_update(). This
patch splits that logic out and moves it into heap_tuple_update() and
simple_heap_update(). Functionally nothing changes. That's the meat of
this patch.Reorganize heap update logic

0002 - Track changed indexed columns in the executor during UPDATEs

This is the first core set of changes, it doesn't expand HOT updates but
it does restructure where HeapDetermineColumnsInfo()'s core work happens.

A new function ExecCheckIndexedAttrsForChanges() in nodeModifyTable.c is
now responsible for checking for changes between Datum in the old/new
TupleTableSlots. This is different from before in that we're not
checking the new HeapTuple Datum verses the HeapTuple we read from the
buffer page while holding the lock on that page.

An update starts off by reading the existing tuple using the table AM.
Then a new updated tuple is created as the set of changes to the old.
Then the new TupleTableSlot is the combination of the existing one we
just read and the changes we just recorded. So, in the executor before
calling into the table AM's update function we have a pin on the buffer
and the before/after TupleTableSlots for this update. So, I've put the
call to my new ExecCheckIndexedAttrsForChanges() function just before
calling table_tuple_update() and I've added the "mix_attrs" into that
call which get passed on to the heap in heap_tuple_update() and then
heap_update() and all is well.

Why is this safe? The way I read heap_update() is that it has always
historically had code to deal with cases where the tuple is concurrently
updated and react accordingly thanks to HeapTupleSatisfiesUpdate() which
remains where it was in heap_update(). Visibility checks happened when
we first read the tuple to form the updated tuple and later in
heap_update() when we call HeapTupleSatisfiesVisibility() to check for
transaction-snapshot mode RI updates.

So, this new update path from the executor into the table AM seems to me
to be okay and almost functionally equivalent. But there is one big
change to discuss before moving to the simple_heap_update() path.

In nodeModifyTable.c tts_attr_equal() replaces heap_attr_equal()
changing the test for equality when calling into heap_tuple_update().
In the past we used datumIsEqual(), essentially a binary comparison
using memcmp(), now the comparison code in tts_attr_equal uses
type-specific equality function when available and falls back to
datumIsEqual() when not.

The other parts of HeapDetermineColumnsInfo() remain in the code, but
they still happen within the simple_heap_update() and
heap_tuple_update() code. That's where you'll find that after the
buffer is locked we do (1) and (2) from above. This keeps the
heap-specific work in the heap, but we've moved some work up into the
executor and outside the buffer lock.

While this accomplishes the goal of removing HeapDetermineColumnsInfo()
from heap_update() on the path that uses the table AM API
heap_tuple_update() but it doesn't on the simple_heap_update() path.
That remains the same as it was in the previous patch. Ideally, my
patch to restructure how catalog tuples are updated [5] is committed and
we can fully remove HeapDetermineColumnsInfo() and likely speed up all
catalog updates in the process. That's what motivated [5], please take
a look, it required a huge number of changes so I thought it deserved a
life/thread of its own.

Finally, there is the ExecSimpleRelationUpdate() path and
slot_modify_data(). On this path we know what attributes are being
updated, so we just check to see if they changed and then intersect that
with the set of indexed attributes and we have our modified indexed
attributes set to pass into simple_heap_update().

0003 - Replace index_unchanged_by_update with ri_ChangedIndexedCols

This patch removes the function index_unchanged_by_update() in
execIndexing.c and simply re-uses the modified indexed attributes that
we've stashed away in ResultRelInfo as ri_ChangedIndexedCols. This
provides a hint when calling into the index AM's index_insert() function
indicating if the UPDATE was without logical change to the data or not.
We've done that check, we don't need to do it again.

0004 - Enable HOT updates for expression and partial indexes

This finally gets us back to where this project started, but on much
more firm ground than before because we're not going to self-deadlock.
The idea has grown from a small function into something larger, but only
out of necessity.

In this patch I add ExecWhichIndexesRequireUpdates() in execIndexing.c
which implements (c) finding the set of attributes that force new index
updates. This set can be very different from the modified indexed
attributes. We know that some attributes are not equal to their
previous versions, but does that mean that the index that references
that attribute needs a new index tuple? It may, or it may not. Here's
the comment on that function that explains:

/*
* ExecWhichIndexesRequireUpdates
*
* Determine which indexes need updating given modified indexed attributes.
* This function is a companion to ExecCheckIndexedAttrsForChanges().
On the
* surface, they appear similar but they are doing two very different things.
*
* For a standard index on a set of attributes this is the intersection of
* the mix_attrs and the index attrs (key, expression, but not predicate).
*
* For expression indexes and indexes which implement the amcomparedatums()
* index AM API we'll need to form index datum and compare each
attribute to
* see if any actually changed.
*
* For expression indexes the result of the expression might not change
at all,
* this is common with JSONB columns which require expression indexes
and where
* it is commonplace to index a field within a document and have updates that
* generally don't update that field.
*
* Partial indexes won't trigger index tuples when the old/new tuples
are both
* outside of the predicate range.
*
* For nbtree the amcomparedatums() API is critical as it requires that key
* attributes are equal when they memcmp(), which might not be the case when
* using type-specific comparison or factoring in collation which might make
* an index case insensitive.
*
* All of this is to say that the goal is for the executor to know,
ahead of
* calling into the table AM for the update and before calling into the index
* AM for inserting new index tuples, which attributes at a minimum will
* necessitate a new index tuple.
*
...
*/

Whereas before we were comparing Datum in the table relation, now we're
comparing Datum in the index relation. Index AMs are free to store what
they want, we need to know if what's changed and referenced by the index
means that the index needs a new tuple or not.

In the case of a JSONB expression index (or any expression index) the
expression is evaluated when calling FormIndexDatum(). The result of
the expression is the Datum in the values/isnull arrays. Then we need
to compare them to see if they changed. This can be done using the same
tts_attr_equal() function, but with the attribute desc of the index, not
table relation.

In some cases that's not enough, for instance nbtree and it's more
stringent equality requirements. For that reason (and another coming up
in a second) we need a new optional index AM API, amcomparedatums().
Indexes implementing this function have the ability to compare two
Datums for equality in what ever way they want. For nbtree, that's a
binary comparison.

The new case here that this also supports is for indexes like GIN/RUM
where there is an opclass that can extract zero or more pieces from the
attribute and form multiple index entries when needed. Those extracted
pieces might have odd equality rules as well. This opens the door for
index implementations to provide information that will help inform heap
when making HOT decisions that didn't exist before.

Take for example the Linux Foundation's DocumentDB [6] project [7] which
aims to be an open source alternative to MongoDB built on top of
PostgreSQL. One of the pieces of that project is its "extended RUM"
index AM implementation. This index extracts portions of the BSON
documents stored and forms index keys from that. Here is an example:

CREATE INDEX documents_rum_index_14002
ON documentdb_data.documents_14001
USING documentdb_rum
(document bson_rum_single_path_ops (path=a, iswildcard='true', tl='2699'))

An index on the "document" column which uses the
"bson_rum_single_path_ops" opclass to extract a portion of the BSON
document that matches "path=a".

For this to potentially be stored by heap as a HOT update we need to
know that what changed within that document didn't intersect with the
"path=a" and if it did that the new value(s) were all equal to the old
values. Equality in DocumentDB isn't what you think, it's quite odd and
specific to BSON and rules defined by MongoDB so it's important to allow
the index AM to execute what's necessary for it's use case.

For the more common JSONB use case we can now have heap make an informed
decision about HOT updates after evaluating the expression. For
GIN/RUM/etc. implementation there is a path to HOT. For nbtree there is
a way to maintain its requirements.

Is there a cost to all this? Yes, of course. There is net new work
being done on some paths. IndexFormDatum() will be called more
frequently and sometimes twice for the same thing. This could be
improved, there might be a way to cache that information. But to have
tests of old/new we'll have to do that work at least twice.

Is there a benefit? Yes, of course. Some redundant code paths are gone
and in the end we've increased the number of cases where HOT updates are
a possibility. This especially helps out users of JSONB, but not only them.

What's left undone?

* I need to check code coverage so that I might
* create tests covering all the new cases
* update the README.HOT documentation, wiki, etc.
* performance...

For performance I'd like to examine some worst cases as in lots of
indexes that have a lot of new code to exercise and all but the last
index would allow for a HOT update. That should represent the maximum
amount of new overhead for this code. Then, the other side of the
equation, how much does this help JSONB? I think that is something to
measure in terms of TPS as well as "bloat" avoided and time spent vacuuming.

I also don't like the TU_Updating enum, I think it's a leaky abstraction
and really pointless now. I'd like to remove it in favor of the bitmap
of attributes known to force index tuples to be inserted. Maybe I'll
layer that into the next set.

In the end, this is a lot of work and I believe that it moves the ball
forward. I'll have more metrics on that soon I hope, but I wanted to
get the conversation re-started ASAP as we're late in the v19 cycle.

Finally, the "elephant" in the room (ha!) is PHOT[9]/WARM[10][11]. Yes,
some of this work does help make a solution to allow HOT updates when
only updating a subset of indexes closer to reality, I'd be lying not to
mention that here, it is a big part of my overall plan for my next year
and (I hope) v20 and I need this work to get there. Specifically, PHOT
is in part based on HeapDetermineColumnsInfo() and I needed to make that
more truthful for it to work.

I hope you see the value in this work and will partner with me to
finalize it and get it into master.

best.

-greg

[1] https://www.postgresql.org/message-id/flat/4d9928ee-a9e6-15f9-9c82-5981f13ffca6%40postgrespro.ru
[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c203d6cf8
[3] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=05f84605dbeb9cf8279a157234b24bbb706c5256
[4] https://www.postgresql.org/message-id/2877.1541538838%40sss.pgh.pa.us
[5] https://www.postgresql.org/message-id/flat/2C5C8B8D-8B36-4547-88EB-BDCF9A7C8D94(at)greg(dot)burd(dot)me
[6] https://www.linuxfoundation.org/press/linux-foundation-welcomes-documentdb-to-advance-open-developer-first-nosql-innovation
[7] https://github.com/documentdb/documentdb
[8] https://github.com/documentdb/documentdb/tree/main/pg_documentdb_extended_rum
[9] https://www.postgresql.org/message-id/flat/2ECBBCA0-4D8D-4841-8872-4A5BBDC063D2%40amazon.com
[10] https://www.postgresql.org/message-id/flat/CABOikdMop5Rb_RnS2xFdAXMZGSqcJ-P-BY2ruMd%2BbuUkJ4iDPw%40mail.gmail.com
[11]
https://www.postgresql.org/message-id/flat/CABOikdMNy6yowA%2BwTGK9RVd8iw%2BCzqHeQSGpW7Yka_4RSZ_LOQ%40mail.gmail.com

On Nov 16 2025, at 1:53 pm, Greg Burd <greg(at)burd(dot)me> wrote:

> 0004 - Enable HOT updates for expression and partial indexes
>
> This finally gets us back to where this project started, but on much
> more firm ground than before because we're not going to self-deadlock.
> The idea has grown from a small function into something larger, but only
> out of necessity.
>
> In this patch I add ExecWhichIndexesRequireUpdates() in execIndexing.c
> which implements (c) finding the set of attributes that force new index
> updates. This set can be very different from the modified indexed
> attributes. We know that some attributes are not equal to their
> previous versions, but does that mean that the index that references
> that attribute needs a new index tuple? It may, or it may not. Here's
> the comment on that function that explains:
>
> /*
> * ExecWhichIndexesRequireUpdates
> *
> * Determine which indexes need updating given modified indexed attributes.
> * This function is a companion to ExecCheckIndexedAttrsForChanges().
> On the
> * surface, they appear similar but they are doing two very different things.
> *
> * For a standard index on a set of attributes this is the intersection of
> * the mix_attrs and the index attrs (key, expression, but not predicate).
> *
> * For expression indexes and indexes which implement the amcomparedatums()
> * index AM API we'll need to form index datum and compare each
> attribute to
> * see if any actually changed.
> *
> * For expression indexes the result of the expression might not change
> at all,
> * this is common with JSONB columns which require expression indexes
> and where
> * it is commonplace to index a field within a document and have
> updates that
> * generally don't update that field.
> *
> * Partial indexes won't trigger index tuples when the old/new tuples
> are both
> * outside of the predicate range.
> *
> * For nbtree the amcomparedatums() API is critical as it requires that key
> * attributes are equal when they memcmp(), which might not be the case when
> * using type-specific comparison or factoring in collation which might make
> * an index case insensitive.
> *
> * All of this is to say that the goal is for the executor to know,
> ahead of
> * calling into the table AM for the update and before calling into the index
> * AM for inserting new index tuples, which attributes at a minimum will
> * necessitate a new index tuple.
> *
> ...
> */

Attached are rebased (d5b4f3a6d4e) patches with the only changes
happening in the last patch in the series.

0004 - Enable HOT updates for expression and partial indexes

I was never happy with the dual functions
ExecCheckIndexedAttrsForChanges() and ExecWhichIndexesRequireUpdates(),
it felt like too much overhead and duplication of effort. While
updating my tests, adding a few cases, I found that there was also a
flaw in the logic. So, time to rewrite and combine them.

What did I discover? Before the logic was to find the set of modified
indexed attributes then review all the indexes for changed attributes
using FormIndexDatum() and comparing before/after to see if expressions
really changed the value to be indexed or not. The first pass didn't
take into account expressions, the second did. So, an expression index
over JSONB data wouldn't extract and test the field within the document,
it was just comparing the entire document before/after using the jsonb
comparison function, no bueno.

This approach wraps both functions into one somewhat simplified
function. The logic is basically, iterate over the indexes reviewing
indexed attributes for changes. Along the way we call into the new
index AM's comparison function when present, otherwise we find and use
the proper type-specific comparison function for the datum. At the end
of the function we have our Bitmapset of attributes that should trigger
new index tuples.

> What's left undone?
>
> * I need to check code coverage so that I might

I did this and it was quite good, I'll do it again for this new series
but it's nice to see that the tests are exercising the vast majority of
the code paths.

> * create tests covering all the new cases

I think the coverage is good, maybe even redundant or overly complex in places.

> * update the README.HOT documentation, wiki, etc.

Soon, I hope to have this approach solid and under review before
solidifying the docs.

> * performance...

Still as yet unmeasured, I know that there is more work per-update to
perform these checks, so some overhead, but I don't know if that
overhead is more than before with HeapDetermineColumnsInfo() and
index_unchanged_by_update(). Those two functions did essentially the
same thing, only with binary comparison (datumIsEqual()). I need to
measure that. What about doing all this work outside of the buffer lock
in heap_update()? Surely that'll give back a bit or at least add to
concurrency. Forming index tuples a few extra times and evaluating the
expressions 3 times rather than 1 is going to hurt, I think I can come
up with a way to cache the formed datum and use it later on, but is that
worth it? Complex expressions, yes. Also, what about expressions that
expect to be executed once... and now are 3x? That's what forced my
update to the insert-conflict-specconflict.out test, but AFAICT there is
no way to test if an expression's value is going to change on update
without exercising it once for the old tuple and once for the new tuple.
Even if it were possible for an index to provide the key it might have
changed after the expression evaluation (as is the case in hash), so I
don't think this is avoidable. Maybe that's reason enough to add a
reloption to disable the expression evaluation piece of this? Given
that it might create a logic or performance regression. The flip side
is the potential to use the HOT path, that's a real savings.

One concerning thing is that nbtree's assumption that key attributes for
TIDs must use binary comparison for equality. This means that for our
common case (heap/btree) there is more work per-update than before,
which is why I need to measure. I could look into eliminating the
nbtree requirement, I don't understand it too well as yet by I believe
that on page split there is an attempt to deduplicate TIDs into a
TIDBitmap and the test for when that's possible is datumIsEqual(). If
that were the same as in this new code, possibly evening using
tts_attr_equal(), then... I don't know, I'll have to investigate. Chime
in here if you can educate me on this one. :)

best.

-greg

On Nov 19 2025, at 1:00 pm, Greg Burd <greg(at)burd(dot)me> wrote:

>
> On Nov 16 2025, at 1:53 pm, Greg Burd <greg(at)burd(dot)me> wrote:
>
>> 0004 - Enable HOT updates for expression and partial indexes
>>
>> This finally gets us back to where this project started, but on much
>> more firm ground than before because we're not going to
>> self-deadlock.
>> The idea has grown from a small function into something larger, but only
>> out of necessity.
>>
>> In this patch I add ExecWhichIndexesRequireUpdates() in execIndexing.c
>> which implements (c) finding the set of attributes that force new index
>> updates. This set can be very different from the modified indexed
>> attributes. We know that some attributes are not equal to their
>> previous versions, but does that mean that the index that references
>> that attribute needs a new index tuple? It may, or it may not. Here's
>> the comment on that function that explains:
>>
>> /*
>> * ExecWhichIndexesRequireUpdates
>> *
>> * Determine which indexes need updating given modified indexed attributes.
>> * This function is a companion to ExecCheckIndexedAttrsForChanges().
>> On the
>> * surface, they appear similar but they are doing two very different things.
>> *
>> * For a standard index on a set of attributes this is the
>> intersection of
>> * the mix_attrs and the index attrs (key, expression, but not predicate).
>> *
>> * For expression indexes and indexes which implement the amcomparedatums()
>> * index AM API we'll need to form index datum and compare each
>> attribute to
>> * see if any actually changed.
>> *
>> * For expression indexes the result of the expression might not change
>> at all,
>> * this is common with JSONB columns which require expression indexes
>> and where
>> * it is commonplace to index a field within a document and have
>> updates that
>> * generally don't update that field.
>> *
>> * Partial indexes won't trigger index tuples when the old/new tuples
>> are both
>> * outside of the predicate range.
>> *
>> * For nbtree the amcomparedatums() API is critical as it requires
>> that key
>> * attributes are equal when they memcmp(), which might not be the
>> case when
>> * using type-specific comparison or factoring in collation which
>> might make
>> * an index case insensitive.
>> *
>> * All of this is to say that the goal is for the executor to know,
>> ahead of
>> * calling into the table AM for the update and before calling into
>> the index
>> * AM for inserting new index tuples, which attributes at a minimum will
>> * necessitate a new index tuple.
>> *
>> ...
>> */
>
> Attached are rebased (d5b4f3a6d4e) patches with the only changes
> happening in the last patch in the series.
>
> 0004 - Enable HOT updates for expression and partial indexes
>
> I was never happy with the dual functions
> ExecCheckIndexedAttrsForChanges() and ExecWhichIndexesRequireUpdates(),
> it felt like too much overhead and duplication of effort. While
> updating my tests, adding a few cases, I found that there was also a
> flaw in the logic. So, time to rewrite and combine them.
>
> What did I discover? Before the logic was to find the set of modified
> indexed attributes then review all the indexes for changed attributes
> using FormIndexDatum() and comparing before/after to see if expressions
> really changed the value to be indexed or not. The first pass didn't
> take into account expressions, the second did. So, an expression index
> over JSONB data wouldn't extract and test the field within the document,
> it was just comparing the entire document before/after using the jsonb
> comparison function, no bueno.
>
> This approach wraps both functions into one somewhat simplified
> function. The logic is basically, iterate over the indexes reviewing
> indexed attributes for changes. Along the way we call into the new
> index AM's comparison function when present, otherwise we find and use
> the proper type-specific comparison function for the datum. At the end
> of the function we have our Bitmapset of attributes that should trigger
> new index tuples.
>
>> What's left undone?
>>
>> * I need to check code coverage so that I might
>
> I did this and it was quite good, I'll do it again for this new series
> but it's nice to see that the tests are exercising the vast majority of
> the code paths.
>
>> * create tests covering all the new cases
>
> I think the coverage is good, maybe even redundant or overly complex
> in places.
>
>> * update the README.HOT documentation, wiki, etc.
>
> Soon, I hope to have this approach solid and under review before
> solidifying the docs.
>
>> * performance...
>
> Still as yet unmeasured, I know that there is more work per-update to
> perform these checks, so some overhead, but I don't know if that
> overhead is more than before with HeapDetermineColumnsInfo() and
> index_unchanged_by_update(). Those two functions did essentially the
> same thing, only with binary comparison (datumIsEqual()). I need to
> measure that. What about doing all this work outside of the buffer lock
> in heap_update()? Surely that'll give back a bit or at least add to
> concurrency. Forming index tuples a few extra times and evaluating the
> expressions 3 times rather than 1 is going to hurt, I think I can come
> up with a way to cache the formed datum and use it later on, but is that
> worth it? Complex expressions, yes. Also, what about expressions that
> expect to be executed once... and now are 3x? That's what forced my
> update to the insert-conflict-specconflict.out test, but AFAICT there is
> no way to test if an expression's value is going to change on update
> without exercising it once for the old tuple and once for the new tuple.
> Even if it were possible for an index to provide the key it might have
> changed after the expression evaluation (as is the case in hash), so I
> don't think this is avoidable. Maybe that's reason enough to add a
> reloption to disable the expression evaluation piece of this? Given
> that it might create a logic or performance regression. The flip side
> is the potential to use the HOT path, that's a real savings.
>
> One concerning thing is that nbtree's assumption that key attributes for
> TIDs must use binary comparison for equality. This means that for our
> common case (heap/btree) there is more work per-update than before,
> which is why I need to measure. I could look into eliminating the
> nbtree requirement, I don't understand it too well as yet by I believe
> that on page split there is an attempt to deduplicate TIDs into a
> TIDBitmap and the test for when that's possible is datumIsEqual(). If
> that were the same as in this new code, possibly evening using
> tts_attr_equal(), then... I don't know, I'll have to investigate. Chime
> in here if you can educate me on this one. :)
>
> best.
>
> -greg

Doh!

I forgot to commit the fixed regression test expected output before
formatting the patch set, here it is.

-greg

On Wed, 19 Nov 2025 at 19:00, Greg Burd <greg(at)burd(dot)me> wrote:
>
> Attached are rebased (d5b4f3a6d4e) patches with the only changes
> happening in the last patch in the series.

Here's a high-level review of the patchset, extending on what I shared
offline. I haven't looked too closely at the code changes.

Re: Perf testing

Apart from the workload we've discussed offline, there's another
workload to consider: Right now, we only really consider HOT when we
know there's space on the page. This patch, however, will front-load a
lot more checks before we have access to the page, and that will
~always impact update performance.
I'm a bit worried that the cost of off-page updates (when the page of
the old tuple can't fit the new tuple) will be too significantly
increased, especially considering that we have a default fillfactor of
100 -- if a table's tuples only grow, it's quite likely the table
frequently can't apply HOT regardless of the updated columns. So, a
workload that's tuned to only update tuples in a way that excercises
'can we HOT or not' code for already-full pages would be appreciated.
A solution to the issue might be needed if we lose too much
performance on expensive checks; a solution like passing page space of
the old tuple to the checks, and short-circuiting the non-HOT path if
that page space is too small for the new tuple.

0001:

I'm not sure I understand why the code is near completely duplicated
here. Maybe you can rename the resulting heap_update to
heap_update_ext, and keep a heap_update around which wraps this
heap_update_ext, allowing old callers to keep their signature until
they need to use _ext's features? Then you can introduce the
duplication if and when needed in later patches -- though I don't
expect a lot of this duplication to be strictly necessary, though that
may need some new helper functions.

I also see that for every update we're now copying, passing 5
bitmapsets individually and then freeing those bitmaps just a moment
later. I'd like to avoid that overhead and duplication if possible.
Maybe we can store these in an 'update context' struct, passed by
reference down to table_tuple_update() from the calling code, and then
onward to heap_update? That might then also be a prime candidate to
contain the EState * of ExecCheckIndexedAttrsForChanges.

0002:

This patch seems to have some formatting updates to changes you made
in 0001, without actually changing the code (e.g. at heap_update's
definition). When updating code, please put it in the expected
formatting in the same patch.

---
In the patch subject:
> For instance,
> indexes with collation information allowing more HOT updates when the
> index is specified to be case insensitive.

It is incorrect to assume that indexed "btree-equal" datums allow HOT
updates. The user can not be returned an old datum in index-only
scans, even if it's sorted the same as the new datum -- after all, a
function on the datum may return different results even if the datums
are otherwise equal. Think: count_uppercase(string). See also below at
"HOT, datum compare, etc".

---
With the addition of rd_indexedattr we now have 6 bitmaps in
RelationData, which generally only get accessed through
RelationGetIndexAttrBitmap by an enum value. Maybe it's now time to
bite the bullet and change that to a more general approach with
`Bitmapset *rd_bitmaps[NUM_INDEX_ATTR_BITMAP]`? That way, the hot
path in RelationGetIndexAttrBitmap would not depend on the compiler to
determine that the fast path can do simple offset arithmatic to get
the requested bitmap.

0003:
This looks like it's a cleanup patch for 0002, and doesn't have much
standing on its own. Maybe the changes for ri_ChangedIndexedCols can
all be moved into 0003? I think that gives this patch more weight and
standing.

0004:
This does two things:
1. Add index expression evaluation to the toolset to determine which
indexes were unchanged, and
2. Allow index access methods to say "this value has not changed"
even if the datum itself may have changed.

Could that be split up into two different patches?

(aside: 2 makes a lot of sense in some cases, like trgm indexes
strings if no new trigrams are added/removed, so I really like the
idea behind this change)

HOT, datum compare, etc.:

Note that for index-only scans on an index to return correct results,
you _must_ update the index (and thus, do a non-HOT update) whenever a
value changes its binary datum, even if the value has the same btree
sort location as the old value. Even for non-IOS-supporting indexes,
the index may need more information than what's used in btree
comparisons when it has a btree opclass.
As SP/GIST have IOS support, they also need to compare the image of
the datum and not use ordinary equality as defined in nbtree's compare
function: the value must be exactly equal to what the table AM
would've provided.

Primary example: Btree compares the `numeric` datums of `1.0` and
`1.00` as equal, but for a user there is an observable difference; the
following SQL must return `1.0` in every valid plan:

BEGIN;
INSERT INTO mytab (mynumeric) VALUES ('1.00');
UPDATE mytab SET mynumeric = '1.0';
SELECT mynumeric FROM mytab;

So, IMO, the default datum compare in ExecCheckIndexedAttrsForChanges
and friends should just use datumIsEqual, and not this new
tts_attr_equal.
Indexes without IOS support might be able to opt into using a more lax
datum comparator, but 1.) it should never be the default, as it'd be a
loaded footgun for IndexAM implementers, and 2.) should not depend on
another AM's understanding of attributes, as that is a very leaky
abstraction.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

On Nov 21 2025, at 10:25 am, Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)com> wrote:

> On Wed, 19 Nov 2025 at 19:00, Greg Burd <greg(at)burd(dot)me> wrote:
>>
>> Attached are rebased (d5b4f3a6d4e) patches with the only changes
>> happening in the last patch in the series.
>
> Here's a high-level review of the patchset, extending on what I shared
> offline. I haven't looked too closely at the code changes.

Matthias, thanks for spending a bit of time writing up your thoughts and
for chatting a bit with me before doing so. I really appreciate your
point of view.

> Re: Perf testing
>
> Apart from the workload we've discussed offline, there's another
> workload to consider: Right now, we only really consider HOT when we
> know there's space on the page.

Yes, and no. In master every UPDATE triggers a call into
HeapDetermineColumnsInfo() in heap_update(). That function's job is to
examine all the indexed attributes checking each attribute for changes.
The set of indexed attributes is generated by combining a set of
Bitmapsets fetched (copied) from the relcache:

hot_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_HOT_BLOCKING);
sum_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_SUMMARIZED);
key_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_KEY);
id_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_IDENTITY_KEY);

interesting_attrs = NULL;
interesting_attrs = bms_add_members(interesting_attrs, hot_attrs);
interesting_attrs = bms_add_members(interesting_attrs, sum_attrs);
interesting_attrs = bms_add_members(interesting_attrs, key_attrs);
interesting_attrs = bms_add_members(interesting_attrs, id_attrs);

These "interesting attributes" are each checked comparing the newly
formed updated HeapTuple passed in from the executor against a HeapTuple
read from the page. This happens after taking the buffer lock on that
page. The comparison is done with heap_attr_compare() which calls
datumIsEqual() which boils down to a memcmp(). The function returns the
"modified_attrs".

This is the primary "work" that happens before considering HOT that I've
moved outside the buffer lock and into the executor. The rest of the
HOT decision is 1) will it fit (possibly after being TOASTed), and b) do
the HOT blocking attributes overlap with the modified_attrs? If they
do, unless they are all summarizing you can't go HOT.

Net new work with my approach is only related to the more complicated
checks that have to be done when there are expressions, partial indexes,
or an index implements the new index AM API for comparing datums. In
those cases there is a bit more work, especially when it comes to
expression indexes. Evaluating partial index predicate twice rather
than once is a bit more overhead. Using type-specific comparison when
the index doesn't support index-only scans is a tad more. Overall, not
a whole lot of new work. Certainly a much more complex code path, and
maybe that'll show up in performance tests. I don't know yet.

> This patch, however, will front-load a
> lot more checks before we have access to the page, and that will
> ~always impact update performance.

Disagree, 90% of that work happens today on that same path after the
page lock. Even in the case that the HeapTuple can't fit into the page
that work will happen, there's not much net new "wasted" effort.

Concurrency under load may be better because buffer locks will be held
for less time.

> I'm a bit worried that the cost of off-page updates (when the page of
> the old tuple can't fit the new tuple) will be too significantly
> increased, especially considering that we have a default fillfactor of
> 100 -- if a table's tuples only grow, it's quite likely the table
> frequently can't apply HOT regardless of the updated columns. So, a
> workload that's tuned to only update tuples in a way that excercises
> 'can we HOT or not' code for already-full pages would be appreciated.
> A solution to the issue might be needed if we lose too much
> performance on expensive checks; a solution like passing page space of
> the old tuple to the checks, and short-circuiting the non-HOT path if
> that page space is too small for the new tuple.

Here's the problem, there's not much that can be known about what
ultimate size the HeapTuple will take or if the page can hold it until
after the lock.

Second, I feel that this signal would be specific to the heap, it's
particular MVCC implementation, and it's optimization (HOT). I really
wanted this solution to be non-heap-specific, but heap-enabling.

For me that meant that in the general case the executor should be
concerned with updating only the set of indexes that it must and no
more. So performing this work in the executor ahead of calling into the
table AM or index AM makes sense.

It is only due to heap's "special" model that we have other concerns
related to HOT. Sure, everyone/everything today uses heap so we should
pay attention to this, but I set out not to create yet another thing
that depends on heap's specific operational model. I think I did that.

> 0001:
>
> I'm not sure I understand why the code is near completely duplicated
> here. Maybe you can rename the resulting heap_update to
> heap_update_ext, and keep a heap_update around which wraps this
> heap_update_ext, allowing old callers to keep their signature until
> they need to use _ext's features? Then you can introduce the
> duplication if and when needed in later patches -- though I don't
> expect a lot of this duplication to be strictly necessary, though that
> may need some new helper functions.

This is just a split where I move the top portion of heap_update() into
the two paths that use it. Sure I could have pulled that into another
function, but in this series the next step is to obliterate one half and
(if I get my other patch in cf-6221) then I can completely remove
HeapDetermineColumnsInfo() and vastly simplify simple_heap_update().

> I also see that for every update we're now copying, passing 5
> bitmapsets individually and then freeing those bitmaps just a moment
> later. I'd like to avoid that overhead and duplication if possible.

We do this today, nothing new here. They are passed by reference, not
value into heap_update().

> Maybe we can store these in an 'update context' struct, passed by
> reference down to table_tuple_update() from the calling code, and then
> onward to heap_update? That might then also be a prime candidate to
> contain the EState * of ExecCheckIndexedAttrsForChanges.

I've explored using UpdateContext before, not a bad idea but again this
is just a setup commit. It could be cleaner on it's own, but it doesn't
really take a step backward on any dimension.

> 0002:
>
> This patch seems to have some formatting updates to changes you made
> in 0001, without actually changing the code (e.g. at heap_update's
> definition). When updating code, please put it in the expected
> formatting in the same patch.

I'll find/fix those after v23 attached, sorry for the noise.

> ---
> In the patch subject:
>> For instance,
>> indexes with collation information allowing more HOT updates when the
>> index is specified to be case insensitive.
>
> It is incorrect to assume that indexed "btree-equal" datums allow HOT
> updates. The user can not be returned an old datum in index-only
> scans, even if it's sorted the same as the new datum -- after all, a
> function on the datum may return different results even if the datums
> are otherwise equal. Think: count_uppercase(string). See also below at
> "HOT, datum compare, etc".

Thanks for pointing out the oversight for index-oriented scans (IOS),
you're right that the code in v22 doesn't handle that correctly. I'll
fix that. I still think that indexes that don't support IOS can and
should use the type-specific equality checks. This opens the door to
HOT with custom types that have unusual equality rules (see BSON).

> With the addition of rd_indexedattr we now have 6 bitmaps in
> RelationData, which generally only get accessed through
> RelationGetIndexAttrBitmap by an enum value. Maybe it's now time to
> bite the bullet and change that to a more general approach with
> `Bitmapset *rd_bitmaps[NUM_INDEX_ATTR_BITMAP]`? That way, the hot
> path in RelationGetIndexAttrBitmap would not depend on the compiler to
> determine that the fast path can do simple offset arithmatic to get
> the requested bitmap.

More than a few of those bitmaps in RelationData are purely for the HOT
tests, yes. I'll review those and see if I can shrink the set
meaningfully. It was something that had occurred to me too. I'll
review and consider ways to consolidate them.

> 0003:
> This looks like it's a cleanup patch for 0002, and doesn't have much
> standing on its own. Maybe the changes for ri_ChangedIndexedCols can
> all be moved into 0003? I think that gives this patch more weight and
> standing.

The goal in this patch was to show that we could eliminate the redundant
set work of index_unchanged_by_update() in execIndexing's update path.
Separating it out was done to make it more easily reviewed and to prove
that before/after tests passed making the change safe.

> 0004:
> This does two things:
> 1. Add index expression evaluation to the toolset to determine which
> indexes were unchanged, and
> 2. Allow index access methods to say "this value has not changed"
> even if the datum itself may have changed.

Yes, that's what it does. :)

> Could that be split up into two different patches?

Maybe, I might be able to add the index AM piece first and then the
expression piece.

> (aside: 2 makes a lot of sense in some cases, like trgm indexes
> strings if no new trigrams are added/removed, so I really like the
> idea behind this change).

Nice, I appreciate that.

> HOT, datum compare, etc.:
>
> Note that for index-only scans on an index to return correct results,
> you _must_ update the index (and thus, do a non-HOT update) whenever a
> value changes its binary datum, even if the value has the same btree
> sort location as the old value.

Yes, I get this now. I also wasn't testing the INCLUDING (non-key)
columns. I've fixed both of those in v23.

> Even for non-IOS-supporting indexes,
> the index may need more information than what's used in btree
> comparisons when it has a btree opclass.

It's not always just the btree opclass for equality, but that is common.

> As SP/GIST have IOS support, they also need to compare the image of
> the datum and not use ordinary equality as defined in nbtree's compare
> function: the value must be exactly equal to what the table AM
> would've provided.

In v23 I've changed the logic to use datumIsEqual() for any index that
supports IOS and doesn't supply a custom amcomparedatums() function.

> Primary example: Btree compares the `numeric` datums of `1.0` and
> `1.00` as equal, but for a user there is an observable difference; the
> following SQL must return `1.0` in every valid plan:
>
> BEGIN;
> INSERT INTO mytab (mynumeric) VALUES ('1.00');
> UPDATE mytab SET mynumeric = '1.0';
> SELECT mynumeric FROM mytab;

I'll try to reproduce this and add a test if I can.

> So, IMO, the default datum compare in ExecCheckIndexedAttrsForChanges
> and friends should just use datumIsEqual, and not this new
> tts_attr_equal.

Sure, and that's the case in v23. tts_attr_equal() still has value in
other cases so it's not gone.

> Indexes without IOS support might be able to opt into using a more lax
> datum comparator, but 1.) it should never be the default, as it'd be a
> loaded footgun for IndexAM implementers, and 2.) should not depend on
> another AM's understanding of attributes, as that is a very leaky
> abstraction.

Agree, which is why the default for IOS indexes is datumIsEqual() now.

> Kind regards,
>
> Matthias van de Meent
> Databricks (https://www.databricks.com)

Thanks again for the time and renewed interest in the patch! I've also
added a lot more tests into the heap_hot_updates.sql regression suite,
likely too many, but for now it's good to be testing the corners.

v23 attached, changes are all in 0004, best.

-greg

On Sat, 22 Nov 2025, 22:30 Greg Burd, <greg(at)burd(dot)me> wrote:
> Thanks for pointing out the oversight for index-oriented scans (IOS),
> you're right that the code in v22 doesn't handle that correctly. I'll
> fix that. I still think that indexes that don't support IOS can and
> should use the type-specific equality checks. This opens the door to
> HOT with custom types that have unusual equality rules (see BSON).

Do you have specific examples why it would be safe to default to
"unusual equality rules" for generally any index's data ingestion
needs? Why e.g. BSON must always be compared with their special
equality test (and not datumIsEqual), and why IOS-less indexes in
general are never going to distinguish between binary distinct but
btree-equal values, and why exact equality is the special case here?

I understand that you want to maximize optimization for specific
workloads that you have in mind, but lacking evidence to the contrary
I am really not convinced that your workloads are sufficiently
generalizable that they can (and should) be the baseline for these new
HOT rules: I have not yet seen good arguments why we could relax
"datum equality" to "type equality" without potentially breaking
existing indexes.

HOT was implemented quite conservatively to make sure that there are
no issues where changed values are not reflected in indexes: each
indexed TID represents a specific and unchanging set of indexed
values, in both the key and non-key attributes of indexes. If a value
changes however so slightly, that may be a cause for indexes to treat
it differently, and thus HOT must not be used.

Aside: The amsummarizing optimization gets around that check by
realizing the TID itself isn't really indexed, so the rules can be
relaxed around that, but it still needs to go through the effort to
update the summarizing indexes if the relevant attributes were ever so
slightly updated.

This patch right now wants to change these rules and behaviour of HOT
in two ways:

1.) Instead of only testing attributes mentioned by indexed
expressions for changes, it wants to test the output of the indexed
expressions.
I would consider this to be generally safe, as long as the expressions
comply with the rules we have for indexed expressions. [Which, if not
held, would break IOS and various other things, too, so relying on
these rules isn't new or special].

2.) Instead of datumIsEqual, it (by default) wants to do equality
checks as provided by the type's default btree opclass' = operator.
I have not seen evidence that this is safe. I have even explained with
an example that IOS will return distinctly wrong results if only
btree's = operator is used to determine if HOT can be applied, and
that doesn't even begin to cover the issues related to indexes that
may handle data differently from Btree.
I also don't want indexes that at some point in the future invent
support for IOS to return subtly incorrect results due to HOT checks
that depended on the output of a previous version's amcanreturn
output.

So, IMV, tts_attr_equal is just a rather expensive version of
datumIsEqual: the fast path cases would've been handled by
datumIsEqual at least as fast (without a switch() statement with 18
specific cases and a default branch); if there is no btree operator
it'll still default to btree compare, and if not then if the slow path
uses correctly implemented compare operators (for HOT, and potentially
all other possible indexes), then these would have an output that is
indistinguishable from datumIsEqual, with the only difference the
address and performance of the called function and a lot of added
catalog lookups.

All together, I think it's best to remove the second component of the
changes to the HOT rules (changing the type of matching done for
indexed values with tts_attr_compare) from this patchset.
If you believe this should be added regardless, I think it's best
discussed separately in its own thread and patchset -- it should be
relatively easy to introduce in both current and future versions of
this code, and (if you're correct and this is safe) it would have some
benefits even when committed on its own.

Kind regards,

Matthias van de Meent

On Nov 24 2025, at 1:59 pm, Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)com> wrote:

> ... <awesome thoughtful questions, insights, etc> ...
>
> Kind regards,
>
> Matthias van de Meent

Hey Matthias,

I've updated the patch set to v25 and taken your suggested approach of
minimizing changes in hopes of getting the majority of this patch
series committed (I hope) soon. By that I mean that $subject isn't
really accurate anymore. This patch uses datumIsEqual(), but still
introduces the new index AM API and still moves
HeapDetermineColumnsInfo() into a function in nodeModifyTable.c called
ExecWhichIndexesRequireUpdates(). The "big idea" is that before calling
into the table AM to update a tuple the executor should know what the
impact of that update will be for the indexes on the relation.
ExecWhichIndexesRequireUpdates() finds the set of attributes that were
a) modified and b) are referenced by an index and cause that index to
need a new index tuple. This is left up to heap now, but really should
be generic across all table AMs (IMO), so that's what I've done.

At some point in the future maybe there's a way to switch from the
heap-specific model of all/none/summarized-only to something where we
only update indexes that really require the updates (HOT, WARM, etc.)
and I think this is a step in that direction, but for now the logic
remains the same as does the signal (TU_Updated).

I'll re-introduce $subject as a layer on v24 next week at which point
I'll try to address all the good points you raised in your email. Those
additions (HOT expressions, HOT partial indexes, and type-specific
equality tests) are the most controversial.

I think it may be possible that these first few patches are less
controversial and could make the cut sooner while those other ideas
remain up for debate. I'm open to that, I think the work in the attached
set is good and valuable on it's own.

So, the attached patch set

Benefits of the patch:
* the tests for what changed move outside of the buffer lock
* the redundant index_unchanged_by_update() is removed

Downsides of the patch:
* a bit of new overhead in some cases
* a bit more complicated logic than before

Recall that this patch set combines with another one of mine on the list
[1] which covers the simple_heap_update() path, this one doesn't cover
that case and you'll see that simple_heap_update() still depends on HeapDetermineColumnsInfo().

* 0001 - Prepare heapam_tuple_update() and simple_heap_update() for divergence

This splits off the top of heap_update() and places that logic in both
heapam_tuple_update() and simple_heap_update(). This patch is also
present in the other thread [1] and essentially the same. That thread
addresses the changes to the catalog tuple updates. No real effort was
made to make this patch "pretty" or "stand-alone" as it really is a
precursor to the work in 0002 and in [1].

* 0002 - Track changed indexed columns in the executor during UPDATEs

This is where the meat is, as described in earlier emails and the commit
message. HeapDetermineColumnsInfo() logic moves up into the executor
into ExecWhichIndexesRequireUpdates(). Some heap-specific logic related
to replica identity remains in heapam_tuple_update().

* 0003 - Replace index_unchanged_by_update() with ri_ChangedIndexedCols

This removes the now redundant index_unchanged_by_update() function and
instead uses the information gathered in
ExecWhichIndexesRequireUpdates() and recorded in ri_ChangedIndexCols for
the same outcome.

best.

-greg

[1] https://www.postgresql.org/message-id/flat/2C5C8B8D-8B36-4547-88EB-BDCF9A7C8D94(at)greg(dot)burd(dot)me

I've updated the patch set a tad and I've got some benchmark results
(and questions).

PATCHES
===========================================================

* 0001 - Prepare heapam_tuple_update() and simple_heap_update() for divergence

Unchanged.

* 0002 - Track changed indexed columns in the executor during UPDATEs

Bug/oversight minor fix related to partial index attributes.

Also, I mistakenly said that v25 removed the $subject (ability to allow
expression indexes to be HOT). That's not true, they can go HOT with
this patch provided that the result of the expression evaluated using
the before/after attribute values are equal using datumIsEqual(). When
that is the case, as can happen with updates to fields within JSONB
columns when indexes are on other fields, the update can be HOT should
the heap find room on the page to store the new tuple.

* 0003 - Replace index_unchanged_by_update() with ri_ChangedIndexedCols

Unchanged.

* 0004 - Identify if partial indexes are impacted by an update

This is the new piece, it existed in the v24 patch set and now it is
back. This checks the before/after partial index expression and when
both are outside the predicate then it is possible that heap can use the
HOT path whereas in the past this couldn't happen. In the past any
update to an attribute in an index, even if it was outside the
predicate, was (is) HOT blocking.

SUMMARY
===========================================================

I've just started to scratch the surface of performance testing for
this, attached is a very simple comparison of master/patch for a basic
update load that should always go HOT in either case. It shows about 1%
variance between the two (-O0), tests run on my laptop so that's
essentially no difference despite more overhead of the new function and
that it seems to be called more frequently due to (guessing here) more
opportunity for TM_Updated to be the return from heapam_tuple_update.
Your thoughts welcome here, or best/worst case ideas for tests to run.

Next up I plan to layer the controversial type-specific piece into this
patch set if nothing else just as a record of what's left over. Then
I'll try to better isolate good/bad performance implications of this
patch set.

Ideally, this patch set and the one (under development) for catalog
tuples could combine to completely restructure the heap update process
and open the door to more HOT updates and faster catalog updates. But,
I still have to demonstrate that. For JSONB heavy applications this
should be a net win, for the rest it should be a minor or zero
regression. For other custom implementations of indexes over
specialized types (as is the case for the new open sourced DocumentDB
work) this opens the door for HOT updates when possible. All of that is
the the hope, it's time to measure hope against reality. :)

This patch set does start to move the executor away from a heap-specific
view of the world where updates are all/none/summarizing. This
potentially eases the integration of WARM or PHOT-like solutions where
we only update those indexes that are materially impacted by an update.
It should be clear by now, that's my ultimate goal.

best.

-greg

Rebased to address conflicts.

best.

-greg

Rebased again to address build failure.

-greg

Hello,

TL;DR, I'm going to put a pin in this idea for now.

I'll net out where this patch set is quickly and leave the majority of the
detail for anyone curious in an addendum below.

The patch set enables HOT updates a few new cases:
* when expressions on indexes remain constant
* when updates fall outside partial index predicates
* when indexes report that their keys are unchanged by the update

The benefits are:
* more HOT updates
* reduced index bloat
* modified index attributes identified ahead of table AM update

The drawbacks are:
* the overhead of doing net-new work determining modified attributes
* higher concurrency causing retries, compounding that work
* index and predicate expressions executed more frequently

Performance:
------------

The good,
* HOT improvements: 95-100%
* Bloat reduction: 50-98%
* Update 100 rows at once: +27% throughput, -98% bloat

the bad,
* -2% to -6% throughput regression on updates across the board

and the ugly.
- net new work finding modified indexed attributes
- net new retries on update due to added concurrency (TU_Updated)
- retries then must re-identify modified indexed attributes

While I still believe in expanding HOT updates and that moving the logic for
finding the set of modified index attributes into the executor was a good idea,
it is late in the v19 cycle and at this stage the trade-offs feel unacceptable.

So I'm going to withdraw this patch and pause here on development and reconsider
my approach in the v20 cycle. I have some ideas on how to avoid the need to
evaluate these expressions entirely and I still have a fondness for the idea of
only updating those indexes that changed (PHOT/WARM/whatever) on update, maybe
I'll combine the ideas.

best.

-greg

ADDENDUM:

ExecUpdate (nodeModifyTable.c)
-> [PATCHED] after the l2: label, ExecCheckIndexedAttrsForChanges()
-> [UNPATCHED] (nothing at this point...)
-> table_tuple_update()
-> heap_update()
-> LockBuffer(BUFFER_LOCK_EXCLUSIVE)
-> [UNPATCHED] HeapDetermineColumnsInfo() // uses memcmp() for datum comparison
-> returns TM_Updated immediately, frees modified_attrs
-> EvalPlanQual() // recheck with updated tuple
-> ExecUpdate (retry)
-> [PATCHED] ExecCheckIndexedAttrsForChanges() // second evaluation
-> [UNPATCHED] (nothing at this point...)
-> table_tuple_update()
-> heap_update()
-> LockBuffer(BUFFER_LOCK_EXCLUSIVE)
-> HeapDetermineColumnsInfo() // second evaluation

As illustrated above, the patches move indexed attribute checking from
HeapDetermineColumnsInfo() (called once in heap_update under buffer lock
BUFFER_LOCK_EXCLUSIVE) to ExecCheckIndexedAttrsForChanges() (called in the
executor before calling table_tuple_update() when the buffer with the old tuple
is pinned, but not holding locked exclusively). In the unpatched code, when
heap_update() returns TM_Updated due to a concurrent update, it immediately
returns to the executor without retrying - the modified_attrs bitmapset is freed
and the executor performs an EPQ (EvalPlanQual) recheck before calling
table_tuple_update() again. On this second call, the unpatched code re-executes
HeapDetermineColumnsInfo() with the new tuple because the modified_attrs may
have changed. The patched code similarly calls ExecCheckIndexedAttrsForChanges()
again, as it will have jumped to the l2: label, before retrying
table_tuple_update().

Both versions re-evaluate to find the modified attributes bitmap on concurrent
conflicts, the key differences are:

1) the patched version performs richer comparisons (partial index predicates,
index AM amcomparedatums(), expression evaluation) while the unpatched
version only does binary datum comparison using memcmp() and,

2) under high contention running outside the buffer lock has increased the
probability TM_Updated because he window for concurrent modifications has
increased.

It may be that expanding the window for concurrency (2) during updates is a
mistake as it is impossible for the heap to update two different tuples on the
same page concurrently so this has no potential benefit and compounds the
expense.

However, it is entirely possible that other table AMs could have different
concurrency models and so in theory we should allow for that. Consider how some
LSMs work, concurrent updates are just appends to the current log file. Maybe
the table AM should indicate a need for a buffer lock or not during updates?

PATCHES:

0001: Refactors heapam_tuple_update()/heap_update() by removing some code in the
beginning of heap_update() into both heapam_tuple_update() and
simple_heap_update() as a preface for removing the need for
HeapDetermineColumnsInfo() when calling heapam_tuple_update().

0002: Moves the detection of modified indexed columns from within heap into the
executor in a new function ExecWhichIndexesRequireUpdates() that replaces
HeapDetermineColumnsInfo().

This shift now detects the set of modified indexed columns earlier in the
query execution process (just after the l2: label in nodeModifyTable.c)
and crucially outside of an exclusive buffer page lock as was the case
before in heap_update().

ExecCheckIndexedAttrsForChanges() returns a Bitmapset, identical to
HeapDetermineColumsInfo(), of the modified indexed columns. While similar
in spirit to HeapDetermineColumsInfo(), this new function also evaluates
expressions, and potentially calls into a new index AM API
amcomparedatums(). This is the key change that enables HOT updates when
expressions are unchanged on update.

The new optional index AM function amcomparedatums() allows index
implementations to define their own custom logic for determining if a
datum has changed and if that change requires that the index be updated
or not. This has been implemented for GIN and HASH.

The patch also updates the replication logic in execReplication() to
correctly provide the set of modified indexed attributes extracted from
the replicated tuples to avoid overhead of rediscovery later on.

Catalog tuple updates are unchanged and continue to use
simple_heap_update() which in turn calls HeapDetermineColumsInfo(). Note
that I proposed a separate patch set to remove HeapDetermineColumsInfo()
from simple_heap_update() by changing the way catalog tuples are updated,
but that has since been abandoned. See cf-6221 for more details on this.

0003: Removes the now-redundant index_unchanged_by_update() function. Its
functionality is superseded by simply reusing the Bitmapset created by
ExecWhichIndexesRequireUpdates() and stored in ri_ChangedIndexedCols.

0004: Moves the evaluation of partial index predicates from after the table
update to before it, within the ExecWhichIndexesRequireUpdates(). If both
the old and new tuples are outside the index's predicate, the index is not
considered impacted, and so the attributes in the predicate are not
recorded in the resulting Bitmapset potentially allowing for a HOT update.

PERFORMANCE TEST RESULTS:

Platform: FreeBSD 15 (Intel NUC)
Duration: 600 seconds (10 minutes) per-test, ~3.5 hours total
Baseline: 8ebdf41c262 (origin/master)
Patched: bdfe58c4537 (origin/master + 4 patches)

HOT Update Percentages:
────────────────────────────────────────────────────────────────
Test Baseline Patched Δ
────────────────────────────────────────────────────────────────
JSONB BTREE expression index 0.0% 95.0% 95.0%
JSONB GIN expression index 0.0% 98.1% 98.1%
Partial index 0.0% 96.3% 96.3%
3 expression indexes 0.0% 98.2% 98.2%
6 expression indexes 0.0% 100.0% 100.0%
Array expression index 0.0% 97.8% 97.8%
UPDATE 100 rows 0.0% 99.7% 99.7%
Control: indexed field changes 47.7% 48.2% .5%
Control: expensive expression 0.0% 0.0% 0%
Control: Induced TU_Updated 0.1% 0.1% 0%

Throughput (10 clients):
────────────────────────────────────────────────────────────────
Test Baseline Patched Δ
────────────────────────────────────────────────────────────────
JSONB BTREE expression index 563.6 543.2 -3.6%
JSONB GIN expression index 556.5 536.6 -3.5%
Partial index 415.6 406.4 -2.2%
3 expression indexes 553.6 535.1 -3.3%
6 expression indexes 528.9 511.8 -3.2%
Array expression index 560.7 542.6 -3.2%
UPDATE 100 rows 840.9 1066.7 26.8%
Control: indexed field changes 564.6 529.0 -6.3%
Control: expensive expression 563.4 527.3 -6.4%
Control: Induced TU_Updated 1677.5 1730.4 3.1%

Index Bloat After Updates (MB):
────────────────────────────────────────────────────────────────
Test Baseline Patched Reduction
────────────────────────────────────────────────────────────────
JSONB BTREE expression index 8.2 2.9 64.6%
JSONB GIN expression index 8.8 6.3 27.3%
Partial index 4.3 2.1 49.8%
3 expression indexes 13.4 4.1 68.7%
6 expression indexes 26.7 5.9 77.7%
Array expression index 12.8 4.3 66.5%
UPDATE 100 rows 453.5 6.9 98.4%
Control: indexed field changes 4.6 4.5 .6%
Control: expensive expression 27.2 25.9 4.8%
Control: Induced TU_Updated 7.9 7.9 0%

On Tue, Feb 10, 2026, at 10:09 AM, Greg Burd wrote:
> Hello,
>
> TL;DR, I'm going to put a pin in this idea for now.

Okay, I couldn't put the pen down after all. :)

Here's my thinking, this patch set can be thought of as:

a) moving HeapDetermineColumnsInfo() into the executor
b) all that HOT nonsense

I feel that (a) has value even without (b), that removing a chunk of work from within an exclusive buffer lock to outside that lock is a Good Thing(TM) and could in this case result in more concurrency.

To that end, present to you a single patch that *only* does (a), it moves the logic of HeapDeterminColumnsInfo() into the executor and doesn't change anything else. Meaning that what goes HOT today (without this patch), should continue to be HOT tomorrow (with this patch) and nothing else.

Catalog tuples use simple_heap_update() which calls HeapDeterminColumnsInfo() just as it does now, so these results are also identical.

Logically replicated tuples avoid HeapDeterminColumnsInfo() because the carry with them the set of changed attributes, so I surface that from within slot_modify_data() and intersect it with the set of indexed attributes resulting in an identical update while avoiding that overhead.

This has to run faster, I'll measure it ASAP and post, but I thought I'd share this now to potentially keep the ball rolling.

best.

-greg

PS: I'll layer in the additional changes in a future post that expand HOT, but those can be viewed in the context of "maybe in v20" while I hope that this patch could be potentially acceptable in v19.

On Fri, 2026-02-13 at 16:06 -0500, Greg Burd wrote:
> Here's my thinking, this patch set can be thought of as:
>
> a) moving HeapDetermineColumnsInfo() into the executor

This feels like the core of the series: moving the logic into the
executor make it possible to be smarter about whether HOT can be
applied or not.

I think that is a good direction to go. I don't think HOT is
fundamentally a heap concept because other AMs may do something similar
and would want similar information. There are two parts to the decision
of whether to use HOT: first, is there a logical change to the indexed
value; and second, does the AM need to break the change for some other
reason (e.g. the current page is full).

It seems reasonable to me that the executor is the right place for the
first check, because it can be more precise. The motivating example
here is a JSON document where one field is indexed and unrelated fields
are being updated, in which case we can still do a HOT update because
the indexed value isn't actually changing.

As far as the patch itself, it seems like you're moving a lot of code
out of heap_update() and into simple_heap_update() &
heapam_tuple_update(). Can you explain why that's needed? Perhaps I
just need to look closer.

> b) all that HOT nonsense
>
> I feel that (a) has value even without (b), that removing a chunk of
> work from within an exclusive buffer lock to outside that lock is a
> Good Thing(TM) and could in this case result in more concurrency.

Right. It would be nice to see some numbers here, but moving work
outside of the buffer lock seems like a good idea.

> To that end, present to you a single patch that *only* does (a), it
> moves the logic of HeapDeterminColumnsInfo() into the executor and
> doesn't change anything else.  Meaning that what goes HOT today
> (without this patch), should continue to be HOT tomorrow (with this
> patch) and nothing else.

Why are there test diffs?

Also, does this move us (slightly) closer to PHOT?

Regards,
Jeff Davis

On Sat, Feb 14, 2026, at 2:39 PM, Jeff Davis wrote:
> On Fri, 2026-02-13 at 16:06 -0500, Greg Burd wrote:
>> Here's my thinking, this patch set can be thought of as:
>>
>> a) moving HeapDetermineColumnsInfo() into the executor

Hey Jeff, thanks for taking a look at this! :)

> This feels like the core of the series: moving the logic into the
> executor make it possible to be smarter about whether HOT can be
> applied or not.

Yes, but put a different way this change moves what is not heap-specific outside the heap and leaves heap-isms inside heap.

> I think that is a good direction to go. I don't think HOT is
> fundamentally a heap concept because other AMs may do something similar
> and would want similar information.

The ability for a table AM to skip writing new index entries for a subset of updates is the non-heap-specific feature I'm making more general in this patch. Heap calls this "HOT" (heap-only tuple) but while the name is a heap-ism, the concept isn't. Other table AM implementations might have different behavior as they might implement MVCC differently, have UNDO logs, or even be Index-oriented tables (IoT). What's baked into the code today is very tightly aligned with how heap works. The first thing I'd like to adjust is the identification of the "modified indexed attributes" on update. The second will be the TU_UpdateIndexes enum.

> There are two parts to the decision
> of whether to use HOT: first, is there a logical change to the indexed
> value; and second, does the AM need to break the change for some other
> reason (e.g. the current page is full).

I view this as, "agreeing on which indexes require index writes during an update" and I think there are three parts of the system that work together to determine this:

1. indexes
2. types
3. tables

(1) Indexes influence this decision when they are summarizing. Otherwise it is assumed they only need updates when the table AM signals TU_All. I think indexes need more influence over this, but that's for a later commit.

(2) Types don't influence this decision today. Their equality operators are not used, attributes are memcmp(). This is a requirement for index-only scans. I think it's insufficient for types like JSONB which have internal structure that can be extracted to form index keys, but that's for a later commit.

(3) Tables, really only heap today, own this decision in our code as it stands now. The heap informs the executor: TU_All, TU_None, TU_Summarizing (all, none, some). The choice between these three outcomes is the "HOT decision" in the heap_update() code. All indexes are updated when any indexed attribute is modified. No indexes are updated when there are no modifications to indexed attributes. If only attributes referenced by summarizing indexes were modified then all summarizing indexes are updated (even those without modified attributes). Of course if the newly updated tuple (even after being compressed/TOASTed) won't fit on the same page as the old one then the result is TU_All. This is where HEAD's logic is today.

Future AMs, or even heap, might be able to:
- chain updates across pages
- serve as the primary key index as well as tuple storage (IoT)

Indexes might be able to:
- identify that they only index portions of the key datum provided
- ask types if that portion of the data changed or not

Types might be able to:
- record how they've been indexed on a relation
- record during mutation on update if the changes intersect with what's provided to indexes

But none of that is in this patch now. The only change that might be useful is to avoid updating unmodified summarized indexes on a HOT/summarized update. That's a simple addition on top of this patch, but not in the attached patch.

> It seems reasonable to me that the executor is the right place for the
> first check, because it can be more precise. The motivating example
> here is a JSON document where one field is indexed and unrelated fields
> are being updated, in which case we can still do a HOT update because
> the indexed value isn't actually changing.

Yes, JSON is a good example of how a type plays a role in deciding which indexes need updates. That's where this thread started, expression indexes on JSONB that are unchanged during update should allow HOT updates when unchanged (IMO).

> As far as the patch itself, it seems like you're moving a lot of code
> out of heap_update() and into simple_heap_update() &
> heapam_tuple_update(). Can you explain why that's needed? Perhaps I
> just need to look closer.

I can see how this might be confusing, you're asking a good question. Why not just add the mix_attrs as an argument to the table AM update call and be done?

1. Bitmapsets that are NULL mean empty, so how would simple_heap_update() signal to heap_update() that it needs to determine the modified indexed attributes? We'd have to add a bool along with the mix_attrs Bitmapset to indicate: "we've not calculated the set yet, you need to do that."

2. After fetching the exclusive buffer lock there is the test `!ItemIdIsNormal(lp)` to cover the case where a simple_heap_update() the otid origin is the syscache, there is no pin or snapshot, and so there might be LP_* states other than LP_NORMAL due to concurrent pruning. This only happens when updating catalog tuples, so this logic need not be present at all in the heapam_tuple_update(). Yes, the if() branch will be fast (frequently predicted by the CPU) but this feels like logic specific to the update of catalog tuples.

3. HeapDetermineColumnsInfo() actually does more than find the modified indexed attributes, it also performs half of the check for the requirement to WAL log the replica identity attributes. The replacement function in the executor doesn't do this work, so that is coded into heapam_tuple_update() but not simple_heap_update(). The second half is in ExtractReplicaIdentity() that happens later in the heap_update() function after determining if HOT is a possibility or not.

I have moved these changes back into heap_update(), add the mix_attrs and mix_attrs_valid to see how things look, that's the attached patch.

>> b) all that HOT nonsense
>>
>> I feel that (a) has value even without (b), that removing a chunk of
>> work from within an exclusive buffer lock to outside that lock is a
>> Good Thing(TM) and could in this case result in more concurrency.
>
> Right. It would be nice to see some numbers here, but moving work
> outside of the buffer lock seems like a good idea.

No numbers yet.

>> To that end, present to you a single patch that *only* does (a), it
>> moves the logic of HeapDeterminColumnsInfo() into the executor and
>> doesn't change anything else.  Meaning that what goes HOT today
>> (without this patch), should continue to be HOT tomorrow (with this
>> patch) and nothing else.
>
> Why are there test diffs?

Removed test differences.

> Also, does this move us (slightly) closer to PHOT?

I'd argue that it does move the code closer to something WARM/PHOT-like meaning an ability to only update a subset of indexes on update; a change from "all/none/some" to "the required subset, and no more." That's a ways off.

> Regards,
> Jeff Davis

thanks again for looking Jeff,

-greg

On Sun, 2026-02-15 at 15:39 -0500, Greg Burd wrote:

> (2) Types don't influence this decision today.  Their equality
> operators are not used, attributes are memcmp().  This is a
> requirement for index-only scans.  I think it's insufficient for
> types like JSONB which have internal structure that can be extracted
> to form index keys, but that's for a later commit.

That's an interesting path but would require more infrastructure, and
to justify going down that path we should look for other opportunities
to use that type infra beyond just HOT. Brainstorming: maybe something
in the planner can make use of intelligence around expressions that are
effectively setter/accessor methods on complex types? (Obviously work
for later.)

> I can see how this might be confusing, you're asking a good
> question.  Why not just add the mix_attrs as an argument to the table
> AM update call and be done?
>
> 1. Bitmapsets that are NULL mean empty, so how would
> simple_heap_update() signal to heap_update() that it needs to
> determine the modified indexed attributes?  We'd have to add a bool
> along with the mix_attrs Bitmapset to indicate: "we've not calculated
> the set yet, you need to do that."

Maybe an extra bool is not ideal, but it's better than moving code onto
the wrong side of an API boundary.

> 2. After fetching the exclusive buffer lock there is the test
> `!ItemIdIsNormal(lp)` to cover the case where a simple_heap_update()
> the otid origin is the syscache, there is no pin or snapshot, and so
> there might be LP_* states other than LP_NORMAL due to concurrent
> pruning.  This only happens when updating catalog tuples, so this
> logic need not be present at all in the heapam_tuple_update().  Yes,
> the if() branch will be fast (frequently predicted by the CPU) but
> this feels like logic specific to the update of catalog tuples.

If convenient I'm fine with moving that branch out, but I think it
needs to be done with the buffer locked (right?), so heap_update()
looks like the right place for that test for now.

> 3. HeapDetermineColumnsInfo() actually does more than find the
> modified indexed attributes, it also performs half of the check for
> the requirement to WAL log the replica identity attributes.  The
> replacement function in the executor doesn't do this work, so that is
> coded into heapam_tuple_update() but not simple_heap_update().  The
> second half is in ExtractReplicaIdentity() that happens later in the
> heap_update() function after determining if HOT is a possibility or
> not.

IIUC you are saying that the decision is too heap-specific to expose to
the executor. I think that's true today (as ExtractReplicaIdentity() is
in heapam.c), but perhaps that's not fundamental: TOAST is not heap-
specific, replica IDs are not heap-specific, and if you are WAL-logging
a replica identity key it seems like you need to know whether it's
external or not regardless of the AM.

I'm not asking for a change here, just trying to understand the API
boundaries.

> I have moved these changes back into heap_update(), add the mix_attrs
> and mix_attrs_valid to see how things look, that's the attached
> patch.

Thank you -- that's easier to understand.

Why does simple_heap_update() need to do the HeapDetermineColumnsInfo()
inside heap_update()? It seems like you're trying to avoid doing the
same work the executor is doing to determine the modified_attrs bitmap,
but either (a) the work is cheap; or (b) the work to make the bitmap is
expensive.

If (a), then just construct the correct bitmap in simple_heap_update()
and simplify the code. If (b), then optimizing the simple_heap_update()
case isn't good enough, we need to find ways of avoiding the work in
the most common cases in the executor, as well.

Regards,
Jeff Davis

On Mon, Feb 16, 2026, at 2:36 PM, Jeff Davis wrote:
> On Sun, 2026-02-15 at 15:39 -0500, Greg Burd wrote:
>
>> (2) Types don't influence this decision today.  Their equality
>> operators are not used, attributes are memcmp().  This is a
>> requirement for index-only scans.  I think it's insufficient for
>> types like JSONB which have internal structure that can be extracted
>> to form index keys, but that's for a later commit.
>
> That's an interesting path but would require more infrastructure, and
> to justify going down that path we should look for other opportunities
> to use that type infra beyond just HOT. Brainstorming: maybe something
> in the planner can make use of intelligence around expressions that are
> effectively setter/accessor methods on complex types? (Obviously work
> for later.)

Agreed, but this is my best idea at the moment to re-introduce $subjet into this patch set. I'm open to other ideas, but fundamentally to allow $subjet requires that somewhere we discover that while portions of the JSONB attribute changed during the update the resulting index key attributes extracted from the JSONB did not.

In all patches v1-v29 that was discovered by evaluating the index expressions on the old and new tuples and comparing the index key datum that resulted for equality. When equal, while the attribute changed the indexes need not opening the door for a HOT update. The overhead of that wasn't horrible, but it does change some expectations about how often expressions are evaluated and that might be confusing.

>> I can see how this might be confusing, you're asking a good
>> question.  Why not just add the mix_attrs as an argument to the table
>> AM update call and be done?
>>
>> 1. Bitmapsets that are NULL mean empty, so how would
>> simple_heap_update() signal to heap_update() that it needs to
>> determine the modified indexed attributes?  We'd have to add a bool
>> along with the mix_attrs Bitmapset to indicate: "we've not calculated
>> the set yet, you need to do that."
>
> Maybe an extra bool is not ideal, but it's better than moving code onto
> the wrong side of an API boundary.

I'm not sure this is an "API boundary", but I'm open to debate on that. The attached patch does add a bool to heap_update() in addition to the Bitmapset of modified/indexed attributes (modified_attrs) and IMO is fine although I don't like that it further complicates (muddies?) an already huge and highly complicated function, heap_update().

>> 2. After fetching the exclusive buffer lock there is the test
>> `!ItemIdIsNormal(lp)` to cover the case where a simple_heap_update()
>> the otid origin is the syscache, there is no pin or snapshot, and so
>> there might be LP_* states other than LP_NORMAL due to concurrent
>> pruning.  This only happens when updating catalog tuples, so this
>> logic need not be present at all in the heapam_tuple_update().  Yes,
>> the if() branch will be fast (frequently predicted by the CPU) but
>> this feels like logic specific to the update of catalog tuples.
>
> If convenient I'm fine with moving that branch out, but I think it
> needs to be done with the buffer locked (right?), so heap_update()
> looks like the right place for that test for now.
>
>> 3. HeapDetermineColumnsInfo() actually does more than find the
>> modified indexed attributes, it also performs half of the check for
>> the requirement to WAL log the replica identity attributes.  The
>> replacement function in the executor doesn't do this work, so that is
>> coded into heapam_tuple_update() but not simple_heap_update().  The
>> second half is in ExtractReplicaIdentity() that happens later in the
>> heap_update() function after determining if HOT is a possibility or
>> not.
>
> IIUC you are saying that the decision is too heap-specific to expose to
> the executor. I think that's true today (as ExtractReplicaIdentity() is
> in heapam.c), but perhaps that's not fundamental: TOAST is not heap-
> specific, replica IDs are not heap-specific, and if you are WAL-logging
> a replica identity key it seems like you need to know whether it's
> external or not regardless of the AM.
>
> I'm not asking for a change here, just trying to understand the API
> boundaries.

I'm on the fence on this one, maybe all table AMs will need this same logic but at the moment it feels very heap-specific to me. For now it lives in heap_update() and HeapDetermineColumnsInfo(). I think I called this out only to say that if you look at the new vs old method for computing modified_attrs you'll see that's missing and done later in heap_update().

>> I have moved these changes back into heap_update(), add the mix_attrs
>> and mix_attrs_valid to see how things look, that's the attached
>> patch.
>
> Thank you -- that's easier to understand.

Excellent.

I've muddied the review again a bit by abstracting out a function ExecCompareSlots() which can loosely be thought of as a replacement for heap_attr_equal(). Doing that let's me vastly simplify the replication worker changes and reuse it after ExecBRUpdateTriggers().

> Why does simple_heap_update() need to do the HeapDetermineColumnsInfo()
> inside heap_update()? It seems like you're trying to avoid doing the
> same work the executor is doing to determine the modified_attrs bitmap,
> but either (a) the work is cheap; or (b) the work to make the bitmap is
> expensive.

simple_heap_update() is exclusively called during catalog tuple updates and does not involve the executor at all, these are direct calls into heap to store catalog tuples. These updates don't preserve the set of updated attributes (see cf-6221 [0]) and so for them to potentially use the HOT path in heap_update() we need to identify which attributes changed. This requires comparing the new heap tuple vs the one read from the buffer page. This is the same logic as before the patch. If I could come up with a simple method to retain the set of modified attributes during catalog tuple updates then we could excise HeapDetermineColumnsInfo() entirely.

> If (a), then just construct the correct bitmap in simple_heap_update()
> and simplify the code. If (b), then optimizing the simple_heap_update()
> case isn't good enough, we need to find ways of avoiding the work in
> the most common cases in the executor, as well.

Construct the correct bitmap how? That function is called with the otid and the updated HeapTuple, not enough to build the bitmap of modified indexed attributes.

I said in an earlier email that the results for the modified/indexed bitmap and the replica identity are identical. I added some test code (messy, but attached for your amusement should you want) that looks for differences between the two. It turns out that for replica identity the results are identical, for modified/indexed attributes there are a few differences.

In brin.sql at 409:
UPDATE brintest SET int8col = int8col * int4col;

This updates the indexed value of the int8col (attribute 4) which is correctly detected in the new code, but isn't in HeapDetermineColumnsInfo(). The "interesting_cols" are identical. I'm still investigating.

I'm also working on a good benchmark that hopefully can show that under heavy concurrent read/update mixed load the reduced exclusive lock time will allow more work to be performed. Some contrived benchmarks have shown 15% improvement, but I need to zero in on this before publishing results.

> Regards,
> Jeff Davis

Attached is a new version of the patch. The file heap-check.c contains the code I had been using to test for equal results across methods. That code is... well, it's just for me but I include it to show that I'm doing due diligence to ensure this stuff is either the same, or different for a good reason. Thanks for your continued interest in this work.

best.

-greg

[0] https://commitfest.postgresql.org/patch/6221/

Hello,

This is an updated version of the last patch with a few fixes and a layer on top of it that tries to cleanup heap_update().

v20260219:

0001 - Not much changed in this patch, some clean up and fixed a few mistakes. This patch passes all tests without any need to modify them. I've added ExecCompareSlotAttrs() helper function.

0002 - As before in v29 I've split off the top half of heap_update() and moved that into heapam_tuple_update() and simple_heap_update(). I've created helper functions for different steps in these early stages: HeapUpdateHotAllowable(), HeapUpdateRequiresReplicaId(), HeapUpdateDetermineLockmode(). This allows for a cleaner set of bitmaps and logic (as I read it) on the heapam_tuple_update() path. I reuse these helper functions in simple_heap_update() when possible, even trimming up HeapDeterminColumnsInfo() a bit so as to reuse HeapUpdateRequiresReplicaId().

I've tested with code that validates in heapam_tuple_update() that the modified attr bitmaps are identical:

{
Bitmapset *hot_attrs = RelationGetIndexAttrBitmap(relation,
INDEX_ATTR_BITMAP_INDEXED);
Bitmapset *id_attrs = RelationGetIndexAttrBitmap(relation,
INDEX_ATTR_BITMAP_IDENTITY_KEY);
Bitmapset *hdci_attrs = HeapDetermineColumnsInfo(relation, hot_attrs,
&oldtup, tuple);

Assert(bms_equal(mix_attrs, hdci_attrs));
bms_free(hot_attrs);
bms_free(id_attrs);
bms_free(hdci_attrs);
}

Despite that passing two tests became non-deterministic without an "ORDER BY" on a select. You'll see those in generated_virtual.sql and updatable_views.sql in the second patch. I don't know yet why that happened, but the results are otherwise identical.

I will continue to performance test. Most things I've tried differ by less than 0.5% before/after this patch. Some operations where multiple rows are matched in an UPDATE and there are concurrent reads are faster (10-20%), I need to dig into this. I'm expanding the tests I'm running to try to find any cases where holding the buffer lock for less time could possibly result in higher TPS. The goals for $subject were faster TPS, but more importantly to lower index bloat and help shorten vacuum times.

Next up I'll work on re-introducing some of the other work from $subject and other changes in v29 and earlier patch sets.

* avoid the need for index_unchanged_by_update()
* re-add the new index AM function, "amcomparedatums()" or similar
* add a flag for types indicating that they have "sub-attributes" (JSONB, XML, ARRAY)
* store in pg_attribute during CREATE INDEX when types with sub-attributes are in expressions the relation and some representation of what the sub-attribute is
* update JSONB functions that mutate content to use the pg_attribute information and record if there were changes to indexed "sub-attributes" or not
* use that recorded information later in the executor to identify if the indexed sub-attribute changed or not opening the door for $subject without evaluating the before/after expressions
* re-examine partial indexes as well
* consider how one might layer a PHOT/WARM-thingie on this... (in a different thread in the future, like next year)

best.

-greg

Hi,

On 2026-02-17 16:15:02 -0500, Greg Burd wrote:
> > Why does simple_heap_update() need to do the HeapDetermineColumnsInfo()
> > inside heap_update()? It seems like you're trying to avoid doing the
> > same work the executor is doing to determine the modified_attrs bitmap,
> > but either (a) the work is cheap; or (b) the work to make the bitmap is
> > expensive.
>
> simple_heap_update() is exclusively called during catalog tuple updates and
> does not involve the executor at all, these are direct calls into heap to
> store catalog tuples.

Just FYI, there are probably a fair number of extensions using
simple_heap_update(). That number used to be a lot higher, but I don't think
there should be a hard assumption about it just being used for catalog updates
in the code.

Greetings,

Andres Freund

On Thu, Feb 19, 2026, at 3:43 PM, Andres Freund wrote:
> Hi,
>
> On 2026-02-17 16:15:02 -0500, Greg Burd wrote:
>> > Why does simple_heap_update() need to do the HeapDetermineColumnsInfo()
>> > inside heap_update()? It seems like you're trying to avoid doing the
>> > same work the executor is doing to determine the modified_attrs bitmap,
>> > but either (a) the work is cheap; or (b) the work to make the bitmap is
>> > expensive.
>>
>> simple_heap_update() is exclusively called during catalog tuple updates and
>> does not involve the executor at all, these are direct calls into heap to
>> store catalog tuples.

Hey Andres,

> Just FYI, there are probably a fair number of extensions using
> simple_heap_update(). That number used to be a lot higher, but I don't think
> there should be a hard assumption about it just being used for catalog updates
> in the code.

Makes sense. These patches don't change simple_heap_update() in any functional way and so extensions should be fine.

> Greetings,
>
> Andres Freund

best.

-greg

Hello.

Attached is a new patch set that fixes a few issues identified in the last set.

0001 - creates a new way to identify the set of attributes both modified by the update and referenced by one or more indexes on the target relation being updated. This patch keeps the HeapDetermineColumnsInfo() path within heap_update() for calls from simple_heap_update() when modified_attrs_valid is set to false. I'm not a huge fan of this, but it does serve as a way to illustrate a minimal set of changes easing review a bit.

0002 - splits out the top portion of heap_update() into both heapam_tuple_update() and simple_heap_update(), adds a few helper functions and tries to reduce repeated code. The goal here was to remove some of the mess related to the various bitmaps used to make decisions during the update.

Performance tests so far haven't shown a regression of note for this set of changes.

I'm still working on:
a) cleaning this up a bit more
b) create ExecCheckUpdateRequiresReplicaId() in executor
c) look for a way to cleanly pass/maintain per-table AM state during update
d) root cause for difference in tests
e) look into UPDATE WHERE > 1 row performance

best.

-greg

On Mon, 2026-02-23 at 14:23 -0500, Greg Burd wrote:
> Hello.
>
> Attached is a new patch set that fixes a few issues identified in the
> last set.
>
> 0001 - creates a new way to identify the set of attributes both
> modified by the update and referenced by one or more indexes on the
> target relation being updated.  This patch keeps the
> HeapDetermineColumnsInfo() path within heap_update() for calls from
> simple_heap_update() when modified_attrs_valid is set to false.  I'm
> not a huge fan of this, but it does serve as a way to illustrate a
> minimal set of changes easing review a bit.
>
> 0002 - splits out the top portion of heap_update() into both
> heapam_tuple_update() and simple_heap_update(), adds a few helper
> functions and tries to reduce repeated code.  The goal here was to
> remove some of the mess related to the various bitmaps used to make
> decisions during the update.

IIUC, a minimal version of this patch set might be:

* add 'mix_attrs' bitmap to API for table_tuple_update
* have executor calculate the bitmap, using the old slot to see if
expression results have changed
* have simple_heap_update calculate the bitmap using heap_fetch to get
the old tuple (would be a redundant pin, but not sure if that's a
problem or not)

And leave the rest mostly unchanged.

Did I miss something? If not, it would be nice to see such a minimal
patch and/or understand why we don't follow that approach.

Regards,
Jeff Davis

On Wed, Feb 25, 2026, at 4:03 PM, Jeff Davis wrote:
> On Mon, 2026-02-23 at 14:23 -0500, Greg Burd wrote:
>> Hello.
>>
>> Attached is a new patch set that fixes a few issues identified in the
>> last set.
>>
>> 0001 - creates a new way to identify the set of attributes both
>> modified by the update and referenced by one or more indexes on the
>> target relation being updated.  This patch keeps the
>> HeapDetermineColumnsInfo() path within heap_update() for calls from
>> simple_heap_update() when modified_attrs_valid is set to false.  I'm
>> not a huge fan of this, but it does serve as a way to illustrate a
>> minimal set of changes easing review a bit.
>>
>> 0002 - splits out the top portion of heap_update() into both
>> heapam_tuple_update() and simple_heap_update(), adds a few helper
>> functions and tries to reduce repeated code.  The goal here was to
>> remove some of the mess related to the various bitmaps used to make
>> decisions during the update.
>
> IIUC, a minimal version of this patch set might be:
>
> * add 'mix_attrs' bitmap to API for table_tuple_update
> * have executor calculate the bitmap, using the old slot to see if
> expression results have changed
> * have simple_heap_update calculate the bitmap using heap_fetch to get
> the old tuple (would be a redundant pin, but not sure if that's a
> problem or not)
>
> And leave the rest mostly unchanged.
>
> Did I miss something? If not, it would be nice to see such a minimal
> patch and/or understand why we don't follow that approach.

Hey Jeff, thanks for sticking with me on this journey. :)

I think your approach makes sense, here's a summary of what's attached (v30) and at the bottom of this email are some early performance measurements.

* in the executor
* identify the mix_attrs
* one new argument to table_tuple_update( ..., mix_attrs, ...)
* heapam_tuple_update( ..., mix_attrs, ...)
* calculates hot_allowed using mix_attrs
* calculates lockmode using key_attrs and mix_attrs
* two new arguments to heap_update(..., mix_attrs, hot_allowed, ...)
* on return determines what to do with TU_UpdateIndexes
* heap_update( ..., mix_attrs, hot_allowed, ... )
* takes buffer lock
* calculates rep_id_key_req, passes that to ExtractReplicaId()
* if newbuf==buffer && hot_allowed -> HOT
* releases buffer lock

* simple_heap_update( ... no changes to API ... )
* now needs to compare old/new tuples *BEFORE* calling heap_update()
* uses heap_fetch() to turn otid -> oldtuple
* calls HeapUpdateModIdxAttrs()
* calculates lockmode
* calculates if hot is allowed
* calls into heap_update(..., mix_attrs, hot_allowed, ...)
* on return determines what to do with TU_UpdateIndexes
* renamed HeapDetermineColumnsInfo() to HeapUpdateModIdxAttrs()
* removed logic related to rep_id_key_req, that is in heap_update()

> Regards,
> Jeff Davis

There are a pair of functions now for finding "mix_attrs" that replace the singular HeapDetermineColumnsInfo() function:
ExecUpdateModIdxAttrs()
HeapUpdateModIdxAttrs()
These do essentially the same thing, only with different available information and where the latter is called within the context of a buffer lock.

In ExecUpdateModIdxAttrs() we compare two TupleTableSlots, the existing and the plan slot, using a new helper function ExecCompareSlotAttrs(). This gives us the "mix_attrs" (modified indexed attributes) bitmap. In this function we have the ResultRelInfo and EState so it is possible to use the ExecGetAllUpdatedCols() function to potentially reduce the set of attributes we need to check for changes. The function only reviews indexed attributes that also exist in that set, which led to an interesting discovery... see below.

In HeapUpdateModIdxAttrs() we start with an old TID and a HeapTuple, so first we need to fetch that old HeapTuple so we can compare old/new datum and find any modified indexed attributes.

A new function HeapUpdateHotAllowable() is used in heapam_tuple_update() and simple_heap_update() encapsulating that logic in one place including the "only summarized" test. Heap will use the HOT path if that function returns true and the tuple fits on the same buffer page. No logic changes, just moved the decision making around a bit.

A new function HeapUpdateDetermineLockMode() is used to choose exclusive or shared lock mode ahead of calling into heap_update(). Again, same logic as before.

It turns out that ExecGetAllUpdatedCols() doesn't get all updated columns as the name advertises. It finds all the attributes (columns) that were mentioned in the UPDATE statement or any triggers that will fire during the update, but that overlooks any attributes changed within before-row triggers that invoke functions which call heap_modify_tuple(). This happens when tsvector_update_trigger() is called in tsearch.sql, the code modifies an indexed attribute not mentioned in the UPDATE or triggers. I've fixed this oversight and to me this makes sense, but tell me if you disagree.

generated_virtual.sql and updatable_views.sql had tests where the scan order of the tuples on the pages seems to now be non-deterministic. I've updated those tests to ensure stability. AFAICT my changes in this patch should not change any HOT decision or any replica identity key WAL logging decision, but somehow they uncovered this instability. Or there is a bug, but I've not spotted that as yet. Feel free to point out the obvious if you do. :)

Just to be clear, this patch doesn't include any of $subject. In tests I've not measured any performance regressions, and that's not surprising as the sum total computational effort is nearly identical before/after the patch. Yes, the patch moves some computation outside the buffer lock on the heap page and that might open the door to more concurrency or slightly different behavior when updates are highly contentious. There may be more occasions where TU_Updated is returned, or some speed improvements when updating more than one row at a time.

My hope is to get this into a shape where we're comfortable with these changes and it can be committed even though none of $subject is achieved because it does lay some ground work for those future HOT expanding and WARM/PHOT enabling ideas I've been working on.

Things on my TODO list, short term:
* Re-introduce the index AM's new function to allow indexes to play a role in when they require new index entries
* I'm not a fan of TU_UpdateIndexes, it's *very* heap-specific, I'd like to eliminate this

Longer term, so as to return to working on $subject:
* Allow types to indicate that they maintain "sub-attributes" that might be used to form index key datum
* Identify in the executor for each attribute SET if a) it has sub-attributes, and if so b) does the new value for the attribute change any sub-attribute that is used to form index keys
* With the previous two ideas I think we can safely re-introduce HOT for expressions without re-evaluating the expressions and comparing index datum (read: without the overhead I've measured in the past)
* PHOT or WARM or <other nifty name here>, teach heap how to only update changed indexes (rather than the all or nothing approach we have today)

I look forward to community feedback.

best.

-greg

----------------- PERFORMANCE TEST RESULTS

DISCLAIMER: "claude" and I worked on the perf-cf5556-v30.sh script, as I'm sure is apparent. I think people call it "vibe coding" when you try to contain the enthusiasm of your friendly LLM and direct it toward some goal. IME it's like trying to control a room full of dangerously knowledgeable and overly eager to please kindergarten-aged parrots. I admit to needing more time to review the script, the test cases, and the results to fully explore these changes and validate that they actually measure something meaningful. If you find something silly or a glaring mistake, go easy on me (and "claude") but do let me (us?) know.

$ ./perf-cf5556-v30.sh
Checking for running PostgreSQL instances...
✓ No other PostgreSQL instances running

╔════════════════════════════════════════════════════════════════════╗
║ CF-5556 PERFORMANCE TEST SUITE
╚════════════════════════════════════════════════════════════════════╝
Configuration:
Test duration : 60s
Clients / Jobs : 8 / 4
Results directory : /tmp/cf5556-perf-results/20260226_150623
Setup extensions : NO
Test extensions : NO

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
BUILDING AND TESTING
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Baseline: d0833fdae7e (origin/master)
Patches: 1 patch(es) to test cumulatively

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
VERSION: baseline (d0833fdae7e)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Building PostgreSQL...
✓ PostgreSQL built
Starting server...
✓ Server started
shared_preload_libraries: pg_stat_statements
Setting up test databases...
Creating driver_license table (100k rows, 5 BTREE indexes)...
✓ driver_license ready (100000 rows)
Creating t_jsonb table (10k rows, 3 BTREE expression indexes)...
✓ t_jsonb ready (10k rows)
Creating t_gin table (10k rows, GIN index — control)...
✓ t_gin ready (10k rows, GIN — control)

Running isolated tests (60s each)...

license_write_single TPS: 69816.475176 Lat: 0.115ms
jsonb_write_single TPS: 56571.238721 Lat: 0.141ms
jsonb_write_batch TPS: 3606.918699 Lat: 2.218ms
gin_write_single TPS: 64040.989986 Lat: 0.125ms
pgbench_tpcb-like TPS: 20133.238199 Lat: 0.397ms
pgbench_simple-update TPS: 19219.239741 Lat: 0.416ms

Running concurrent read/write tests...

Running concurrent test: 2 writers + 6 readers...
jsonb_2w_6r Write: 14776.018733 TPS Read: 80627.112253 TPS
Write: 0.135 ms Read: 0.074 ms
Running concurrent test: 4 writers + 4 readers...
jsonb_4w_4r Write: 29399.436764 TPS Read: 52688.734439 TPS
Write: 0.136 ms Read: 0.076 ms
Running concurrent test: 6 writers + 2 readers...
jsonb_6w_2r Write: 43151.037340 TPS Read: 25295.378828 TPS
Write: 0.139 ms Read: 0.079 ms
Running concurrent test: 2 writers + 6 readers...
license_2w_6r Write: 18891.968944 TPS Read: 74113.652071 TPS
Write: 0.106 ms Read: 0.081 ms
Running concurrent test: 4 writers + 4 readers...
license_4w_4r Write: 37305.015382 TPS Read: 48489.296785 TPS
Write: 0.107 ms Read: 0.082 ms
Running concurrent test: 6 writers + 2 readers...
license_6w_2r Write: 54403.687584 TPS Read: 23519.461792 TPS
Write: 0.110 ms Read: 0.085 ms

Stopping server...
✓ Server stopped

fatal: a branch named 'cf-5556-test-all-patches' already exists
Applying all 1 patches cumulatively...
Applying v20260226b-0001-Idenfity-modified-indexed-attributes-in-t.patch...

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
VERSION: patched (526c2a8733d)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Building PostgreSQL...
✓ PostgreSQL built
Starting server...
✓ Server started
shared_preload_libraries: pg_stat_statements
Setting up test databases...
Creating driver_license table (100k rows, 5 BTREE indexes)...
✓ driver_license ready (100000 rows)
Creating t_jsonb table (10k rows, 3 BTREE expression indexes)...
✓ t_jsonb ready (10k rows)
Creating t_gin table (10k rows, GIN index — control)...
✓ t_gin ready (10k rows, GIN — control)

Running isolated tests (60s each)...

license_write_single TPS: 70093.895595 Lat: 0.114ms
jsonb_write_single TPS: 56751.907107 Lat: 0.141ms
jsonb_write_batch TPS: 4141.086856 Lat: 1.932ms
gin_write_single TPS: 63845.491951 Lat: 0.125ms
pgbench_tpcb-like TPS: 19911.229480 Lat: 0.402ms
pgbench_simple-update TPS: 19840.566625 Lat: 0.403ms

Running concurrent read/write tests...

Running concurrent test: 2 writers + 6 readers...
jsonb_2w_6r Write: 14821.571968 TPS Read: 81057.390470 TPS
Write: 0.135 ms Read: 0.074 ms
Running concurrent test: 4 writers + 4 readers...
jsonb_4w_4r Write: 29428.063408 TPS Read: 52533.626129 TPS
Write: 0.136 ms Read: 0.076 ms
Running concurrent test: 6 writers + 2 readers...
jsonb_6w_2r Write: 43204.958598 TPS Read: 25301.939523 TPS
Write: 0.139 ms Read: 0.079 ms
Running concurrent test: 2 writers + 6 readers...
license_2w_6r Write: 18958.924353 TPS Read: 74548.095482 TPS
Write: 0.105 ms Read: 0.080 ms
Running concurrent test: 4 writers + 4 readers...
license_4w_4r Write: 37185.369146 TPS Read: 48580.299936 TPS
Write: 0.108 ms Read: 0.082 ms
Running concurrent test: 6 writers + 2 readers...
license_6w_2r Write: 54461.228141 TPS Read: 23692.388873 TPS
Write: 0.110 ms Read: 0.084 ms

Stopping server...
✓ Server stopped

╔════════════════════════════════════════════════════════════════════╗
║ RESULTS SUMMARY
╚════════════════════════════════════════════════════════════════════╝

═══════════════════════════════════════════════════════════════════════════════════
ISOLATED WORKLOAD COMPARISON (Patched vs Baseline)
═══════════════════════════════════════════════════════════════════════════════════
Table Workload Baseline TPS Patched TPS Δ%
───────────────────────────────────────────────────────────────────────────────────
gin write_single 64041.0 63845.5 -0.3%
jsonb write_batch 3606.9 4141.1 +14.8%
jsonb write_single 56571.2 56751.9 +0.3%
license write_single 69816.5 70093.9 +0.4%
pgbench simple-update 19219.2 19840.6 +3.2%
pgbench tpcb-like 20133.2 19911.2 -1.1%
───────────────────────────────────────────────────────────────────────────────────

═══════════════════════════════════════════════════════════════════════════════════
CONCURRENT WORKLOAD ANALYSIS (Write Pressure Impact on Reads)
═══════════════════════════════════════════════════════════════════════════════════
Table Write:Read Base Write Patch Write Base Read Patch Read
───────────────────────────────────────────────────────────────────────────────────
jsonb 2w_6r 14776.0 14821.6 80627.1 81057.4
license 2w_6r 18892.0 18958.9 74113.7 74548.1
jsonb 4w_4r 29399.4 29428.1 52688.7 52533.6
license 4w_4r 37305.0 37185.4 48489.3 48580.3
jsonb 6w_2r 43151.0 43205.0 25295.4 25301.9
license 6w_2r 54403.7 54461.2 23519.5 23692.4
───────────────────────────────────────────────────────────────────────────────────

Output files:
/tmp/cf5556-perf-results/20260226_150623/results.txt (raw results)
/tmp/cf5556-perf-results/20260226_150623/*_server.log (server startup/error logs)
/tmp/cf5556-perf-results/20260226_150623/*_setup.log (database setup logs)
/tmp/cf5556-perf-results/20260226_150623/*_build.log (build logs)
/tmp/cf5556-perf-results/20260226_150623/*_*.txt (pgbench output)
/tmp/cf5556-perf-results/20260226_150623/*_*.sql (test queries)

Cleaning up...
✓ Cleanup complete

Okay, here's hoping that the CI bot likes v31. :)

-greg

Hello Jeff, hackers,

In v33 I've updated a test in triggers.sql to address differences across platforms identified by the cf-bot and rebased the work.

I thought it might be prudent to add tests that validate all the corner cases of HOT that I could come up with, maybe too many (you tell me). In addition, because code that impacts HOT is also involved in what is WAL logged for the purposes of logical replication, I've added tests that try to explore the corners of that too. The goal of these first few patches is to NOT change the behavior of these things, but to only move the logic into the executor and out of heap then it makes sense to validate that explicitly.

At some point when I get back to $subjet I'll want to document how things changed. The best way to do that is by changing tests along with code. So, that is "0001" in this v33 patch set.

I've also run longer performance tests which show minimal performance differences between master and patched.

Workload 60s 300s 600s
jsonb_write_batch +14.8% -7.0% +0.1%
jsonb_write_single +0.3% +0.2% -0.0%
license_write_single +0.4% +0.2% -0.1%
gin_write_single -0.3% -0.2% -0.4%
pgbench_simple-update +3.2% +6.2% +0.9%
pgbench_tpcb-like -1.1% +0.8% -2.0%

Changing tests isn't something I take lightly, I dug into this quite a bit. I ran an analysis of ALL regression tests comparing master vs patched after instrumenting the code (see below) so I could record HOT and replica identity decisions and record where the tuple landed on the page.

Patched code produced:
simple_heap_update: 17,028 calls (72.5% - catalog updates, direct heap ops)
heapam_tuple_update: 6,462 calls (27.5% - executor path via table AM)
Total entry points: 23,490

This matched master's log line output for the same tests.

Replica identity decisions were identical, 342 unique patterns with 0 differences.

HOT eligibility was also identical, 398 unique patterns matched, again 0 differences.

The physical placement of tuples on pages was 99.991% identical, only 2 of 23,473 updates had different buffer placement.

Across test runs there were a few differences noted for pg_sequence, target, and wslot. Both master and patched agreed on hot_allowed=1 (logic identical), but in some cases use_hot_update differed (buffer placement, newbuf =?= buffer). To me this reads as non-deterministic behavior, not a bug introduced in this patch.

At this point I'd say that v33 patch is functionally correct and performance neutral. This set of changes isn't exactly exciting on the surface, but I feel that it opens the door to other changes that will be more interesting/valuable down the line.

Thank you for your time and interest.

best.

-greg

COMPARISON TESTING NOTES:
---------------------------------------------------------------------------------------
src/backend/access/heap/heapam.c
3514: elog(LOG, "PATCHED heap_update (replica check): rel=%s otid=(%u,%u) rep_id_key_required=%d",
3515- RelationGetRelationName(relation),
3516- ItemPointerGetBlockNumber(otid),
3517- ItemPointerGetOffsetNumber(otid),
3518- rep_id_key_required);
3519-
--
4106: elog(LOG, "PATCHED heap_update (final HOT): rel=%s otid=(%u,%u) hot_allowed=%d newbuf==buffer=%d use_hot_update=%d",
4107- RelationGetRelationName(relation),
4108- ItemPointerGetBlockNumber(otid),
4109- ItemPointerGetOffsetNumber(otid),
4110- hot_allowed, (newbuf == buffer), use_hot_update);
4111-
--
4693: elog(LOG, "PATCHED simple_heap_update: rel=%s otid=(%u,%u) hot_allowed=%d summarized_only=%d lockmode=%d",
4694- RelationGetRelationName(relation),
4695- ItemPointerGetBlockNumber(otid),
4696- ItemPointerGetOffsetNumber(otid),
4697- hot_allowed, summarized_only, lockmode);
4698-

src/backend/access/heap/heapam_handler.c
333: elog(LOG, "PATCHED heapam_tuple_update: rel=%s otid=(%u,%u) hot_allowed=%d summarized_only=%d lockmode=%d",
334- RelationGetRelationName(relation),
335- ItemPointerGetBlockNumber(otid),
336- ItemPointerGetOffsetNumber(otid),
337- hot_allowed, summarized_only, *lockmode);

Hello again,

Attached is v35 (master(at)f4a4ce52c0d) where I've separated out changes into three patches. Still nothing related to $subject directly, but foundational for that work (coming soon). I'd like to get these into v19 if at all possible and then target the rest of $subject for v20 so that it has more time to soak.

0001 - This patch adds tests to validate and capture the expected behavior of Heap-only tuple (HOT) updates. This also serves as a foundation that will aide in documenting what exactly changed in the commits implementing $subject at some later date. This patch isn't required, but it does a good job of demonstrating that a) the changes in 0002 don't impact HOT decisions (as intended) and b) that future patches which change HOT behavior have a very obvious record of what changed because they update these test results (not tests) to illustrate that. That said, if the next two patches are merged without this one I'd be just as happy as if all 3 made it into v19.

0002 - This patch plugs a hole (bug?) in ExecGetAllUpdatedCols() which is triggered by an existing test in tsearch.sql and the tsvector_update_trigger(). That trigger uses heap_modify_tuple() to change an indexed attribute that is not discovered by ExecGetAllUpdatedCols(), which seems odd to me at best and at worst wrong (or even a potential security issue). This patch finds and adds columns that are updated into the Bitmapset returned by ExecGetAllUpdatedCols(). The patch includes a helper function ExecCompareSlotAttrs() that will be used in follow-on patches as well.

0003 - This patch moves the logic for HeapDetermineColumnsInfo() into the executor while preserving the functionality of simple_heap_update(). A few helper functions are created to better illustrate HOT and lock mode decision making and are reused when possible. The portion of HeapDetermineColumnsInfo() related to replica identity key WAL logging is now in-line in heap_update().

These commits maintain 100% identical logic for HOT, lockmode, and replica identity decisions (or there's a flaw and that should be fixed so let me know) They simply juggle the logic into places where I think they fit better and provide for future work in this area.

I appreciate your time and effort considering these changes.

best.

-greg

On Wed, Mar 11, 2026 at 11:51:03AM -0400, Greg Burd wrote:
> 0002 - This patch plugs a hole (bug?) in ExecGetAllUpdatedCols() which is
> triggered by an existing test in tsearch.sql and the
> tsvector_update_trigger(). That trigger uses heap_modify_tuple() to
> change an indexed attribute that is not discovered by
> ExecGetAllUpdatedCols(), which seems odd to me at best and at worst wrong
> (or even a potential security issue). This patch finds and adds columns
> that are updated into the Bitmapset returned by ExecGetAllUpdatedCols().
> The patch includes a helper function ExecCompareSlotAttrs() that will be
> used in follow-on patches as well.

I just looked at this one for now.

> The net is that the functions like HeapDetermineColumnsInfo() have to
> scan all indexed attributes for changes rather than being able to first
> reduce the indexed set by intersecting it with the set of attributes
> known to be potentially updated.

I noticed the patch doesn't update HeapDetermineColumnsInfo() accordingly.
Is that intended?

> This commit introduces ExecCompareSlotAttrs() as a utility function to
> identify those attributes that have changed. It compares a subset of
> attributes between two TupleTableSlots and returns a Bitmapset of
> attributes that differ.

Hm. Most of this new function looks duplicated from
HeapDetermineColumnsInfo(), so IIUC this commit effectively adds another
scan through all the attributes. Does this produce noticeably more
overhead?

> It would be nice to integrate this into HeapDetermineColumnsInfo(),
> however it would be a layering violation given that it is within
> heap_update().

It'd be good to understand whether the current behavior is intentional or
just a happy accident. I found commit 2fd8685e7f, which looks like it was
intended as a prerequisite for the WARM feature (which I don't think was
ever committed). And it seems to have scanned through all indexed columns
when HOT was first introduced in commit 282d2a03dd.

I'm also curious whether anything else could modify columns that won't be
discovered by ExecGetAllUpdatedCols(). Having HeapDetermineColumnsInfo()
scan everything seems like a defense against such things, which is perhaps
why you've left it unchanged in the patch. I haven't looked into 0003 yet.
Is 0002 a prerequisite for that or a separate fix?

--
nathan

On Thu, Mar 12, 2026, at 4:33 PM, Nathan Bossart wrote:
> On Wed, Mar 11, 2026 at 11:51:03AM -0400, Greg Burd wrote:
>> 0002 - This patch plugs a hole (bug?) in ExecGetAllUpdatedCols() which is
>> triggered by an existing test in tsearch.sql and the
>> tsvector_update_trigger(). That trigger uses heap_modify_tuple() to
>> change an indexed attribute that is not discovered by
>> ExecGetAllUpdatedCols(), which seems odd to me at best and at worst wrong
>> (or even a potential security issue). This patch finds and adds columns
>> that are updated into the Bitmapset returned by ExecGetAllUpdatedCols().
>> The patch includes a helper function ExecCompareSlotAttrs() that will be
>> used in follow-on patches as well.
>
> I just looked at this one for now.

Hey Nathan!

Thanks for taking the time to review 0002.

>> The net is that the functions like HeapDetermineColumnsInfo() have to
>> scan all indexed attributes for changes rather than being able to first
>> reduce the indexed set by intersecting it with the set of attributes
>> known to be potentially updated.
>
> I noticed the patch doesn't update HeapDetermineColumnsInfo() accordingly.
> Is that intended?

Yes, that is intended. The 0002 patch is bug fix that I'd hidden along with what is now 0003, I pulled it out for clarity and to discuss independent of the other changes.

>> This commit introduces ExecCompareSlotAttrs() as a utility function to
>> identify those attributes that have changed. It compares a subset of
>> attributes between two TupleTableSlots and returns a Bitmapset of
>> attributes that differ.
>
> Hm. Most of this new function looks duplicated from
> HeapDetermineColumnsInfo(), so IIUC this commit effectively adds another
> scan through all the attributes. Does this produce noticeably more
> overhead?

Yes, it appears similar to that for a reason but it differs in one key way. It compares TupleTableSlots, not HeapTuples.

The commit doesn't add another scan, the new code only scans the attributes that ExecGetAllUpdatedCols() didn't pick up earlier and have cached for us at this point. The intersection between that set and what is indexed is almost always the NULL set because most UPDATEs don't invoke functions via triggers that modify indexed columns using heap_modify_tuple() directly. But, notably there is the case in tsearch.sql that does.

This introduces almost no net new overhead and when it does in fact do some work it's doing no more than what was done before in HeapDetermineColumnsInfo().

>> It would be nice to integrate this into HeapDetermineColumnsInfo(),
>> however it would be a layering violation given that it is within
>> heap_update().
>
> It'd be good to understand whether the current behavior is intentional or
> just a happy accident. I found commit 2fd8685e7f, which looks like it was
> intended as a prerequisite for the WARM feature (which I don't think was
> ever committed). And it seems to have scanned through all indexed columns
> when HOT was first introduced in commit 282d2a03dd.

Hard to tell if it was accidental or intentional, more digging required, but I'd bet that others poking in this area noticed the test failure and didn't connect the dots fully and just assumed best practice was to scan all indexed columns, even ones that could not have been updated at all.

Honestly, if we wrote this section from scratch again today I'm better it'd be closer to where my patch takes us than not.

> I'm also curious whether anything else could modify columns that won't be
> discovered by ExecGetAllUpdatedCols(). Having HeapDetermineColumnsInfo()
> scan everything seems like a defense against such things, which is perhaps
> why you've left it unchanged in the patch. I haven't looked into 0003 yet.
> Is 0002 a prerequisite for that or a separate fix?

Other than the heap_modify_tuple() calls I don't know of something that allows for direct changes but that doesn't matter, 0002 will scan and pick up those attributes even if we introduce a new modification path in the future (as intended).

HeapDetermineColumnsInfo() can't call ExecGetAllUpdatedCols() because that function needs resultRelInfo/EState both not available inside heap (table AM) calls. Also, the new helper compares TTS, not HeapTuples, which is what we have in heapam_tuple_update(), so not an option

0002 is a both a bug fix (IMO) and a pre-req for 0003 because in the next patch we use the new ExecCompareSlotAttrs() function from within the executor ahead of calling into ExecUpdate().

> --
> nathan

Thanks for your time and comments, let me know if you have more. :)

best.

-greg

On Thu, 2026-03-12 at 17:31 -0400, Greg Burd wrote:
> Other than the heap_modify_tuple() calls I don't know of something
> that allows for direct changes but that doesn't matter, 0002 will
> scan and pick up those attributes even if we introduce a new
> modification path in the future (as intended).

Why do extra work in ExecBRUpdateTriggers() to eliminate the false
negative case if we don't rely on it anyway? If we do need to rely on
it in subsequent patches, then we need to be sure, right?

I guess I'm confused about whether 0002 is introducing a new guarantee
or if it's just a convenient place to eliminate one source of false
negatives.

Regards,
Jeff Davis

On Sun, Mar 15, 2026, at 5:11 PM, Jeff Davis wrote:
> On Thu, 2026-03-12 at 17:31 -0400, Greg Burd wrote:
>> Other than the heap_modify_tuple() calls I don't know of something
>> that allows for direct changes but that doesn't matter, 0002 will
>> scan and pick up those attributes even if we introduce a new
>> modification path in the future (as intended).

Hello Jeff, thanks for taking a look! :)

> Why do extra work in ExecBRUpdateTriggers() to eliminate the false
> negative case if we don't rely on it anyway? If we do need to rely on
> it in subsequent patches, then we need to be sure, right?

Later commits do currently rely on it, ExecUpdateModifiedIdxAttrs() uses it in the next commit (0003) to avoid reviewing indexed attributes that could not have possibly changed. Imagine a table with a lot of indexes where updates only modify one or two at a time. Why are we testing indexed attributes for changes in HeapDeterminColumnsInfo() that couldn't have changed? The answer is that a) HeapDeterminColumnsInfo() lives in heap, not the executor (see patch 0003) so it has no ability to call ExecGetAllUpdatedCols(), and b) the set returned by ExecGetAllUpdatedCols() is sometimes incomplete.

I see (a) as something I fix in patch 0003 and (b) as an oversight (or bug). I'll also argue that the overhead of checking for additional attributes in ExecBRUpdateTriggers() vs the overhead of checking all indexed attributes in HeapDeterminColumnsInfo() is net zero once patch 0003 is applied.

The argument to keep 0002 is both performance as much as correctness. After 0002 and 0003 ExecUpdateModifiedIdxAttrs() replaces HeapDeterminColumnsInfo() and doesn't have to scan all indexed attributes anymore. Relations with lots of indexed attributes but update patterns that only focus on subsets of those attributes will benefit as there will be fewer memcmp() calls when comparing datums.

What do we "need to be sure" of? That ExecGetAllUpdatedCols() not really contains all attributes that its name implies? I think it now does that after 0002, do you disagree?

> I guess I'm confused about whether 0002 is introducing a new guarantee
> or if it's just a convenient place to eliminate one source of false
> negatives.

I think it is a new guarantee that was implied before now but not required until 0003. I think this change reduces overhead and helps to avoid some future security feature that depends on ExecGetAllUpdatedCols() to provide that guarantee.

Does that make sense?

> Regards,
> Jeff Davis

best.

-greg

On Mon, Mar 16, 2026 at 12:23:04PM -0400, Greg Burd wrote:
> On Sun, Mar 15, 2026, at 5:11 PM, Jeff Davis wrote:
>> Why do extra work in ExecBRUpdateTriggers() to eliminate the false
>> negative case if we don't rely on it anyway? If we do need to rely on
>> it in subsequent patches, then we need to be sure, right?
>
> [...]
>
> What do we "need to be sure" of? That ExecGetAllUpdatedCols() not really
> contains all attributes that its name implies? I think it now does that
> after 0002, do you disagree?

I'm admittedly still digging into the details, but the main question on my
mind is whether there are other cases lurking that our in-tree tests aren't
catching or that only exist in extensions. Will there be some sort of
check or assertion to catch those?

--
nathan

On Mon, 2026-03-16 at 12:23 -0400, Greg Burd wrote:
> Hello Jeff, thanks for taking a look! :)

Hi, thank you for working on this problem!

> > Why do extra work in ExecBRUpdateTriggers() to eliminate the false
> > negative case if we don't rely on it anyway? If we do need to rely
> > on
> > it in subsequent patches, then we need to be sure, right?
>
> Later commits do currently rely on it, ExecUpdateModifiedIdxAttrs()
> uses it in the next commit (0003) to avoid reviewing indexed
> attributes that could not have possibly changed.

OK. The first half of the commit message for 0002 is slightly confusing
because it's referring to pre-existing behavior, behavior changed by
the commit, and also future work. It might help to clarify the tenses
like:

- Previously, ExecGetAllUpdatedCols() had gaps ..., but not a real bug
because ...
- This commit closes those gaps by updating ri_extraUpdatedCols in
ExecBRUpdateTriggers(), making ExecGetAllUpdatedCols() reliable.
- We know there are no other gaps because ...
- Useful to fix because later work will rely on it for [very brief
reason]

>   Imagine a table with a lot of indexes where updates only modify one
> or two at a time.  Why are we testing indexed attributes for changes
> in HeapDeterminColumnsInfo() that couldn't have changed?  The answer
> is that a) HeapDeterminColumnsInfo() lives in heap, not the executor
> (see patch 0003) so it has no ability to call
> ExecGetAllUpdatedCols(), and b) the set returned by
> ExecGetAllUpdatedCols() is sometimes incomplete.

That's helpful, thank you.

> What do we "need to be sure" of?  That ExecGetAllUpdatedCols() not
> really contains all attributes that its name implies?  I think it now
> does that after 0002, do you disagree?

I don't disagree, but I think we need some kind statement that we
believe that it's true, and a brief explanation why. (I don't have much
of an opinion about whether it's in this thread, the commit message, or
the code.)

>
> I think it is a new guarantee that was implied before now but not
> required until 0003.  I think this change reduces overhead and helps
> to avoid some future security feature that depends on
> ExecGetAllUpdatedCols() to provide that guarantee.
>
> Does that make sense?

A subtlety here is that perhaps ExecGetAllUpdatedCols() already *was*
correct, and it just meant something different than we thought: the
*targeted* columns of an update, instead of the actually-updated
values.

If so we should think about whether that distinction should be
preserved. For instance, column filtering for triggers should be based
on the targeted columns (rather than actually-updated values) because,
semantically, it should still fire even for a no-op update. Perhaps
similar for choosing the lock mode?

>
Regards,
Jeff Davis

On Mon, Mar 16, 2026, at 1:55 PM, Jeff Davis wrote:
> On Mon, 2026-03-16 at 12:23 -0400, Greg Burd wrote:
>> Hello Jeff, thanks for taking a look! :)
>
> Hi, thank you for working on this problem!
>
>> > Why do extra work in ExecBRUpdateTriggers() to eliminate the false
>> > negative case if we don't rely on it anyway? If we do need to rely
>> > on
>> > it in subsequent patches, then we need to be sure, right?
>>
>> Later commits do currently rely on it, ExecUpdateModifiedIdxAttrs()
>> uses it in the next commit (0003) to avoid reviewing indexed
>> attributes that could not have possibly changed.
>
> OK. The first half of the commit message for 0002 is slightly confusing
> because it's referring to pre-existing behavior, behavior changed by
> the commit, and also future work. It might help to clarify the tenses
> like:
>
> - Previously, ExecGetAllUpdatedCols() had gaps ..., but not a real bug
> because ...
> - This commit closes those gaps by updating ri_extraUpdatedCols in
> ExecBRUpdateTriggers(), making ExecGetAllUpdatedCols() reliable.
> - We know there are no other gaps because ...
> - Useful to fix because later work will rely on it for [very brief
> reason]

Hey Jeff,

Good idea, I'll fix the commit message.

>>   Imagine a table with a lot of indexes where updates only modify one
>> or two at a time.  Why are we testing indexed attributes for changes
>> in HeapDeterminColumnsInfo() that couldn't have changed?  The answer
>> is that a) HeapDeterminColumnsInfo() lives in heap, not the executor
>> (see patch 0003) so it has no ability to call
>> ExecGetAllUpdatedCols(), and b) the set returned by
>> ExecGetAllUpdatedCols() is sometimes incomplete.
>
> That's helpful, thank you.
>
>> What do we "need to be sure" of?  That ExecGetAllUpdatedCols() not
>> really contains all attributes that its name implies?  I think it now
>> does that after 0002, do you disagree?
>
> I don't disagree, but I think we need some kind statement that we
> believe that it's true, and a brief explanation why. (I don't have much
> of an opinion about whether it's in this thread, the commit message, or
> the code.)

Okay, I can do that.

>>
>> I think it is a new guarantee that was implied before now but not
>> required until 0003.  I think this change reduces overhead and helps
>> to avoid some future security feature that depends on
>> ExecGetAllUpdatedCols() to provide that guarantee.
>>
>> Does that make sense?
>
> A subtlety here is that perhaps ExecGetAllUpdatedCols() already *was*
> correct, and it just meant something different than we thought: the
> *targeted* columns of an update, instead of the actually-updated
> values.

Fair, I think a simple way to side-step this is for me to create a new function ExecGetKnownUpdatedAttrs() that does a) ExecGetAllUpdatedCols() and then b) looks for anything missing. I'll use that function and leave this one, which was only used in triggers and one other place in execIndexing.c (which I remove in one of my later patches), for that.

> If so we should think about whether that distinction should be
> preserved. For instance, column filtering for triggers should be based
> on the targeted columns (rather than actually-updated values) because,
> semantically, it should still fire even for a no-op update. Perhaps
> similar for choosing the lock mode?

Now I want to rename ExecGetAllUpdatedCols() to ExecUpdateTargetedCols(), maybe I will. And while I'm at it I'll change the single non-trigger use case in index_unchanged_by_update() to my new ExecUpdateTargetedCols() function which better matches that use anyway.

>>
> Regards,
> Jeff Davis

best.

-greg

On Mon, Mar 16, 2026, at 1:29 PM, Nathan Bossart wrote:
> On Mon, Mar 16, 2026 at 12:23:04PM -0400, Greg Burd wrote:
>> On Sun, Mar 15, 2026, at 5:11 PM, Jeff Davis wrote:
>>> Why do extra work in ExecBRUpdateTriggers() to eliminate the false
>>> negative case if we don't rely on it anyway? If we do need to rely on
>>> it in subsequent patches, then we need to be sure, right?
>>
>> [...]
>>
>> What do we "need to be sure" of? That ExecGetAllUpdatedCols() not really
>> contains all attributes that its name implies? I think it now does that
>> after 0002, do you disagree?
>
> I'm admittedly still digging into the details, but the main question on my
> mind is whether there are other cases lurking that our in-tree tests aren't
> catching or that only exist in extensions. Will there be some sort of
> check or assertion to catch those?

Hey Nathan,

I think based on Jeff's questions I'm going to side-step this a bit with a new function ExecUpdateTargetedCols(). Hopefully I can have an assert in there that double checks the assumption and validates the contract.

> --
> nathan

-greg

On Mon, 2026-03-16 at 14:35 -0400, Greg Burd wrote:
> Now I want to rename ExecGetAllUpdatedCols() to
> ExecUpdateTargetedCols(), maybe I will.  And while I'm at it I'll
> change the single non-trigger use case in index_unchanged_by_update()
> to my new ExecUpdateTargetedCols() function which better matches that
> use anyway.

Does this mean a new bitmap of "actually changed values" that's a
subset of targeted columns?

If so it feels like quite a few bitmaps and we might need to expand
some comments to explain the subtle meanings of each, and when they are
valid, and what kinds of false positives they might contain (hopefully
none have false negatives).

Also, the "actually changed values" is only valid for a single tuple,
and it would be good to clarify that and make sure there's not a lot of
room for confusion there.

Regards,
Jeff Davis

On Mon, Mar 16, 2026, at 4:01 PM, Jeff Davis wrote:
> On Mon, 2026-03-16 at 14:35 -0400, Greg Burd wrote:
>> Now I want to rename ExecGetAllUpdatedCols() to
>> ExecUpdateTargetedCols(), maybe I will.  And while I'm at it I'll
>> change the single non-trigger use case in index_unchanged_by_update()
>> to my new ExecUpdateTargetedCols() function which better matches that
>> use anyway.
>
> Does this mean a new bitmap of "actually changed values" that's a
> subset of targeted columns?

No, I'd been thinking of targeted columns *and* any other columns we can identify as modified.

> If so it feels like quite a few bitmaps and we might need to expand
> some comments to explain the subtle meanings of each, and when they are
> valid, and what kinds of false positives they might contain (hopefully
> none have false negatives).

I agree, and also this commit has garnered too much attention relative to the important changes that come after it.

> Also, the "actually changed values" is only valid for a single tuple,
> and it would be good to clarify that and make sure there's not a lot of
> room for confusion there.

Yes, that's true... too much confusion and not enough juice for the squeeze. I'm dropping that.

> Regards,
> Jeff Davis

So, attached is v36 with the:

0001 - New hot_updates.sql tests, important for future commits but can be skipped for now.
0002 - Move HeapDetermineColumnsInfo() to executor

best.

-greg

Catching up here. I see that you dropped 0002. Can you explain why that's
no longer needed?

On Mon, Mar 16, 2026 at 04:51:31PM -0400, Greg Burd wrote:
> Refactor executor update logic to determine which indexed columns have
> actually changed during an UPDATE operation rather than leaving this up
> to HeapDetermineColumnsInfo() in heap_update(). Finding this set of
> attributes is not heap-specific, but more general to all table AMs and
> having this information in the executor could inform other decisions
> about when index inserts are required and when they are not regardless
> of the table AM's MVCC implementation strategy.

Nice, this is a crisp motivation statement.

> Development of this feature exposed nondeterministic behavior in three
> existing tests which have been adjusted to avoid inconsistent test
> results due to tuple ordering during heap page scans.

Logistically speaking, these could be nice to get out of the way early as a
prerequisite patch so we can focus on the substance of this patch.

The rest of my comments are from a relatively quick skim. Deeper review to
follow...

> + /*
> + * Reduce the set under review to only the unmodified indexed replica
> + * identity key attributes. idx_attrs is copied (by bms_difference())
> + * not modified here.
> + */
> + attrs = bms_difference(idx_attrs, modified_idx_attrs);
> + attrs = bms_int_members(attrs, rid_attrs);
> +
> + while ((attidx = bms_next_member(attrs, attidx)) >= 0)

Could it be worth moving this loop (and some surrounding code) to a helper
function?

> - * Note: beyond this point, use oldtup not otid to refer to old tuple.
> + * NOTE: beyond this point, use oldtup not otid to refer to old tuple.

nitpick: Please remove unnecessary changes.

> @@ -5269,10 +5269,10 @@ RelationGetIndexPredicate(Relation relation)
> * in expressions (i.e., usable for FKs)
> * INDEX_ATTR_BITMAP_PRIMARY_KEY Columns in the table's primary key
> * (beware: even if PK is deferrable!)
> + * INDEX_ATTR_BITMAP_SUMMARIZED Columns only included in summarizing indexes
> * INDEX_ATTR_BITMAP_IDENTITY_KEY Columns in the table's replica identity
> * index (empty if FULL)
> - * INDEX_ATTR_BITMAP_HOT_BLOCKING Columns that block updates from being HOT
> - * INDEX_ATTR_BITMAP_SUMMARIZED Columns included in summarizing indexes
> + * INDEX_ATTR_BITMAP_INDEXED Columns referenced by indexes

Is the meaning of INDEX_ATTR_BITMAP_SUMMARIZED changing in this patch? I
see you moved it and dropped the "only".

> - Bitmapset *summarizedattrs; /* columns with summarizing indexes */
> + Bitmapset *indexedattrs; /* columns referenced by indexes */
> + Bitmapset *summarizedattrs; /* columns only in summarizing indexes */

But you added an "only" here...

--
nathan

On Tue, Mar 17, 2026, at 11:22 AM, Nathan Bossart wrote:
> Catching up here. I see that you dropped 0002. Can you explain why that's
> no longer needed?

Hey Nathan,

Certainly. 0002 in v35 was an attempt to add modified attributes to the set produced by ExecGetAllUpdatedCols() with anything changed in a before-row trigger via heap_modify_tuple() as happens in tsearch.sql testing. However, that function produces a bitmapset of the *targeted* attributes which applies to *all* tuples being updated (when there is more than one in the UPDATE), not just one. My change added a attribute when it changed in a specific tuple, which may not be true for all tuples. So 0002 would have had to change to fix that bug by re-discovering any modified attributes for each tuple. That seems bad and the more that I looked at it the more I felt that the simple approach of just scanning all indexed tuples for updates would work perfectly fine without additional overhead relative to today's code. So, I've pulled that out of the series.

> On Mon, Mar 16, 2026 at 04:51:31PM -0400, Greg Burd wrote:
>> Refactor executor update logic to determine which indexed columns have
>> actually changed during an UPDATE operation rather than leaving this up
>> to HeapDetermineColumnsInfo() in heap_update(). Finding this set of
>> attributes is not heap-specific, but more general to all table AMs and
>> having this information in the executor could inform other decisions
>> about when index inserts are required and when they are not regardless
>> of the table AM's MVCC implementation strategy.
>
> Nice, this is a crisp motivation statement.

Thanks!

>> Development of this feature exposed nondeterministic behavior in three
>> existing tests which have been adjusted to avoid inconsistent test
>> results due to tuple ordering during heap page scans.
>
> Logistically speaking, these could be nice to get out of the way early as a
> prerequisite patch so we can focus on the substance of this patch.

By that you mean a patch ahead of 0002/v36 that just makes the changes to the tests? That's easy enough to do.

> The rest of my comments are from a relatively quick skim. Deeper review to
> follow...
>
>> + /*
>> + * Reduce the set under review to only the unmodified indexed replica
>> + * identity key attributes. idx_attrs is copied (by bms_difference())
>> + * not modified here.
>> + */
>> + attrs = bms_difference(idx_attrs, modified_idx_attrs);
>> + attrs = bms_int_members(attrs, rid_attrs);
>> +
>> + while ((attidx = bms_next_member(attrs, attidx)) >= 0)
>
> Could it be worth moving this loop (and some surrounding code) to a helper
> function?

I'd done that at one point, I'd even moved this into the executor and then decided that wasn't a good home for it (too heap specific). I can make this into a helper function if you'd like.

>> - * Note: beyond this point, use oldtup not otid to refer to old tuple.
>> + * NOTE: beyond this point, use oldtup not otid to refer to old tuple.
>
> nitpick: Please remove unnecessary changes.

Sure... this is due to my config in my editor it spots the second not the first. But I'll revert that and update my editor config. ;-P

>> @@ -5269,10 +5269,10 @@ RelationGetIndexPredicate(Relation relation)
>> * in expressions (i.e., usable for FKs)
>> * INDEX_ATTR_BITMAP_PRIMARY_KEY Columns in the table's primary key
>> * (beware: even if PK is deferrable!)
>> + * INDEX_ATTR_BITMAP_SUMMARIZED Columns only included in summarizing indexes
>> * INDEX_ATTR_BITMAP_IDENTITY_KEY Columns in the table's replica identity
>> * index (empty if FULL)
>> - * INDEX_ATTR_BITMAP_HOT_BLOCKING Columns that block updates from being HOT
>> - * INDEX_ATTR_BITMAP_SUMMARIZED Columns included in summarizing indexes
>> + * INDEX_ATTR_BITMAP_INDEXED Columns referenced by indexes
>
> Is the meaning of INDEX_ATTR_BITMAP_SUMMARIZED changing in this patch? I
> see you moved it and dropped the "only".

Hmmm... that was a mistake. I'll re-position it and yes that set should be attributes *only* referenced in summarized indexes.

>> - Bitmapset *summarizedattrs; /* columns with summarizing indexes */
>> + Bitmapset *indexedattrs; /* columns referenced by indexes */
>> + Bitmapset *summarizedattrs; /* columns only in summarizing indexes */
>
> But you added an "only" here...

Yes, good catch. Something got lost while juggling patches. :)

> --
> nathan

best.

-greg

On Mon, 2026-03-16 at 16:51 -0400, Greg Burd wrote:
> > Also, the "actually changed values" is only valid for a single
> > tuple,
> > and it would be good to clarify that and make sure there's not a
> > lot of
> > room for confusion there.
>
> Yes, that's true... too much confusion and not enough juice for the
> squeeze.  I'm dropping that.

That is an interesting case you found in that the columns targeted by
an update are not a superset of the columns with actually changed
values. But I'm not sure exactly what to make of that fact, and if it's
not important for your other changes then I agree that we should drop
it.

However, it might be good to comment somewhere that your changes (which
are based on values in specific tuples) cannot rely on
ExecGetAllUpdatedCols(), to avoid confusion in the future.

Regards,
Jeff Davis

On Tue, Mar 17, 2026, at 12:38 PM, Jeff Davis wrote:
> On Mon, 2026-03-16 at 16:51 -0400, Greg Burd wrote:
>> > Also, the "actually changed values" is only valid for a single
>> > tuple,
>> > and it would be good to clarify that and make sure there's not a
>> > lot of
>> > room for confusion there.
>>
>> Yes, that's true... too much confusion and not enough juice for the
>> squeeze.  I'm dropping that.
>
> That is an interesting case you found in that the columns targeted by
> an update are not a superset of the columns with actually changed
> values. But I'm not sure exactly what to make of that fact, and if it's
> not important for your other changes then I agree that we should drop
> it.
>
> However, it might be good to comment somewhere that your changes (which
> are based on values in specific tuples) cannot rely on
> ExecGetAllUpdatedCols(), to avoid confusion in the future.

Fair point, I'll do that.

> Regards,
> Jeff Davis

v37 attached with changes you and Nathan asked for so far. More please! :)

thanks Jeff and Nathan!

best.

-greg

Thanks for the new patch. As a general note, please be sure to run
pgindent on patches. My review is still rather surface-level, sorry.

On Tue, Mar 17, 2026 at 02:04:11PM -0400, Greg Burd wrote:
> - id_attrs = RelationGetIndexAttrBitmap(relation,
> - INDEX_ATTR_BITMAP_IDENTITY_KEY);
> [...]
> + rid_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_IDENTITY_KEY);

I'm nitpicking, but it took me a while to parse the
replica-identity-related code in heap_update() until I discovered that this
variable was renamed. I think we ought to leave the name alone.

> /*
> * At this point newbuf and buffer are both pinned and locked, and newbuf
> - * has enough space for the new tuple. If they are the same buffer, only
> - * one pin is held.
> + * has enough space for the new tuple so we can use the HOT update path if
> + * the caller determined that it is allowable.
> + *
> + * NOTE: If newbuf == buffer then only one pin is held.
> */
> -
> if (newbuf == buffer)

Sorry, more nitpicks. In addition to the unnecessary removal of the blank
line, I'm not sure the changes to this comment are needed.

> - /*
> - * If it is a HOT update, the update may still need to update summarized
> - * indexes, lest we fail to update those summaries and get incorrect
> - * results (for example, minmax bounds of the block may change with this
> - * update).
> - */
> - if (use_hot_update)
> - {
> - if (summarized_update)
> - *update_indexes = TU_Summarizing;
> - else
> - *update_indexes = TU_None;
> - }
> - else
> - *update_indexes = TU_All;

So, the "HOT but still need to update summarized indexes" code has been
moved from heap_update() to HeapUpdateHotAllowable(), which is called by
heap_update()'s callers (i.e., simple_heap_update() and
heapam_tuple_update()). That looks correct to me at a glance.

> -simple_heap_update(Relation relation, const ItemPointerData *otid, HeapTuple tup,
> +simple_heap_update(Relation relation, const ItemPointerData *otid, HeapTuple tuple,

nitpick: This variable name change looks unnecessary.

> @@ -944,8 +946,13 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
> if (rel->rd_rel->relispartition)
> ExecPartitionCheck(resultRelInfo, slot, estate, true);
>
> + modified_idx_attrs = ExecUpdateModifiedIdxAttrs(resultRelInfo,
> + estate, searchslot, slot);
> +
> simple_table_tuple_update(rel, tid, slot, estate->es_snapshot,
> - &update_indexes);
> + modified_idx_attrs, &update_indexes);
> + bms_free(modified_idx_attrs);

I don't know how constructive of a comment this is, but this change in
particular seems quite out of place. It feels odd to me that we expect
callers of simple_table_tuple_update() to determine the
modified-index-attributes. I guess I'm confused why this work doesn't
belong one level down, i.e., in the tuple_update function.

> - * INDEX_ATTR_BITMAP_SUMMARIZED Columns included in summarizing indexes
> + * INDEX_ATTR_BITMAP_INDEXED Columns referenced by indexes
> + * INDEX_ATTR_BITMAP_SUMMARIZED Columns only included in summarizing indexes

> - Bitmapset *summarizedattrs; /* columns with summarizing indexes */
> + Bitmapset *indexedattrs; /* columns referenced by indexes */
> + Bitmapset *summarizedattrs; /* columns only in summarizing indexes */

As before, the comment changes for the summarized-attr-related stuff seem
unnecessary.

> if (indexDesc->rd_indam->amsummarizing)
> attrs = &summarizedattrs;
> else
> - attrs = &hotblockingattrs;
> + attrs = &indexedattrs;

> + /*
> + * Record what attributes are only referenced by summarizing indexes. Then
> + * add that into the other indexed attributes to track all referenced
> + * attributes.
> + */
> + summarizedattrs = bms_del_members(summarizedattrs, indexedattrs);
> + indexedattrs = bms_add_members(indexedattrs, summarizedattrs);

The difference between hotblockingattrs and indexedattrs seems quite
subtle. Am I understanding correctly that indexedattrs is essentially just
hotblockingattrs + summarizedattrs? And that this is all meant for
INDEX_ATTR_BITMAP_INDEXED?

- INJECTION_POINT("heap_update-before-pin", NULL);
+ INJECTION_POINT("simple_heap_update-before-pin", NULL);

Why was this changed in heap_update()?

--
nathan

On Mon, Mar 23, 2026, at 2:39 PM, Nathan Bossart wrote:
> Thanks for the new patch. As a general note, please be sure to run
> pgindent on patches. My review is still rather surface-level, sorry.

Hello Nathan,

Thanks for continuing to review my work. I appreciate your time. I do run pgindent on all patches, maybe something slipped it. Apologies if that's the case. :)

> On Tue, Mar 17, 2026 at 02:04:11PM -0400, Greg Burd wrote:
>> - id_attrs = RelationGetIndexAttrBitmap(relation,
>> - INDEX_ATTR_BITMAP_IDENTITY_KEY);
>> [...]
>> + rid_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_IDENTITY_KEY);
>
> I'm nitpicking, but it took me a while to parse the
> replica-identity-related code in heap_update() until I discovered that this
> variable was renamed. I think we ought to leave the name alone.

Okay, reverted to "id_attrs".

>> /*
>> * At this point newbuf and buffer are both pinned and locked, and newbuf
>> - * has enough space for the new tuple. If they are the same buffer, only
>> - * one pin is held.
>> + * has enough space for the new tuple so we can use the HOT update path if
>> + * the caller determined that it is allowable.
>> + *
>> + * NOTE: If newbuf == buffer then only one pin is held.
>> */
>> -
>> if (newbuf == buffer)
>
> Sorry, more nitpicks. In addition to the unnecessary removal of the blank
> line, I'm not sure the changes to this comment are needed.

Okay, reverted to earlier comment and blank line re-inserted. :)

>> - /*
>> - * If it is a HOT update, the update may still need to update summarized
>> - * indexes, lest we fail to update those summaries and get incorrect
>> - * results (for example, minmax bounds of the block may change with this
>> - * update).
>> - */
>> - if (use_hot_update)
>> - {
>> - if (summarized_update)
>> - *update_indexes = TU_Summarizing;
>> - else
>> - *update_indexes = TU_None;
>> - }
>> - else
>> - *update_indexes = TU_All;
>
> So, the "HOT but still need to update summarized indexes" code has been
> moved from heap_update() to HeapUpdateHotAllowable(), which is called by
> heap_update()'s callers (i.e., simple_heap_update() and
> heapam_tuple_update()). That looks correct to me at a glance.

Yes, that's indeed what that is.

>> -simple_heap_update(Relation relation, const ItemPointerData *otid, HeapTuple tup,
>> +simple_heap_update(Relation relation, const ItemPointerData *otid, HeapTuple tuple,
>
> nitpick: This variable name change looks unnecessary.

Okay, reverted to "tup".

>> @@ -944,8 +946,13 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
>> if (rel->rd_rel->relispartition)
>> ExecPartitionCheck(resultRelInfo, slot, estate, true);
>>
>> + modified_idx_attrs = ExecUpdateModifiedIdxAttrs(resultRelInfo,
>> + estate, searchslot, slot);
>> +
>> simple_table_tuple_update(rel, tid, slot, estate->es_snapshot,
>> - &update_indexes);
>> + modified_idx_attrs, &update_indexes);
>> + bms_free(modified_idx_attrs);
>
> I don't know how constructive of a comment this is, but this change in
> particular seems quite out of place. It feels odd to me that we expect
> callers of simple_table_tuple_update() to determine the
> modified-index-attributes. I guess I'm confused why this work doesn't
> belong one level down, i.e., in the tuple_update function.

Problem is that simple_table_tuple_update() has the old TID and the new slot, but not the old slot (searchslot), so I'd have to change the signature of that function either way. Passing modified_idx_attrs is the new pattern, so I am just reusing that here.

I could replicate what's in simple_heap_update() and call HeapUpdateModifiedIdxAttrs() after re-constructing the HeapTuple, but that feels very ugly/unnecessary to me given that the caller has that information already in slot form.

I've left this as is, but I'm happy to continue discussing options.

>> - * INDEX_ATTR_BITMAP_SUMMARIZED Columns included in summarizing indexes
>> + * INDEX_ATTR_BITMAP_INDEXED Columns referenced by indexes
>> + * INDEX_ATTR_BITMAP_SUMMARIZED Columns only included in summarizing indexes
>
>> - Bitmapset *summarizedattrs; /* columns with summarizing indexes */
>> + Bitmapset *indexedattrs; /* columns referenced by indexes */
>> + Bitmapset *summarizedattrs; /* columns only in summarizing indexes */
>
> As before, the comment changes for the summarized-attr-related stuff seem
> unnecessary.

I disagree, the "only" is required to highlight the logic change here. Before this patch summarized attrs could overlap with indexed attrs, now it should not. This makes the logic a bit easier later in HeapUpdateHotAllowable().

>> if (indexDesc->rd_indam->amsummarizing)
>> attrs = &summarizedattrs;
>> else
>> - attrs = &hotblockingattrs;
>> + attrs = &indexedattrs;
>
>> + /*
>> + * Record what attributes are only referenced by summarizing indexes. Then
>> + * add that into the other indexed attributes to track all referenced
>> + * attributes.
>> + */
>> + summarizedattrs = bms_del_members(summarizedattrs, indexedattrs);
>> + indexedattrs = bms_add_members(indexedattrs, summarizedattrs);
>
> The difference between hotblockingattrs and indexedattrs seems quite
> subtle.

I feel it was *much* more subtle before and mis-named ("hot blocking"). But, let's review. On master today in heapam.c heap_update() near the start and before the buffer lock there is the following:

hot_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_HOT_BLOCKING);
sum_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_SUMMARIZED);
key_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_KEY);
id_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_IDENTITY_KEY);

It turns out that hot_attrs includes all INDEX_ATTR_BITMAP_IDENTITY_KEY and INDEX_ATTR_BITMAP_IDENTITY_KEY except for those found when scanning a summarizing index. So, what comes next is a bit wasteful.

interesting_attrs = NULL;
interesting_attrs = bms_add_members(interesting_attrs, hot_attrs);
interesting_attrs = bms_add_members(interesting_attrs, sum_attrs);
interesting_attrs = bms_add_members(interesting_attrs, key_attrs); <- unnecessary
interesting_attrs = bms_add_members(interesting_attrs, id_attrs); <- unnecessary

And that in the end, what's passed to HeapDetermineColumnsInfo() is all indexed attributes, including summarized, and those found in expressions. That function then reduces the set from "interesting" to "modified (and indexed is implied)". It does this by testing before/after datum for equality (memcmp() via datumIsEqual()) and that becomes our "modified_attrs" set used for lockmode and HOT eligibility tests.

When testing later on (on master) for the HOT or NOT decision the code:

if (newbuf == buffer)
{
// first test to see if any modified/indexed attributes are used
// by non-summarizing indexes
if (!bms_overlap(modified_attrs, hot_attrs))
{
// and if not, we're going HOT
use_hot_update = true;

// at this point if there is any overlap it means that the only
// attributes that might be referenced by an index and modified
// are summarizing, there can't be any non-summarizing attributes
// in the modified_attrs set otherwise our first test would have
// failed, so this tests for the "only summarizing" case
if (bms_overlap(modified_attrs, sum_attrs))
only_summarized = true;
}
}

My thinking was, why re-create this every update? Why not have the cached representation of these bitmaps have what's needed?

Now, I've changed the logic in this patch. First in the executor nodeModifyTable.c ExecUpdateModifiedIdxAttrs() identify which indexed attributes were modified (changed value):

// get all attributes indexed on a relation, including summarized
// note how we no longer construct "interesting_attrs" from a number
// of bitmaps, the map we want is the map we cached and the name matches
// the content, *all* indexed attributes (not indexed, but not summarized)
attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_INDEXED);

// compare the old/new reducing the set to only those that changed
// as determined by datum_is_equal() to produce the modified/indexed
// attribute set
attrs = ExecCompareSlotAttrs(attrs, tupdesc, old_tts, new_tts);

Then in heapam_handler.c heapam_tuple_update():

// call our helper function
hot_allowed = HeapUpdateHotAllowable(relation, modified_idx_attrs, &summarized_only);

HeapUpdateHotAllowable()
{
// if no indexed attributes were modified, we're done
if (bms_is_empty(modified_idx_attrs))
return true;
else
{
// now we need the *only* summarized attributes
Bitmapset *sum_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_SUMMARIZED);

// if the modified set is a sumset of the summarized,
// we're only updating summarized
if (bms_is_subset(modified_idx_attrs, sum_attrs))
{
hot_allowed = true;
*summarized_only = true;
}
else
// at least one attribute is modified, referenced by an index
// that isn't summarizing, HOT isn't allowed
hot_allowed = false;

bms_free(sum_attrs);
}
}

So, we go from 3 calls to RelationGetIndexAttrBitmap() to 1, or at most 2 when there's a summarizing index (which is frequently the case).

This feels more logical, cleaner, and has less overhead but supports the same HOT logic.

> Am I understanding correctly that indexedattrs is essentially just
> hotblockingattrs + summarizedattrs? And that this is all meant for
> INDEX_ATTR_BITMAP_INDEXED?
>
> - INJECTION_POINT("heap_update-before-pin", NULL);
> + INJECTION_POINT("simple_heap_update-before-pin", NULL);
>
> Why was this changed in heap_update()?

Oops, that's a mistake. Fixed it.

> --
> nathan

Thanks for your review, v38 attached.

best.

-greg