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
