Re: Expanding HOT updates for expression and partial indexes - Mailing list pgsql-hackers

> On Oct 8, 2025, at 4:48 PM, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> 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.
>
> 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.
>
> 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.
>
> 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.
>
> 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.
>> Good idea or bad precedent?
>
> 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
>
> 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.
>
> 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.

> --
> nathan

best.

-greg