Feedback-ID: i675e48f3:Fastmail
Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: Expanding HOT updates for expression and partial indexes
From: Greg Burd <greg@burd.me>
In-Reply-To: <aObOLg3JsebbwjOU@nathan>
Date: Thu, 9 Oct 2025 16:57:07 -0400
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <600660E1-1A1F-49F0-A64C-BFF143241492@burd.me>
References: <97f0aa72-f172-4673-8b04-533f022c3149@app.fastmail.com>
 <CF007F8C-5FCA-4FC5-A7EC-282F8C9D413C@greg.burd.me> <aObOLg3JsebbwjOU@nathan>
To: Nathan Bossart <nathandbossart@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/600660E1-1A1F-49F0-A64C-BFF143241492%40burd.me>
Precedence: bulk


> On Oct 8, 2025, at 4:48=E2=80=AFPM, Nathan Bossart =
<nathandbossart@gmail.com> wrote:
>=20
> On Tue, Oct 07, 2025 at 05:36:11PM -0400, Greg Burd wrote:
>> I put the patch aside for a while, then this past week at =
PGConf.dev/NYC
>> I heard interest from a few people (Jeff Davis, Nathan Bossart) who
>> encouraged me to move the code executing the expressions to just =
before
>> acquiring the lock but after pinning the buffer. The theory being =
that
>> my new code using the old/new tts to form and test the index tuples
>> resulting from executing expressions was using the resultsRelInfo =
struct
>> created during plan execution, not the information found on the page,
>> and so was safe without the lock.

Thanks for taking a look Nathan.
>=20
> An open question (at least from me) is whether this is safe.  I'm not
> familiar enough with this area of code yet to confidently determine =
that.

My read is that it is safe because we're testing the content of two
TupleTableSlots both formed in the executor.  The function uses only
that information and doesn't reference data on the page at all.

>> After reviewing how updates work in the executor, I discovered that
>> during execution the new tuple slot is populated with the information
>> from ExecBuildUpdateProjection() and the old tuple, but that most
>> importantly for this use case that function created a bitmap of the
>> modified columns (the columns specified in the update).  This bitmap
>> isn't the same as the one produced by HeapDetermineColumnsInfo() as =
the
>> latter excludes attributes that are not changed after testing =
equality
>> with the helper function heap_attr_equals() where as the former will
>> include attributes that appear in the update but are the same value =
as
>> before.  This, happily, is immaterial for the purposes of my function
>> ExecExprIndexesRequireUpdates() which simply needs to check to see if
>> index tuples generated are unchanged.  So I had all I needed to run =
the
>> checks ahead of acquiring the lock on the buffer.
>=20
> Nice.

Handy indeed.  I'm not at all a fan of increasing the size of a plan =
node
but it's only by a little... and for a good cause.

>> There is much room for improvement, and your suggestions are welcome.
>=20
> A general and predictable suggestion is to find ways to break this =
into
> smaller pieces.  As-is, this patch would take me an enormous amount of =
time
> to review in any depth.  If we can break off some smaller pieces that =
we
> can scrutinize and commit independently, we can start making forward
> progress sooner.  The UpdateContext and reloption stuff are examples =
of
> things that might be possible to split into independent patches.

Fair, I'll try.

>> I'll find time to quantify the benefit of this patch for the targeted
>> use cases and to ensure that all other cases see no regressions.
>=20
> Looking forward to these results.  This should also help us decide =
whether
> to set expression_checks by default.

In past my test results were very positive for cases where this helped =
avoid
heap and index bloat and almost immeasurably small even for cases where =
we
were doing the work to test but ultimately unable to take the HOT path.

This will require new tests as the code has changed quite a bit.

>> I added a reloption "expression_checks" to disable this new code =
path.=20
>> Good idea or bad precedent?
>=20
> If there are cases where the added overhead outweighs the benefits =
(which
> seems like it must be true some of the time), then I think we must =
have a
> way to opt-out (or maybe even opt-in).  In fact, I'd advise adding a =
GUC to
> complement the reloption so that users can configure it at higher =
levels.

This evolved from a GUC to a reloption and I'd rather it go away =
entirely.
I hear your concern, but I've yet to measure a perceptable impact and =
I'll
try hard to keep it that way as this matures.  Assuming that's the case,
I'd like to eliminate the potentially confusing tuning knob.

>> In execIndexing I special case for IsolationIsSerializable() and I =
can't
>> remember why now but I do recall one isolation test failing... I'll
>> check on this and get back to the thread.  Or maybe you know why that
>=20
> I didn't follow this.

More later if/when I can reproduce it and understand it better myself.

>> I'd like not to build, then rebuild index tuples for these =
expressions
>> but I can't think of a way to do that without a palloc(), this is
>> avoided today.
>=20
> Is the avoidance of palloc() a strict rule?  Is this discussed in the =
code
> anywhere?

Not that I know of, just my paranoid self trying to avoid it on a path =
that
didn't have it before.

> --=20
> nathan

best.

-greg=