Date: Tue, 17 Mar 2026 10:22:02 -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: <ablxmmcbA_8UFjiN@nathan>
References: <ff2276c5-28f7-4e09-a6ae-40137ceddb67@app.fastmail.com>
 <872b875c-0aa4-4269-9c84-532227b32361@app.fastmail.com>
 <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <427bfbda-c901-49e4-b725-8ddc41bec23d@app.fastmail.com>
Archived-At: <https://www.postgresql.org/message-id/ablxmmcbA_8UFjiN%40nathan>
Precedence: bulk

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