Catching up here. I see that you dropped 0002. Can you explain why that's no longer needed?
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. > 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. 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? > - * 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. > @@ -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". > - 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... -- nathan
