Feedback-ID: i675e48f3:Fastmail
MIME-Version: 1.0
Date: Thu, 12 Mar 2026 17:31:47 -0400
From: "Greg Burd" <greg@burd.me>
To: "Nathan Bossart" <nathandbossart@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
 "Jeff Davis" <pgsql@j-davis.com>
Message-Id: <91f4dbe2-21ed-49f3-bebe-270f9bbec9d5@app.fastmail.com>
In-Reply-To: <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>
 <abMjC0jifWB0cs5F@nathan>
Subject: Re: Expanding HOT updates for expression and partial indexes
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/91f4dbe2-21ed-49f3-bebe-270f9bbec9d5%40app.fastmail.com>
Precedence: bulk


On Thu, Mar 12, 2026, at 4:33 PM, Nathan Bossart wrote:
> 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.

Hey Nathan!

Thanks for taking the time to review 0002.

>> 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?

Yes, that is intended.  The 0002 patch is bug fix that I'd hidden along with what is now 0003, I pulled it out for clarity and to discuss independent of the other changes.

>> 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?

Yes, it appears similar to that for a reason but it differs in one key way.  It compares TupleTableSlots, not HeapTuples.

The commit doesn't add another scan, the new code only scans the attributes that ExecGetAllUpdatedCols() didn't pick up earlier and have cached for us at this point.  The intersection between that set and what is indexed is almost always the NULL set because most UPDATEs don't invoke functions via triggers that modify indexed columns using heap_modify_tuple() directly.  But, notably there is the case in tsearch.sql that does.

This introduces almost no net new overhead and when it does in fact do some work it's doing no more than what was done before in HeapDetermineColumnsInfo().

>> 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.

Hard to tell if it was accidental or intentional, more digging required, but I'd bet that others poking in this area noticed the test failure and didn't connect the dots fully and just assumed best practice was to scan all indexed columns, even ones that could not have been updated at all.

Honestly, if we wrote this section from scratch again today I'm better it'd be closer to where my patch takes us than not.

> 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?

Other than the heap_modify_tuple() calls I don't know of something that allows for direct changes but that doesn't matter, 0002 will scan and pick up those attributes even if we introduce a new modification path in the future (as intended).

HeapDetermineColumnsInfo() can't call ExecGetAllUpdatedCols() because that function needs resultRelInfo/EState both not available inside heap (table AM) calls.  Also, the new helper compares TTS, not HeapTuples, which is what we have in heapam_tuple_update(), so not an option

0002 is a both a bug fix (IMO) and a pre-req for 0003 because in the next patch we use the new ExecCompareSlotAttrs() function from within the executor ahead of calling into ExecUpdate().

> -- 
> nathan

Thanks for your time and comments, let me know if you have more. :)

best.

-greg