Re: Expanding HOT updates for expression and partial indexes - Mailing list pgsql-hackers

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
anythingchanged in a before-row trigger via heap_modify_tuple() as happens in tsearch.sql testing.  However, that
functionproduces a bitmapset of the *targeted* attributes which applies to *all* tuples being updated (when there is
morethan one in the UPDATE), not just one.  My change added a attribute when it changed in a specific tuple, which may
notbe true for all tuples.  So 0002 would have had to change to fix that bug by re-discovering any modified attributes
foreach tuple.  That seems bad and the more that I looked at it the more I felt that the simple approach of just
scanningall 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
heapspecific).  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
editorconfig. ;-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