Date: Mon, 23 Mar 2026 13:39:55 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Greg Burd <greg@burd.me>
Cc: Jeff Davis <pgsql@j-davis.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Expanding HOT updates for expression and partial indexes
Message-ID: <acGI-wnW4NxS87e0@nathan>
References: <abMjC0jifWB0cs5F@nathan>
 <91f4dbe2-21ed-49f3-bebe-270f9bbec9d5@app.fastmail.com>
 <811d6f42e5481935943556b692859aae9146d4c9.camel@j-davis.com>
 <b0404aab-65a1-4922-9cff-986163ad70bb@app.fastmail.com>
 <7b8681a578b5fe77103948eeb7b5cdd80fabad5d.camel@j-davis.com>
 <4f48f75d-4f2a-4240-b66d-597517796e02@app.fastmail.com>
 <3fba3f5671eddb221ba38f5a12acbe7cad27edf3.camel@j-davis.com>
 <427bfbda-c901-49e4-b725-8ddc41bec23d@app.fastmail.com>
 <887b1ee62794553a95c3578fcdc0cc1831e68b27.camel@j-davis.com>
 <d7489d3e-2cbd-4b0f-b662-b5c4386a3f1e@app.fastmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d7489d3e-2cbd-4b0f-b662-b5c4386a3f1e@app.fastmail.com>
Archived-At: <https://www.postgresql.org/message-id/acGI-wnW4NxS87e0%40nathan>
Precedence: bulk

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