On Thu, 13 Feb 2025 at 19:46, Burd, Greg <gregb...@amazon.com> wrote: > > Attached find an updated patchset v5 that is an evolution of v4. > > Changes v4 to v5 are: > * replaced GUC with table reloption called "expression_checks" (open to other > name ideas) > * minimal documentation updates to README.HOT to address changes > * avoid, when possible, the expensive path that requires evaluating an estate > using bitmaps > * determines the set of summarized indexes requiring updates, only updates > those > * more tests in heap_hot_updates.sql (perhaps too many...) > * rebased to master, formatted, and make check-world passes
Thank you for the update. Below some comments, in no particular order. ----- I'm not a fan of how you replaced TU_UpdateIndexes with a bitmap. It seems unergonomic and a waste of performance. In HEAD, we don't do any expensive computations for the fast path of "indexed attribute updated" - at most we do the bitmap compare and then set a pointer. With this patch, the way we signal that is by allocating a bitmap of size O(n_indexes). That's potentially quite expensive (given 1000s of indexes), and definitely more expensive than only a pointer assignment. 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. 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). ----- I think your patch design came from trying to include at least two distinct optimizations: 1) to make the HOT-or-not check include whether the expressions of indexes were updated, and 2) to only insert index tuples into indexes that got updated values when table_tuple_update returns update_indexes=TU_Summarizing. While they touch similar code (clearly seen here), I think those should be implemented in different patches. For (1), the current API surface is good enough when the EState is passed down. For (2), you'll indeed also need an additional argument we can use to fill with the right summarizing indexes, but I don't think that can nor should replace the function of TU_UpdateIndexes. If you agree with my observation of those being distinct optimizations, could you split this patch into parts (but still within the same series) so that these are separately reviewable? ----- I notice that ExecIndexesRequiringUpdates() does work on all indexes, rather than just indexes relevant to this exact phase of checking. I think that is a waste of time, so if we sort the indexes in order of [hotblocking without expressions, hotblocking with expressions, summarizing], then (with stored start/end indexes) we can save time in cases where there are comparatively few of the types we're not going to look at. As an extreme example: we shouldn't do the (comparatively) expensive work evaluating expressions to determine which of 1000s of summarizing indexes has been updated when we're still not sure if we can apply HOT at all. (Sidenote: Though, arguably, we could be smarter by skipping index insertions into unmodified summarizing indexes altogether regardless of HOT status, as long as the update is on the same page - but that's getting ahead of ourselves and not relevant to this discussion.) ----- I noticed you've disabled any passing of "HOT or not" in the simple_update cases, and have done away with the various checks that are in place to prevent corruption. I don't think that's a great idea, it's quite likely to cause bugs. ----- You're extracting type info from the opclass, to use in datum_image_eq(). Couldn't you instead use the index relation's TupleDesc and its stored attribute information instead? That saves us from having to do further catalog lookups during execution. I'm also fairly sure that that information is supposed to be a more accurate representation of attributes' expression output types than the opclass' type information (though, they probably should match). ----- The operations applied in ExecIndexesRequiringUpdates partially duplicate those done in index_unchanged_by_update. Can we (partially) unify this, and pass which indexes were updated through the IndexInfo, rather than the current bitmap? ----- 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. >>>> I mean, it's clear that "updated indexed column's value == non-HOT >>>> update". And that to determine whether an updated *projected* column's >>>> value (i.e., expression index column's value) was actually updated we >>>> need to calculate the previous and current index value, thus execute >>>> the projection twice. But why would we have significant additional >>>> overhead if there are no expression indexes, or when we can know by >>>> bitmap overlap that the only interesting cases are summarizing >>>> indexes? >> >> See the attached approach. Evaluation of the expressions only has to >> happen if there are any HOT-blocking attributes which are exclusively >> hot-blockingly indexed through expressions, so if the updated >> attribute numbers are a subset of hotblockingexprattrs. (substitute >> hotblocking with summarizing for the summarizing approach) > > I believe I've incorporated the gist of your idea in this v5 patch, let me > know if I missed something. Seems about accurate. >>>> I would've implemented this with (1) two new bitmaps, one each for >>>> normal and summarizing indexes, each containing which columns are >>>> exclusively used in expression indexes (and which should thus be used >>>> to trigger the (comparatively) expensive recalculation). >>> >>> That was one where I started, over time that became harder to work as the >>> bitmaps contain the union of index attributes for the table not per-column. >> >> I think it's fairly easy to create, though. >> >>> Now there is one bitmap to cover the broadest case and then a function to >>> find the modified set of indexes where each is examined against bitmaps >>> that contain only attributes specific to the index in question. This >>> helped in cases where there were both expression and non-expression indexes >>> on the same attribute. >> >> Fair, but do we care about one expression index on (attr1->>'data')'s >> value *not* changing when an index on (attr1) exists and attr1 has >> changed? That index on att1 would block HOT updates regardless of the >> (lack of) changes to the (att1->>'data') index, so doing those >> expensive calculations seems quite wasteful. > > Agreed, when both a non-expression and an expression index exist on the same > attribute then the expression checks are unnecessary and should be avoided. > In this v5 patchset this case becomes two checks of bitmaps (first hot_attrs, > then exclusively exp_attrs) before proceeding with a non-HOT update. > > So, in my opinion, we should also keep track of those attributes only > > included in expressions of indexes, and that's fairly easy: see > > attached prototype.diff.txt (might need some work, the patch was > > drafted on v16's codebase, but the idea is clear). > > Thank you for your patch, I've included and expanded it.
