po 12. 7. 2021 v 22:31 odesílatel Tomas Vondra <tomas.von...@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.von...@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.sima...@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
