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