Re: Expanding HOT updates for expression and partial indexes

On Monday, October 19th, 2026 at 5:10 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:

> On Sun, 18 Oct 2026, 13:00 Greg Burd, <greg(at)burd(dot)me> wrote:
> >
> >
> > On Friday, March 27th, 2026 at 1:01 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> >
> > > On Wed, 2026-03-25 at 12:16 -0500, Nathan Bossart wrote:
> > > > I just spoke to Greg off-list and wanted to share my current thoughts
> > > > on
> > > > the list as well. In short, while we feel that the patch is in
> > > > decent
> > > > shape and seems to be performance neutral (or maybe even positive in
> > > > some
> > > > cases), it obviously doesn't accomplish $subject,
> > >
> > > Agreed. I like the direction this is going, but if we can't accomplish
> > > $subject in 19, then let's move to the next cycle.
> >
> > Wow, is it already mid-v20 cycle? Time flies when you're having fun.
> >
> > > Regards,
> > > Jeff Davis
> >
> > And we're back!
> >
> > It has been a bit since the last patch that went out in March. Attached
> > is v39, eight patches on master (d733fcaf5ca), which now actually does what
> > the $subject says and the speed-ups are measured.
>
> Below is an incomplete review, which I was working on during travel to
> PGEU until I noticed v41. It might be in a bit of a rough shape, but
> it gets some comments in that I think are important, and some others
> that might be useful.

Hello Matthais,

Thanks for taking a look, and on the way to PGEU no less. Your review
is of v39; I've answered it against v42, attached, which is back to the
eight patches v39 had. 0009 (exclusion constraints) and 0010 (datum
reuse) from v41 are out of this set for now, so that the review can stay
on the core of it; they'll come back separately.

> > What's interesting is that a few patches in this series have had a different
> > life in a separate thread [1], I need them for both so I guess I'll just add
> > them to both and sort it out later. If that's not The Way(TM) to do this
> > then let me know what is. :)
> >
> > Changes since v38:
> >
> > 0001 HOT test coverage (was v38-0001). Tests only, with master's expected
> > output, so each later patch's behavior change shows up as an
> > expected-output diff. Adds Tom's 2018 gist point->box crash case and
> > an EvalPlanQual isolation spec.
>
> I did not study this test suite in detail.

Fair. One change worth knowing about: the cases that only check which
updates are HOT now live in src/test/regress (hot_updates), and
pageinspect keeps only the HOT chain checks.

> > 0002 Identify modified indexed attributes in the executor (was v38-0002),
> > with Nathan's and Jeff's review comments addressed.
>
> > INDEX_ATTR_BITMAP_HOT_BLOCKING becomes INDEX_ATTR_BITMAP_INDEXED, the
>
> Did you check the history of the HOT_BLOCKING vs SUMMARIZED? I think
> I had a version of the amsummarizing patch where I used the same
> definitions you're introducing, but eventually it got committed with
> the current split. I don't remember the exact reasons from that
> thread, but if there were, maybe they're still relevant?
>
> If it didn't make it onto the list, I suspect it may have been
> concerns about performance, both of building the bitmaps (more because
> it requires more bitmap operations which might never actually be used)
> and because the common path of testing if HOT does _not_ apply now
> requires 2 bitmap operations (both the indexed and the amsummarizing
> bitmap must be checked, assuming an indexed column was modified).
> There may even have been an iteration with SUMMARIZED + HOT_BLOCKING +
> INDEXED that I considered to waste too many bytes in the relcache, but
> I don't have the energy to go through that history right now.

I went back through it. 5753d4ee320 replaced INDEX_ATTR_BITMAP_ALL with
HOT_BLOCKING, e3fcca0d0d2 reverted that, and 19d8e2308bc (your v2 in
the "Ignoring BRIN for HOT updates" thread) brought it back with the
HOT_BLOCKING / SUMMARIZED split, moving the columns that summarizing
indexes use in predicates into SUMMARIZED. I didn't find the INDEXED
iteration you remember on the list, so it must have been off-list.

v42 drops the rename. HOT_BLOCKING and SUMMARIZED keep the names and
meanings they have on master, and INDEXED is added as their union,
computed once when the relcache builds the bitmaps (one bms_union) and
cached. The executor compares old and new values over INDEXED, one
fetch where master's heap_update() fetched HOT_BLOCKING, SUMMARIZED, KEY
and IDENTITY_KEY and unioned them into interesting_attrs; the heap side
still fetches those for the HOT, lock mode and replica identity
decisions, as master does. The HOT test is master's: no overlap with
HOT_BLOCKING, and only then SUMMARIZED. So the path where HOT doesn't
apply costs one overlap test, as it does today. The extra relcache
bytes are one Bitmapset per relation with indexes.

