Thread: Re: Expanding HOT updates for expression and partial indexes

On Thu, 6 Feb 2025 at 23:24, Burd, Greg <gregburd@amazon.com> wrote:
>
> Attached find a patch that expands the cases where heap-only tuple (HOT) updates are possible without changing the
basicsemantics of HOT. This is accomplished by examining expression indexes for changes to determine if indexes require
updatingor not. A similar approach is taken for partial indexes, the predicate is evaluated and, in some cases, HOT
updatesare allowed. Even with this patch if any index is changed, all indexes are updated. Only in cases where none are
modifiedwill 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
updatepath, 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
addressedby 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
feelingas if I violated separation of concerns. If there is a better way to do this let me know or if you think the
costof 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
thatis 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/noneof the indexes) and that may violate the principal of least surprise but I feel that it is better than the
TU_UpdateIndexesenum 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
whilealso updating the index.  No degradation here that I’m aware of, indeed the tests that ensure that behavior are
unchangedand pass.
 

-greg

> On Feb 10, 2025, at 12:17 PM, Matthias van de Meent <boekewurm+postgres@gmail.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@gmail.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
feelingas if I violated separation of concerns. If there is a better way to do this let me know or if you think the
costof 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
thesummarizing attributes are removed.  The first test then in heap_update() is for overlap with the modified set.
Whenthere 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
longercode 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
scanover the list of IndexInfo.  It is really only when there are many expressions/predicates requiring examination
thatthere 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
forthe table not per-column.  Now there is one bitmap to cover the broadest case and then a function to find the
modifiedset 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’scall into a function in execIndexing up into nodeModifiyTable and back down into execIndexing and on to the
indexaccess 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
thatis 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/noneof the indexes) and that may violate the principal of least surprise but I feel that it is better than the
TU_UpdateIndexesenum 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@gmail.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 19:15, Burd, Greg <gregburd@amazon.com> wrote:
>
> Apologies for not being clear, this preserves the current behavior for summarizing indexes allowing for HOT updates
whilealso updating the index.  No degradation here that I’m aware of, indeed the tests that ensure that behavior are
unchangedand 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
attributesagainst 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@gmail.com> wrote:
> 
> On Mon, 10 Feb 2025 at 19:15, Burd, Greg <gregburd@amazon.com> wrote:
>> 
>> Apologies for not being clear, this preserves the current behavior for summarizing indexes allowing for HOT updates
whilealso updating the index.  No degradation here that I’m aware of, indeed the tests that ensure that behavior are
unchangedand 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)

On Thu, 13 Feb 2025 at 19:46, Burd, Greg <gregburd@amazon.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
attributesfor 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
eachis examined against bitmaps that contain only attributes specific to the index in question.  This helped in cases
wherethere 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
unnecessaryand should be avoided.  In this v5 patchset this case becomes two checks of bitmaps (first hot_attrs, then
exclusivelyexp_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.

Hello,

I've rebased and updated the patch a bit.  The biggest change is that the performance penalty measured with v1 of this
patchis essentially gone in v10.  The overhead was due to re-creating IndexInfo information unnecessarily, which I
foundexisted in the estate.  I've added a few fields in IndexInfo that are not populated by default but necessary when
checkingexpression 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
beingupdated.  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
ResultRelInfofound in estate.  I've only seen this happen when there is an INSERT ... ON CONFLICT UPDATE ... with
expressionindexes.  I was hopeful that in all cases I could avoid re-creating those when checking expression indexes to
avoidthat 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@amazon.com> wrote:
> On Feb 15, 2025, at 5:49 AM, Matthias van de Meent <boekewurm+postgres@gmail.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
whichsummarizing indexes really need updating just that as a result of being on the HOT path all summarizing indexes
requireupdates. 

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'llwork 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
filteringthe list of IndexInfo to only include indexes with expressions, makes sense.  That way the only indexes in
questionin that function's loop will be those that may spoil the HOT path.  When that list is length 0, we can skip the
testsentirely. 

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
whatyou 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
oneif you had a specific idea.  The swap I've made in v6 really just moves the IndexInfo list to a filtered list with a
newname 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@amazon.com> wrote:
>
> Hello,
>
> I've rebased and updated the patch a bit.  The biggest change is that the performance penalty measured with v1 of
thispatch is essentially gone in v10.  The overhead was due to re-creating IndexInfo information unnecessarily, which I
foundexisted in the estate.  I've added a few fields in IndexInfo that are not populated by default but necessary when
checkingexpression 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@gmail.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@amazon.com> wrote:
>> On Feb 15, 2025, at 5:49 AM, Matthias van de Meent <boekewurm+postgres@gmail.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
whichsummarizing indexes really need updating just that as a result of being on the HOT path all summarizing indexes
requireupdates.
 
> 
> 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
dependingon the requirements within that path.  It may be, and I didn't check, that the order of indexes in that array
hasmeaning in other contexts in the code so I put this aside.  I think I'll keep focused as much as possible and
considerthat 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'llwork 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
whatyou 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@gmail.com> wrote:
> 
> On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregburd@amazon.com> wrote:
>> 
>> Hello,
>> 
>> I've rebased and updated the patch a bit.  The biggest change is that the performance penalty measured with v1 of
thispatch is essentially gone in v10.  The overhead was due to re-creating IndexInfo information unnecessarily, which I
foundexisted in the estate.  I've added a few fields in IndexInfo that are not populated by default but necessary when
checkingexpression 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
itfrom ExecOpenIndices() if there are expressions on the index.  I think that's cleaner and there's precedent for it in
theform 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
thatled 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
existfor 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
specificto the index access method and not well-defined outside that AFAICT.  If that's the case I'd have to augment
thedefinition of an index access method to provide that information.
 

-greg

On Thu, 6 Mar 2025 at 13:40, Burd, Greg <gregburd@amazon.com> wrote:
>
> > On Mar 5, 2025, at 6:39 PM, Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:
> >
> > On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregburd@amazon.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
theyexist for the index.  In the use case I'm solving for we use a custom operator for equality that is not the same as
amemcmp().  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@gmail.com> wrote:
> 
> On Thu, 6 Mar 2025 at 13:40, Burd, Greg <gregburd@amazon.com> wrote:
> 
>> 
>> 
>>> On Mar 5, 2025, at 6:39 PM, Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:
>>> 
>>> On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregburd@amazon.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
informationnecessary to perform the tests in EEIU().  I've debated adding another bool to ExecOpenIndices() to indicate
thatwe're opening indexes for the purpose of an update to avoid building that information in cases where we are not.
Similarto 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
theyexist for the index.  In the use case I'm solving for we use a custom operator for equality that is not the same as
amemcmp().  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@amazon.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@gmail.com> wrote:
>> 
>> On Thu, 6 Mar 2025 at 13:40, Burd, Greg <gregburd@amazon.com> wrote:
>> 
>> 
>>> 
>>> 
>>> 
>>>> On Mar 5, 2025, at 6:39 PM, Matthias van de Meent <boekewurm+postgres@gmail.com> wrote:
>>>> 
>>>> On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregburd@amazon.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
withinformation necessary to perform the tests in EEIU().  I've debated adding another bool to ExecOpenIndices() to
indicatethat we're opening indexes for the purpose of an update to avoid building that information in cases where we
arenot.  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
theyexist for the index.  In the use case I'm solving for we use a custom operator for equality that is not the same as
amemcmp().  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>