Re: Expanding HOT updates for expression and partial indexes

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.

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

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

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

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

> + /*
> + * 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.

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

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

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.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

[0] https://commitfest.postgresql.org/patch/6309/