Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

Hi,

On Sun, 19 Feb 2023 at 16:04, Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>
> Hi,
>
> On 2/19/23 02:03, Matthias van de Meent wrote:
> > On Thu, 16 Jun 2022 at 15:05, Tomas Vondra
> > <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
> >>
> >> I've pushed the revert. Let's try again for PG16.
> >
> > As we discussed in person at the developer meeting, here's a patch to
> > try again for PG16.
> >
> > It combines the committed patches with my fix, and adds some
> > additional comments and polish. I am confident the code is correct,
> > but not that it is clean (see the commit message of the patch for
> > details).
> >
>
> Thanks for the patch. I took a quick look, and I agree it seems correct,
> and fairly clean too.

Thanks. Based on feedback, attached is v2 of the patch, with as
significant changes:

- We don't store the columns we mention in predicates of summarized
indexes in the hotblocking column anymore, they are stored in the
summarized columns bitmap instead. This further reduces the chance of
failiing to apply HOT with summarizing indexes.
- The heaptuple header bit for summarized update in inserted tuples is
replaced with passing an out parameter. This simplifies the logic and
decreases chances of accidentally storing incorrect data.

Responses to feedback below.

> Which places you think need cleanup/improvement?

I wasn't confident about the use of HEAP_TUPLE_SUMMARIZING_UPDATED -
it's not a nice way to signal what indexes to update. This has been
updated in the attached patch.

> AFAICS some of the code comes from the original (reverted) patch, so
> that should be fairly non-controversial. The two new bits seem to be
> TU_UpdateIndexes and HEAP_TUPLE_SUMMARIZING_UPDATED.

Correct.

> I have some minor review comments regarding TU_UpdateIndexes, but in
> principle it's fine - we need to track/pass the flag somehow, and this
> is reasonable IMHO.
>
> I'm not entirely sure about HEAP_TUPLE_SUMMARIZING_UPDATED yet.

This is the part that I wasn't sure about either. I don't really like
the way it was implemented (temporary in-memory only bits in the tuple
header), but I also couldn't find an amazing alternative back in the
v15 beta window when I wrote the original fix for the now-reverted
commit. I've updated this to utilize 'out parameters' instead.
Although this change requires some more function signature changes, I
think it's better overall.

> It's
> pretty much a counter-part to TU_UpdateIndexes - until now we've only
> had HOT vs. non-HOT, and one bit in header (HEAP_HOT_UPDATED) was
> sufficient for that. But now we need 3 states, so an extra bit is
> needed. That's fine, and using another bit in the header makes sense.
>
> The commit message says the bit is "otherwise unused" but after a while
> I realized it's just an "alias" for HEAP_HOT_UPDATED - I guess it means
> it's unused in the places that need to track set it, right? I wonder if
> something can be confused by this - thinking it's a regular HOT update,
> and doing something wrong.

Yes. A newly inserted tuple, whether created from an update or a fresh
insert, can't already have been HOT-updated, so the bit is only
available (not in use for meaningful operations) in the in-memory
tuple processing path of new tuple insertion (be it update or actual
insert).

> Do we have some precedent for using a header bit like this? Something
> that'd set a bit on in-memory tuple only to reset it shortly after?

I can't find any, but I also haven't looked very far.

> Does it make sense to add asserts that'd ensure we can't set the bit
> twice? Like a code setting both HEAP_HOT_UPDATED and the new flag?

I'm doubtful of that; as this is basically a HOT chain intermediate
tuple being returned (but only in memory), instead of the normal
freshly inserted HOT tuple that's the end of a HOT chain. Anyway, that
code has been removed in the attached patch.

> A couple more minor comments after eye-balling the patch:
>
> * I think heap_update would benefit from a couple more comments, e.g.
> the comment before calculating sum_attrs should probably mention the
> summarization optimization.

Done.

> * heapam_tuple_update is probably the one place that I find hard to read
> not quite readable.

Updated.

> * I don't understand why the TU_UpdateIndexes fields are prefixed TUUI_?
> Why not to just use TU_?

I was under the (after checking, mistaken) impression that we already
had an enum that used the TU_* prefix. This has been updated.

> * indexam.sgml says:
>
> Access methods that do not point to individual tuples, but to (like
>
> I guess "page range" (or something like that) is missing.

Fixed

> Note: I wonder how difficult would it be to also deal with attributes in
> predicates. IIRC if the predicate is false, we can ignore the index, but
> the consensus back then was it's too expensive as it can't be done using
> the bitmaps and requires evaluating the expression, etc. But maybe there
> are ways to work around that by first checking everything except for the
> index predicates, and only when we still think HOT is possible we would
> check the predicates. Tables usually have only a couple partial indexes,
> so this might be a win. Not something this patch should/needs to do, of
> course.

Yes, I think that could be considered separately.

> * bikeshedding: rel.h says
>
> Bitmapset *rd_summarizedattr; /* cols indexed by block-or-larger
> summarizing indexes */
>
> I think the "block-or-larger" bit is unnecessary. I think the crucial
> bit is the index does not contain pointers to individual tuples.
> Similarly for indexam.sgml, which talks about "at least all tuples in
> one block".

That makes sense, fixed.

Kind regards,

Matthias van de Meent