Re: Expanding HOT updates for expression and partial indexes

> On Mar 5, 2025, at 6:39 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>
> On Wed, 5 Mar 2025 at 18:21, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>>
>> Hello,
>>
>> I've rebased and updated the patch a bit. The biggest change is that the performance penalty measured with v1 of this patch is essentially gone in v10. The overhead was due to re-creating IndexInfo information unnecessarily, which I found existed in the estate. I've added a few fields in IndexInfo that are not populated by default but necessary when checking expression indexes, those fields are populated on demand and only once limiting their overhead.
>
> This review is based on a light reading of patch v10. I have not read
> all 90kB, and am unlikely to finish a full review soon:
>
>> * assumes estate->es_result_relations[0] is the ResultRelInfo being updated
>
> I'm not sure that's a valid assumption. I suspect it might be false in
> cases of nested updates, like
>
> $ UPDATE table1 SET value = other.value FROM (UPDATE table2 SET value
> = 2 ) other WHERE other.id = table1.id;
>
> If this table1 or table2 has expression indexes I suspect it may
> result in this assertion failing (but I haven't spun up a server with
> the patch).
> Alternatively, please also check that it doesn't break if any of these
> two tables is partitioned with multiple partitions (and/or has
> expression indexes, etc.).

Valid, and possible. I'll check and find a way to pass along the known-correct RRI index into that array.

>> * uses ri_IndexRelationInfo[] from within estate rather than re-creating it
>
> As I mentioned above, I think it's safer to pass the known-correct RRI
> (known by callers of table_tuple_update) down the stack.

I think passing the known-correct RRI index is the way to go as I need information from both ri_IndexRelationInfo/Desc[] arrays.

>> * augments IndexInfo only when needed for testing expressions and only once
>
> ExecExpressionIndexesUpdated seems to always loop over all indexes,
> always calling AttributeIndexInfo which always updates the fields in
> the IndexInfo when the index has only !byval attributes (e.g. text,
> json, or other such varlena types). You say it happens only once, have
> I missed something?

There's a test that avoids doing it more than once, but I'm going to rename this as BuildExpressionIndexInfo() and call it from ExecOpenIndices() if there are expressions on the index. I think that's cleaner and there's precedent for it in the form of BuildSpeculativeIndexInfo().

> I'm also somewhat concerned about the use of typecache lookups on
> index->rd_opcintype[i], rather than using
> TupleDescCompactAttr(index->rd_att, i); the latter of which I think
> should be faster, especially when multiple wide indexes are scanned
> with various column types. In hot loops of single-tuple update
> statements I think this may make a few 0.1%pt difference - not a lot,
> but worth considering.

I was just working on that. Good idea.

>> * only creates a local old/new TupleTableSlot when not present in estate
>
> I'm not sure it's safe for us to touch that RRI's tupleslots.

Me neither, that's why I mentioned it. It was my attempt to avoid the work to create/destroy temp slots over and over that led to that idea. It's working, but needs more thought.

>> * retains existing summarized index HOT update logic
>
> Great, thanks!
>
> Kind regards,
>
> Matthias van de Meent
> Neon (https://neon.tech)

I might widen this patch a bit to include support for testing equality of index tuples using custom operators when they exist for the index. In the use case I'm solving for we use a custom operator for equality that is not the same as a memcmp(). Do you have thoughts on that? It may be hard to accomplish this as the notion of an equality operator is specific to the index access method and not well-defined outside that AFAICT. If that's the case I'd have to augment the definition of an index access method to provide that information.

-greg