Message-ID: <7b8681a578b5fe77103948eeb7b5cdd80fabad5d.camel@j-davis.com>
Subject: Re: Expanding HOT updates for expression and partial indexes
From: Jeff Davis <pgsql@j-davis.com>
To: Greg Burd <greg@burd.me>, Nathan Bossart <nathandbossart@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Date: Mon, 16 Mar 2026 10:55:26 -0700
In-Reply-To: <b0404aab-65a1-4922-9cff-986163ad70bb@app.fastmail.com>
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>
	 <91f4dbe2-21ed-49f3-bebe-270f9bbec9d5@app.fastmail.com>
	 <811d6f42e5481935943556b692859aae9146d4c9.camel@j-davis.com>
	 <b0404aab-65a1-4922-9cff-986163ad70bb@app.fastmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.3-0ubuntu1.1 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/7b8681a578b5fe77103948eeb7b5cdd80fabad5d.camel%40j-davis.com>
Precedence: bulk

On Mon, 2026-03-16 at 12:23 -0400, Greg Burd wrote:
> Hello Jeff, thanks for taking a look! :)

Hi, thank you for working on this problem!

> > Why do extra work in ExecBRUpdateTriggers() to eliminate the false
> > negative case if we don't rely on it anyway? If we do need to rely
> > on
> > it in subsequent patches, then we need to be sure, right?
>=20
> Later commits do currently rely on it, ExecUpdateModifiedIdxAttrs()
> uses it in the next commit (0003) to avoid reviewing indexed
> attributes that could not have possibly changed.

OK. The first half of the commit message for 0002 is slightly confusing
because it's referring to pre-existing behavior, behavior changed by
the commit, and also future work. It might help to clarify the tenses
like:

 - Previously, ExecGetAllUpdatedCols() had gaps ..., but not a real bug
because ...
 - This commit closes those gaps by updating ri_extraUpdatedCols in
ExecBRUpdateTriggers(), making ExecGetAllUpdatedCols() reliable.
 - We know there are no other gaps because ...
 - Useful to fix because later work will rely on it for [very brief
reason]


> =C2=A0 Imagine a table with a lot of indexes where updates only modify on=
e
> or two at a time.=C2=A0 Why are we testing indexed attributes for changes
> in HeapDeterminColumnsInfo() that couldn't have changed?=C2=A0 The answer
> is that a) HeapDeterminColumnsInfo() lives in heap, not the executor
> (see patch 0003) so it has no ability to call
> ExecGetAllUpdatedCols(), and b) the set returned by
> ExecGetAllUpdatedCols() is sometimes incomplete.

That's helpful, thank you.

> What do we "need to be sure" of?=C2=A0 That ExecGetAllUpdatedCols() not
> really contains all attributes that its name implies?=C2=A0 I think it no=
w
> does that after 0002, do you disagree?

I don't disagree, but I think we need some kind statement that we
believe that it's true, and a brief explanation why. (I don't have much
of an opinion about whether it's in this thread, the commit message, or
the code.)

>=20
> I think it is a new guarantee that was implied before now but not
> required until 0003.=C2=A0 I think this change reduces overhead and helps
> to avoid some future security feature that depends on
> ExecGetAllUpdatedCols() to provide that guarantee.
>=20
> Does that make sense?

A subtlety here is that perhaps ExecGetAllUpdatedCols() already *was*
correct, and it just meant something different than we thought: the
*targeted* columns of an update, instead of the actually-updated
values.

If so we should think about whether that distinction should be
preserved. For instance, column filtering for triggers should be based
on the targeted columns (rather than actually-updated values) because,
semantically, it should still fire even for a no-op update. Perhaps
similar for choosing the lock mode?

>=20
Regards,
	Jeff Davis