Thanks for the new patch. As a general note, please be sure to run
pgindent on patches. My review is still rather surface-level, sorry.
On Tue, Mar 17, 2026 at 02:04:11PM -0400, Greg Burd wrote:
> - id_attrs = RelationGetIndexAttrBitmap(relation,
> -
> INDEX_ATTR_BITMAP_IDENTITY_KEY);
> [...]
> + rid_attrs = RelationGetIndexAttrBitmap(relation,
> INDEX_ATTR_BITMAP_IDENTITY_KEY);
I'm nitpicking, but it took me a while to parse the
replica-identity-related code in heap_update() until I discovered that this
variable was renamed. I think we ought to leave the name alone.
> /*
> * At this point newbuf and buffer are both pinned and locked, and
> newbuf
> - * has enough space for the new tuple. If they are the same buffer,
> only
> - * one pin is held.
> + * has enough space for the new tuple so we can use the HOT update path
> if
> + * the caller determined that it is allowable.
> + *
> + * NOTE: If newbuf == buffer then only one pin is held.
> */
> -
> if (newbuf == buffer)
Sorry, more nitpicks. In addition to the unnecessary removal of the blank
line, I'm not sure the changes to this comment are needed.
> - /*
> - * If it is a HOT update, the update may still need to update summarized
> - * indexes, lest we fail to update those summaries and get incorrect
> - * results (for example, minmax bounds of the block may change with this
> - * update).
> - */
> - if (use_hot_update)
> - {
> - if (summarized_update)
> - *update_indexes = TU_Summarizing;
> - else
> - *update_indexes = TU_None;
> - }
> - else
> - *update_indexes = TU_All;
So, the "HOT but still need to update summarized indexes" code has been
moved from heap_update() to HeapUpdateHotAllowable(), which is called by
heap_update()'s callers (i.e., simple_heap_update() and
heapam_tuple_update()). That looks correct to me at a glance.
> -simple_heap_update(Relation relation, const ItemPointerData *otid, HeapTuple
> tup,
> +simple_heap_update(Relation relation, const ItemPointerData *otid, HeapTuple
> tuple,
nitpick: This variable name change looks unnecessary.
> @@ -944,8 +946,13 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo,
> if (rel->rd_rel->relispartition)
> ExecPartitionCheck(resultRelInfo, slot, estate, true);
>
> + modified_idx_attrs = ExecUpdateModifiedIdxAttrs(resultRelInfo,
> +
> estate, searchslot, slot);
> +
> simple_table_tuple_update(rel, tid, slot, estate->es_snapshot,
> -
> &update_indexes);
> +
> modified_idx_attrs, &update_indexes);
> + bms_free(modified_idx_attrs);
I don't know how constructive of a comment this is, but this change in
particular seems quite out of place. It feels odd to me that we expect
callers of simple_table_tuple_update() to determine the
modified-index-attributes. I guess I'm confused why this work doesn't
belong one level down, i.e., in the tuple_update function.
> - * INDEX_ATTR_BITMAP_SUMMARIZED Columns included in summarizing indexes
> + * INDEX_ATTR_BITMAP_INDEXED Columns referenced by indexes
> + * INDEX_ATTR_BITMAP_SUMMARIZED Columns only included in summarizing
> indexes
> - Bitmapset *summarizedattrs; /* columns with summarizing indexes */
> + Bitmapset *indexedattrs; /* columns referenced by indexes */
> + Bitmapset *summarizedattrs; /* columns only in summarizing indexes
> */
As before, the comment changes for the summarized-attr-related stuff seem
unnecessary.
> if (indexDesc->rd_indam->amsummarizing)
> attrs = &summarizedattrs;
> else
> - attrs = &hotblockingattrs;
> + attrs = &indexedattrs;
> + /*
> + * Record what attributes are only referenced by summarizing indexes.
> Then
> + * add that into the other indexed attributes to track all referenced
> + * attributes.
> + */
> + summarizedattrs = bms_del_members(summarizedattrs, indexedattrs);
> + indexedattrs = bms_add_members(indexedattrs, summarizedattrs);
The difference between hotblockingattrs and indexedattrs seems quite
subtle. Am I understanding correctly that indexedattrs is essentially just
hotblockingattrs + summarizedattrs? And that this is all meant for
INDEX_ATTR_BITMAP_INDEXED?
- INJECTION_POINT("heap_update-before-pin", NULL);
+ INJECTION_POINT("simple_heap_update-before-pin", NULL);
Why was this changed in heap_update()?
--
nathan