> > - TM_FailureData *tmfd, LockTupleMode *lockmode,
> > - TU_UpdateIndexes *update_indexes)
> > + TM_FailureData *tmfd, const LockTupleMode lockmode,
> > + const Bitmapset *modified_idx_attrs, const bool hot_allowed)
>
> I think this is the first time I see pass-by-value arguments being
> const-qualified. Is this intentional? There's nothing wrong with it
> on a technical level, but it doesn't really do anything for the
> function's interface except exposing what the function internally can
> (or can't) do with the arguments under those names, without actually
> making sure the function doesn't modify the values, which limits what
> the implementer of the function can do without providing any practical
> guarantees to the caller.

Not intentional, and you're right that it says nothing to the caller.
Gone: it's LockTupleMode lockmode and bool hot_allowed now.

> > @@ -3444,16 +3424,59 @@ heap_update(Relation relation, const ItemPointerData *otid, HeapTuple newtup,
> > + * ExtractReplicaIdentity() needs to know if a modified indexed attrbute
> > + * is used as a replica indentity or if any of the replica identity
> Spelling: attrbute -> attribute, indentity -> identity

Fixed.

> > + /*
> > + * Reduce the set under review to only the unmodified indexed replica
> > + * identity key attributes. idx_attrs is copied (by bms_difference())
> > + * not modified here.
> > + */
> > + attrs = bms_difference(idx_attrs, modified_idx_attrs);
> > + attrs = bms_int_members(attrs, id_attrs);
>
> Why shouldn't we log the replica identity if the identity columns are
> not part of an index? E.g. a table without indexes with IDENTITY FULL?
> I think we should still inline the toasted columns for such table's
> tuples, right? I think the current patch (erronously) doesn't do
> that.

We still do; this check never decided that case. For REPLICA IDENTITY
FULL, RelationGetIndexAttrBitmap(INDEX_ATTR_BITMAP_IDENTITY_KEY) is
empty, so this set is empty and key_required comes out false, as it
does on master. But ExtractReplicaIdentity() handles FULL before it
looks at key_required: it logs the whole old tuple and
toast_flatten_tuple()s it if it has external values. For DEFAULT and
USING INDEX the identity columns are the columns of an index, so they
are always indexed attributes. So this computes what master's
HeapDetermineColumnsInfo() did for id_has_external: an unmodified
identity column stored externally in the old tuple.

That also made the intersection with idx_attrs a no-op. It's gone: the
loop now runs over bms_difference(id_attrs, modified_idx_attrs) and
nothing else. test_decoding's toast test covers FULL with toasted
columns, and 032_subscribe_use_index updates a FULL table through
logical replication; both pass.

> > + while ((attidx = bms_next_member(attrs, attidx)) >= 0)
> > + {
> [...]
> > + value = heap_getattr(&oldtup, attrnum, tupdesc, &isnull);
>
> This will do up to O(nheapattrs^2) work if any attributes are varlen
> or null, which will be very expensive in wide tables. Can you avoid
> this, e.g. by reusing some of slot_deform_heap_tuple's tricks keeping
> offset state across attribute retrievals?

This loop only visits the replica identity columns the update left
unchanged, and only when the update changed none of them, so it's
O(k * natts) for k identity columns, usually one or two. Master did
more here: HeapDetermineColumnsInfo() called heap_getattr() on both
tuples for every indexed, key and identity column, under the buffer
lock. The comparison of all indexed columns now runs in the executor
on the slots, with slot_getattr(), so it deforms each tuple once,
incrementally, keeping the offset state as you suggest.

The one place that still loops with heap_getattr() over every indexed
column is simple_heap_update(), which has no slots; that's master's
loop moved out of heap_update(), with its "XXX this is pretty
inefficient" comment intact. It would be easy to heap_deform_tuple()
both tuples once up to the last indexed attribute there. I left it
alone to keep the move a move, but I'll do it if you think it's worth
it in this patch.

> > 0003
> > 0004
> > 0005
> Haven't checked these.
>
> > 0006-0008 New: nbtree - datum_image_eq, GIN - same extracted keys, and hash
> > - same hash code, implementations for amcomparedatums.
>
> I think `amcomparedatums` can only be implemented correctly* for
> !amcanreturn columns, which to me means an amcomparedatums
> implementation for nbtree is bogus, or at least putting the cart
> before the horse.
> Toast & HOT themselves should use datum_image_eq if we want HOT to
> treat differently toasted attribute values with the same datum image
> as unchanged -- and indeed, that's an obvious limitation of the
> toast+hot test system that I noticed in my thread for
> SQL-pg_datum_image_equal [0], too.
>
> * That is, a non-datum_image_eq implementation of the function.

I agree. btcomparedatums() is datum_image_eq() and nothing else, so it
tells us nothing about btree; it tells us the generic comparison is too
strict. And the reasoning isn't btree's: any index's entries are a
function of the value's image, so two values with the same image give
every index the same entries.

So for the next version I'd like to do it your way:

- the executor's attribute comparison falls back to datum_image_eq()
when the raw bytes differ, for every indexed column, which covers
btree, GiST, SP-GiST and any extension AM without a callback;
- 0006 goes away;
- amcomparedatums stays only as an answer coarser than the datum image
(the same hash code, the same set of GIN keys), and the executor
doesn't ask it for a column the index can return
(amcanreturn/amproperty "returnable"), so an AM that later learns
index-only scans can't be left with a wrong answer.

That narrows the callback's contract to what you describe, and keeps
your rule enforced by the caller rather than by each AM.

> Note: Forcing detoasting is expensive, so forcing detoasting in btree
> would negate most of TOAST's "optimization" around not checking
> to-be-replaced external attribute values for image equivalence with
> the new values.

Yes. Two things keep the cost down today, and both would carry over.
datum_image_eq() is only reached once datumIsEqual() has said the raw
bytes differ, so an unchanged value that keeps its toast pointer is
never fetched. And datum_image_eq() compares toast_raw_datum_size()
first, so values of different length don't detoast either. What's left
is a change to a toasted, indexed column that keeps the length, where
both values get detoasted; today that update isn't HOT and the index
insertion detoasts the new value anyway, so the extra cost is the old
value's fetch. I haven't measured that case on its own yet. I'll add
it to the benchmarks before changing the comparison, since your concern
is exactly that.

> Kind regards,
>
> Matthias van de Meent
> Databricks (https://www.databricks.com)
>
> [0] https://commitfest.postgresql.org/patch/6309/

Thanks again, this is the sort of thing I needed to hear before going
further with amcomparedatums.

best.

-greg