> On Mar 5, 2025, at 5:56 PM, Matthias van de Meent > <boekewurm+postg...@gmail.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 <gregb...@amazon.com> wrote: >> On Feb 15, 2025, at 5:49 AM, Matthias van de Meent >> <boekewurm+postg...@gmail.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
