Matthias, First off, I can't thank you enough for taking the time to review in detail the patch. I appreciate and value your time and excellent feedback.
Second, I think that I should admit to the fact that I've also been working on
making PHOT functional again. I have it rebased against master, however it is
still at the proof-of-concept phase and there are some larger issues to flesh
out. One of the changes in this patch would have enabled that future with
PHOT, specifically the bitmap as the method for conveying what indexes need
updating.
That said, I agree that this patch should simply focus on expanding HOT to
expression indexes and partial indexes. With that in mind I will return to the
TU_UpdateIndexes approach, even though I'm not a fan of it, and later propose
the bitmap approach (or something better) as part of PHOT if and when that's
ready.
Changes v5 to v6:
* reverted to TU_UpdateIndexes rather than Bitmapset
* renamed ExecIndexesRequiringUpdates to ExecIndexesExpressionsWereNotUpdated
* ExecIndexesExpressionsWereNotUpdated returns bool, exits early if possible
* simple_update paths can be HOT once again
* removed efforts to determine the subset of updated summarizing indexes
* create filtered IndexInfo list in relcache containing only indexes with
expressions
* now using index TupleDesc CompactAttributes for arguments to datum_is_equal()
<- did I get this one right, I'm not sure it is what you had in mind
best.
-greg
> On Feb 15, 2025, at 5:49 AM, Matthias van de Meent
> <boekewurm+postg...@gmail.com> wrote:
>
> On Thu, 13 Feb 2025 at 19:46, Burd, Greg <gregb...@amazon.com> wrote:
>
> -----
>
> 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.
Fair point. I'm not a fan of the TU_UpdateIndexes enum, but I appreciate your
argument against the bitmapset. Under the conditions you describe (1000s of
indexes) it could grow unwieldy and impact performance.
> 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.
> 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.
> -----
>
> 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.
It did. (1) for sure, but the second is more related to the PHOT work (as
mentioned above). With that work almost all updates are on the HOT/PHOT path
and so the bitmap of changed indexes is small. It would take an update of a
table with 1000s of indexes where almost all those were modified to create a
bitmap that was large, which certainly could (and somewhere likely does) exist,
but it's not the common case (I'd imagine). In that case the work to update
all those indexes will likely dwarf the work to build that bitmap, but I could
be wrong.
But you're fundamentally right, I'm conflating two ideas and I shouldn't. I
will focus the changes required for this idea without pulling in changes useful
to a future ones.
> 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.
Okay, I'll combine the earlier v3 patch with the changes in v5. That should
leave TU_UpdateIndexes in place and allow for HOT with expression indexes.
I'll change the ExecIndexesRequiringUpdates() a bit (and rename it) so that it
is just a boolean test for any index that spoils the HOT path and can exit
early in that case potentially avoiding extra work.
> 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 agree, but I think that a single simple more focused patch will suffice.
> -----
>
> 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.
If I plan on just having ExecIndexesRequiringUpdates() return a bool rather
than a bitmap then sorting, or even just filtering the list of IndexInfo to
only include indexes with expressions, makes sense. That way the only indexes
in question in that function's loop will be those that may spoil the HOT path.
When that list is length 0, we can skip the tests entirely.
> 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.
That makes sense, and your sorting idea would inform that kind of work. I'll
keep that in mind if I reintroduce code that aims to only update changed
summarized indexes.
> (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.
Yes. I'll resurrect that.
> -----
>
> 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).
I hadn't thought of that, I think it's a valid idea and I'll update
accordingly. I think I understand what you are suggesting.
>
> -----
>
> 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 think I do that now, feel free to say otherwise. When the expression is
checked in ExecIndexesExpressionsWereNotUpdated() I set:
/* Shortcut index_unchanged_by_update(), we know the answer. */
indexInfo->ii_CheckedUnchanged = true;
indexInfo->ii_IndexUnchanged = !changed;
That prevents duplicate effort in index_unchanged_by_update().
> -----
>
> 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. The current
method avoids pulling a the lock on the index to build the list, but doing that
once in relcache isn't horrible. Maybe you were suggesting using that opaque
struct to pass around the list of IndexInfo? Let me know on this one if you
had a specific idea. The swap I've made in v6 really just moves the IndexInfo
list to a filtered list with a new name created in relcache.
v6-0001-Expand-HOT-update-path-to-include-expression-and-.patch
Description: v6-0001-Expand-HOT-update-path-to-include-expression-and-.patch
