Re: [PATCH] Don't block HOT update by BRIN index - Mailing list pgsql-hackers

po 12. 7. 2021 v 22:31 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:
>
> On 6/30/21 1:43 AM, Josef Šimánek wrote:
> > st 30. 6. 2021 v 1:20 odesílatel Tomas Vondra
> > <tomas.vondra@enterprisedb.com> napsal:
> >>
> >>
> >>
> >> On 6/30/21 12:53 AM, Josef Šimánek wrote:
> >>> st 30. 6. 2021 v 0:31 odesílatel Josef Šimánek <josef.simanek@gmail.com> napsal:
> >>>>
> >>>> Hello!
> >>>>
> >>>> Tomáš Vondra has shared a few ideas to improve BRIN index in czech
> >>>> PostgreSQL mail list some time ago [1 , in czech only]. This is first
> >>>> try to implement one of those ideas.
> >>>>
> >>>> Currently BRIN index blocks HOT update even it is not linked tuples
> >>>> directly. I'm attaching the initial patch allowing HOT update even on
> >>>> BRIN indexed columns. This patch went through an initial review on
> >>>> czech PostgreSQL mail list [1].
> >>>
> >>> I just found out current patch is breaking partial-index isolation
> >>> test. I'm looking into this problem.
> >>>
> >>
> >> The problem is in RelationGetIndexAttrBitmap - the existing code first
> >> walks indnatts, and builds the indexattrs / hotblockingattrs. But then
> >> it also inspects expressions and the predicate (by pull_varattnos), and
> >> the patch fails to do that for hotblockingattrs. Which is why it fails
> >> for partial-index, because that uses an index with a predicate.
> >>
> >> So there needs to be something like:
> >>
> >>      if (indexDesc->rd_indam->amhotblocking)
> >>          pull_varattnos(indexExpressions, 1, &hotblockingattrs);
> >>
> >>      if (indexDesc->rd_indam->amhotblocking)
> >>          pull_varattnos(indexPredicate, 1, &hotblockingattrs);
> >>
> >> This fixes the failure for me.
> >
> > Thanks for the hint. I'm attaching a fixed standalone patch.
> >
>
> Thanks, this version seems to be working fine and passes check-world. So
> I did another round of review, and all I have are some simple comments:
>
>
> 1) naming stuff (this is very subjective, feel free to disagree)
>
> I wonder if we should rename 'amhotblocking' to 'amblockshot' which
> seems more natural to me?
>
> Similarly, maybe rename rd_hotblockingattr to rd_hotattr

OK, I wasn't sure about the naming.

>
> 2) Do we actually need to calculate and store hotblockingattrs
> separately in RelationGetIndexAttrBitmap? It seems to me it's either
> NULL (with amhotblocking=false) or equal to indexattrs. So why not to
> just get rid of hotblockingattr and rd_hotblockingattr, and do something
> like
>
>   case INDEX_ATTR_BITMAP_HOT_BLOCKING:
>     return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL;
>
> I haven't tried, so maybe I'm missing something?

relation->rd_indexattr is currently not used (at least I have not
found anything) for anything, except looking if other values are
already loaded.

/* Quick exit if we already computed the result. */
if (relation->rd_indexattr != NULL)

I think it could be replaced with boolean to make it clear other
values (rd_keyattr, rd_pkattr, rd_idattr, rd_hotblockingattr) are
already loaded.
>
> 3) The patch should update indexam.sgml with description of the new
> field, amhotblocking or how it'll end up named.

I'll do.

>
> regards
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company