public inbox for [email protected]
help / color / mirror / Atom feedFrom: Greg Burd <[email protected]>
To: Jeff Davis <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Expanding HOT updates for expression and partial indexes
Date: Mon, 16 Mar 2026 12:23:04 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<akciabcu3b2hchj7adxhu4kovfaozp2pcn2z7sdljfthxcyg4o@7e6sfyzipvyy>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<abMjC0jifWB0cs5F@nathan>
<[email protected]>
<[email protected]>
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
view thread (44+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Expanding HOT updates for expression and partial indexes
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox