Re: Expanding HOT updates for expression and partial indexes

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