Re: [PATCH] Don't block HOT update by BRIN index
От | Tomas Vondra |
---|---|
Тема | Re: [PATCH] Don't block HOT update by BRIN index |
Дата | |
Msg-id | bddcce8f-ce90-840b-8462-c261efbc132e@enterprisedb.com обсуждение исходный текст |
Ответ на | Re: [PATCH] Don't block HOT update by BRIN index (Josef Šimánek <josef.simanek@gmail.com>) |
Список | pgsql-hackers |
On 7/12/21 10:45 PM, Josef Šimánek wrote: > 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. > TBH I'm not sure either. >> >> 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. Well, RelationGetIndexAttrBitmap is accessible from extensions, so it might be used by code passing INDEX_ATTR_BITMAP_ALL. Not sure if there's such code, of course. My point is that for amhotblocking=true the bitmaps seem to be exactly the same, so we can calculate it just once (so replacing it with a bool flag would not save us anything). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
В списке pgsql-hackers по дате отправления: