Re: Expanding HOT updates for expression and partial indexes

> On Mar 5, 2025, at 5:56 PM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> Sorry for the delay. This is a reply for the mail thread up to 17 Feb,
> so it might be very out-of-date by now, in which case sorry for the
> noise.

Never noise, always helpful.

> On Mon, 17 Feb 2025 at 20:54, Burd, Greg <gregburd(at)amazon(dot)com> wrote:
>> On Feb 15, 2025, at 5:49 AM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
>>>
>>> In HEAD, we have a clear indication of which classes of indexes to
>>> update, with TU_UpdateIndexes. With this patch, we have to derive that
>>> from the (lack of) bits in the bitmap that might be output by the
>>> table_update procedure.
>>
>> Yes, but... that "clear indication" is lacking the ability to convey more detailed information. It doesn't tell you which summarizing indexes really need updating just that as a result of being on the HOT path all summarizing indexes require updates.
>
> Agreed that it's not great if you want to know about which indexes
> were meaningfully updated. I think that barring significant advances
> in performance of update checks, we can devise a way of transfering
> this info to the table_tuple_update caller once we get a need for more
> detailed information (e.g. this could be transfered through the
> IndexInfo* that's currently also used by index_unchanged_by_update).

One idea I had and tested a bit was to re-order the arrays of ri_IndexRelationInfo/Desc[] and then have a ri_NumModifiedIndices. This avoided allocation of a Bitmapset and was something downstream code could use or not depending on the requirements within that path. It may be, and I didn't check, that the order of indexes in that array has meaning in other contexts in the code so I put this aside. I think I'll keep focused as much as possible and consider that within the context of another patch later if needed.

>>> I think we can do with an additional parameter for which indexes would
>>> be updated (or store that info in the parameter which also will hold
>>> EState et al). I think it's cheaper that way, too - only when
>>> update_indexes could be TU_SUMMARIZING we might need the exact
>>> information for which indexes to insert new tuples into, and it only
>>> really needs to be sized to the number of summarizing indexes (usually
>>> small/nonexistent, but potentially huge).
>>
>> Okay, yes with this patch we need only concern ourselves with all, none, or some subset of summarizing as before. I'll work on the opaque parameter next iteration.
>
> Thanks!

I still haven't added the opaque parameter as suggested, but it's on my mind to give it a shot.

>>> -----
>>>
>>> I don't see a good reason to add IndexInfo to Relation, by way of
>>> rd_indexInfoList. It seems like an ad-hoc way of passing data around,
>>> and I don't think that's the right way.
>>
>> At one point I'd created a way to get this set via relcache, I will resurrect that approach but I'm not sure it is what you were hinting at.
>
> AFAIK, we don't have IndexInfo in the relcaches currently. I'm very
> hesitant to add an executor node (!) subtype to catalog caches, as
> IndexInfos are also used to store temporary information about e.g.
> index tuple insertion state, which (if IndexInfo is stored in
> relcaches) would imply modifying relcache entries without any further
> locks, and I'm not sure that's at all an OK thing to do.

This is gone in the v10 patch in favor of finding IndexInfo within the EState's ri_IndexRelationInfo[].

Thanks again for your continued support!

-greg