Date: Thu, 12 Mar 2026 15:33:15 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Greg Burd <greg@burd.me>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
	Jeff Davis <pgsql@j-davis.com>
Subject: Re: Expanding HOT updates for expression and partial indexes
Message-ID: <abMjC0jifWB0cs5F@nathan>
References: <fc17cbe85c24ff9d3cf43bfb37e6ca0d483fb2b4.camel@j-davis.com>
 <e1d06b18-2ebb-466e-bc0f-5d426d17a32e@app.fastmail.com>
 <akciabcu3b2hchj7adxhu4kovfaozp2pcn2z7sdljfthxcyg4o@7e6sfyzipvyy>
 <b8e07500-587d-4360-8f9e-89b14d1c0edc@app.fastmail.com>
 <bce13827-5f98-4d79-b779-ac5fff2b7668@app.fastmail.com>
 <c435d703af02097ec19680c2f8d4479292a3d176.camel@j-davis.com>
 <9bb9bdd6-e1fe-48fe-837d-4d0289396f1c@app.fastmail.com>
 <bed3fd08-ebfd-4980-801e-bff245a27dd7@app.fastmail.com>
 <ff2276c5-28f7-4e09-a6ae-40137ceddb67@app.fastmail.com>
 <872b875c-0aa4-4269-9c84-532227b32361@app.fastmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <872b875c-0aa4-4269-9c84-532227b32361@app.fastmail.com>
Archived-At: <https://www.postgresql.org/message-id/abMjC0jifWB0cs5F%40nathan>
Precedence: bulk

On Wed, Mar 11, 2026 at 11:51:03AM -0400, Greg Burd wrote:
> 0002 - This patch plugs a hole (bug?) in ExecGetAllUpdatedCols() which is
> triggered by an existing test in tsearch.sql and the
> tsvector_update_trigger().  That trigger uses heap_modify_tuple() to
> change an indexed attribute that is not discovered by
> ExecGetAllUpdatedCols(), which seems odd to me at best and at worst wrong
> (or even a potential security issue).  This patch finds and adds columns
> that are updated into the Bitmapset returned by ExecGetAllUpdatedCols().
> The patch includes a helper function ExecCompareSlotAttrs() that will be
> used in follow-on patches as well.

I just looked at this one for now.

> The net is that the functions like HeapDetermineColumnsInfo() have to
> scan all indexed attributes for changes rather than being able to first
> reduce the indexed set by intersecting it with the set of attributes
> known to be potentially updated.

I noticed the patch doesn't update HeapDetermineColumnsInfo() accordingly.
Is that intended?

> This commit introduces ExecCompareSlotAttrs() as a utility function to
> identify those attributes that have changed.  It compares a subset of
> attributes between two TupleTableSlots and returns a Bitmapset of
> attributes that differ.

Hm.  Most of this new function looks duplicated from
HeapDetermineColumnsInfo(), so IIUC this commit effectively adds another
scan through all the attributes.  Does this produce noticeably more
overhead?

> It would be nice to integrate this into HeapDetermineColumnsInfo(),
> however it would be a layering violation given that it is within
> heap_update().

It'd be good to understand whether the current behavior is intentional or
just a happy accident.  I found commit 2fd8685e7f, which looks like it was
intended as a prerequisite for the WARM feature (which I don't think was
ever committed).  And it seems to have scanned through all indexed columns
when HOT was first introduced in commit 282d2a03dd.

I'm also curious whether anything else could modify columns that won't be
discovered by ExecGetAllUpdatedCols().  Having HeapDetermineColumnsInfo()
scan everything seems like a defense against such things, which is perhaps
why you've left it unchanged in the patch.  I haven't looked into 0003 yet.
Is 0002 a prerequisite for that or a separate fix?

-- 
nathan