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

On 7/12/21 10:45 PM, Josef Šimánek wrote:
> po 12. 7. 2021 v 22:31 odesílatel Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)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(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
>
> 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