Re: [PATCH] Don't block HOT update by BRIN index

On 6/30/21 1:43 AM, Josef Šimánek wrote:
> st 30. 6. 2021 v 1:20 odesílatel Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)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(dot)simanek(at)gmail(dot)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

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?

3) The patch should update indexam.sgml with description of the new
field, amhotblocking or how it'll end up named.

regards

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