Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Ignore BRIN indexes when checking for HOT updates

  2. Move test for BRIN HOT behavior to stats.sql

  3. Ignore BRIN indexes when checking for HOT udpates

  1. [PATCH] Don't block HOT update by BRIN index

    Josef Šimánek <josef.simanek@gmail.com> — 2021-06-29T22:31:25Z

    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].

It can be viewed online (latest version) on GitHub [2] as well.

- small overview

1. I have added "amhotblocking" flag to index AM descriptor set to
"true" for all, except BRIN, index types. And later in heap_update
method (heapam.c) I do filter attributes based on this new flag,
instead of currently checking for any existing index.

2. I had to enhance the "RelationGetIndexAttrBitmap" function to be
able to return a bitmap of index attribute numbers related to the new
AM flag using "INDEX_ATTR_BITMAP_HOT_BLOCKING" filter.
PS: Originally the "INDEX_ATTR_BITMAP_ALL" filter was used for HOT
check update and most likely could be removed (including all logic
related in RelationGetIndexAttrBitmap), since I have not found any
other usage.

3. I have created an initial regression test using
"pg_stat_get_tuples_hot_updated" to find out HOT was successful on the
BRIN indexed column. Unfortunately "pg_stat_get_tuples_hot_updated" is
not updated immediately and I have not found any way to enforce the
update. Thus (at least for now) I have used a similar approach to
stats.sql using the "wait_for_stats" function (waiting for 30 seconds
and checking each 100ms for change).

I'm attaching patch to CF 2021-07.

[1] https://groups.google.com/g/postgresql-cz/c/oxA_v3H17Qg
[2] https://github.com/simi/postgres/pull/7

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

    Josef Šimánek <josef.simanek@gmail.com> — 2021-06-29T22:53:02Z

    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.

> It can be viewed online (latest version) on GitHub [2] as well.
>
> - small overview
>
> 1. I have added "amhotblocking" flag to index AM descriptor set to
> "true" for all, except BRIN, index types. And later in heap_update
> method (heapam.c) I do filter attributes based on this new flag,
> instead of currently checking for any existing index.
>
> 2. I had to enhance the "RelationGetIndexAttrBitmap" function to be
> able to return a bitmap of index attribute numbers related to the new
> AM flag using "INDEX_ATTR_BITMAP_HOT_BLOCKING" filter.
> PS: Originally the "INDEX_ATTR_BITMAP_ALL" filter was used for HOT
> check update and most likely could be removed (including all logic
> related in RelationGetIndexAttrBitmap), since I have not found any
> other usage.
>
> 3. I have created an initial regression test using
> "pg_stat_get_tuples_hot_updated" to find out HOT was successful on the
> BRIN indexed column. Unfortunately "pg_stat_get_tuples_hot_updated" is
> not updated immediately and I have not found any way to enforce the
> update. Thus (at least for now) I have used a similar approach to
> stats.sql using the "wait_for_stats" function (waiting for 30 seconds
> and checking each 100ms for change).
>
> I'm attaching patch to CF 2021-07.
>
> [1] https://groups.google.com/g/postgresql-cz/c/oxA_v3H17Qg
> [2] https://github.com/simi/postgres/pull/7

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-06-29T23:20:01Z

    
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.

regards

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

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

    Josef Šimánek <josef.simanek@gmail.com> — 2021-06-29T23:43:15Z

    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.

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

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-07-12T20:31:16Z

    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


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

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-07-12T20:37:40Z

    On 2021-Jul-12, Tomas Vondra wrote:

> 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?

... What?  I thought the whole point is that BRIN indexes do not cause
the columns to become part of this set, while all other index types do.
If you make them both the same, then there's no point.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-07-12T20:45:24Z

    
On 7/12/21 10:37 PM, Alvaro Herrera wrote:
> On 2021-Jul-12, Tomas Vondra wrote:
> 
>> 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?
> 
> ... What?  I thought the whole point is that BRIN indexes do not cause
> the columns to become part of this set, while all other index types do.
> If you make them both the same, then there's no point.
> 

Well, one of us is confused and it might be me ;-)

The point is that BRIN is the only index type with amhotblocking=false,
so it would return NULL (and thus it does not block HOT). All other
indexes AMs have amblocking=true and so should return rd_indexattr (I
forgot to change that in the code chunk).

regards

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

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

    Josef Šimánek <josef.simanek@gmail.com> — 2021-07-12T20:45:30Z

    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

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-07-12T20:54:54Z

    
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

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-07-12T20:55:31Z

    On 2021-Jul-12, Tomas Vondra wrote:

> Well, one of us is confused and it might be me ;-)

:-)

> The point is that BRIN is the only index type with amhotblocking=false,
> so it would return NULL (and thus it does not block HOT). All other
> indexes AMs have amblocking=true and so should return rd_indexattr (I
> forgot to change that in the code chunk).

But RelationGetIndexAttrBitmap is called for the table that contains the
index (and probably contains some other indexes too), not for one
specific index.  So the bitmap is about the columns involved in *all*
indexes of the table ...

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"El destino baraja y nosotros jugamos" (A. Schopenhauer)

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-07-12T21:00:05Z

    On 7/12/21 10:55 PM, Alvaro Herrera wrote:
> On 2021-Jul-12, Tomas Vondra wrote:
> 
>> Well, one of us is confused and it might be me ;-)
> 
> :-)
> 
>> The point is that BRIN is the only index type with amhotblocking=false,
>> so it would return NULL (and thus it does not block HOT). All other
>> indexes AMs have amblocking=true and so should return rd_indexattr (I
>> forgot to change that in the code chunk).
> 
> But RelationGetIndexAttrBitmap is called for the table that contains the
> index (and probably contains some other indexes too), not for one
> specific index.  So the bitmap is about the columns involved in *all*
> indexes of the table ...
> 

D'oh! Well, I did say I might be confused ...

Yeah, that optimization is not possible, unfortunately.


regards

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

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-07-12T21:02:09Z

    On 2021-Jul-12, Josef Šimánek wrote:

> > 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.

Oh, that's interesting.  What this means is that INDEX_ATTR_BITMAP_ALL
is no longer used; its uses must have all been replaced by something
else.  It seems the only one that currently exists is for HOT in
heap_update, which this patch replaces with the new one.  In a quick
search, no external code depends on it, so I'd be inclined to just
remove it ...

I think a boolean is much simpler.  Consider a table with 1600 columns :-)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-07-12T21:15:04Z

    
On 7/12/21 11:02 PM, Alvaro Herrera wrote:
> On 2021-Jul-12, Josef Šimánek wrote:
> 
>>> 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.
> 
> Oh, that's interesting.  What this means is that INDEX_ATTR_BITMAP_ALL
> is no longer used; its uses must have all been replaced by something
> else.  It seems the only one that currently exists is for HOT in
> heap_update, which this patch replaces with the new one.  In a quick
> search, no external code depends on it, so I'd be inclined to just
> remove it ...
> 
> I think a boolean is much simpler.  Consider a table with 1600 columns :-)
> 

I'm not sure how to verify no external code depends on that flag. I have
no idea if there's a plausible use case for it, though.

Even with 1600 columns the amount of wasted memory is only about 200B,
which is not that bad I think. Not great, not terrible.

OTOH most tables won't have any BRIN indexes, in which case indexattr
and hotblockingattr are guaranteed to be exactly the same. So maybe
that's something we could leverage - we need to calculate the "hot"
bitmap, and in most cases we can use it for indexattr too.

Maybe let's leave that for a separate patch, though?


regards

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

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-07-12T23:00:37Z

    On 2021-Jul-12, Tomas Vondra wrote:

> I'm not sure how to verify no external code depends on that flag. I have
> no idea if there's a plausible use case for it, though.

But we don't *have* to, do we?

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Tiene valor aquel que admite que es un cobarde" (Fernandel)

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

    Josef Šimánek <josef.simanek@gmail.com> — 2021-07-12T23:02:19Z

    po 12. 7. 2021 v 23:15 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:
>
>
>
> On 7/12/21 11:02 PM, Alvaro Herrera wrote:
> > On 2021-Jul-12, Josef Šimánek wrote:
> >
> >>> 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.
> >
> > Oh, that's interesting.  What this means is that INDEX_ATTR_BITMAP_ALL
> > is no longer used; its uses must have all been replaced by something
> > else.  It seems the only one that currently exists is for HOT in
> > heap_update, which this patch replaces with the new one.  In a quick
> > search, no external code depends on it, so I'd be inclined to just
> > remove it ...
> >
> > I think a boolean is much simpler.  Consider a table with 1600 columns :-)
> >
>
> I'm not sure how to verify no external code depends on that flag. I have
> no idea if there's a plausible use case for it, though.

I tried GitHub search before to ensure at least it is not a widely
used "API". There were no results outside of PostgreSQL code itself in
first 10 pages of results.


> Even with 1600 columns the amount of wasted memory is only about 200B,
> which is not that bad I think. Not great, not terrible.
>
> OTOH most tables won't have any BRIN indexes, in which case indexattr
> and hotblockingattr are guaranteed to be exactly the same. So maybe
> that's something we could leverage - we need to calculate the "hot"
> bitmap, and in most cases we can use it for indexattr too.
>
> Maybe let's leave that for a separate patch, though?
>
>
> regards
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-10-04T14:17:47Z

    Hi,

I took a look at this patch again to see if I can get it polished and 
fixed. Per the discussion, I've removed the rd_indexattr list and 
replaced it with a simple flag. While doing so, I noticed a couple of 
places that should have consider (init or free) rd_hotblockingattr.

Patch 0001 is the v2, 0002 removes the rd_indexattr etc.

regards

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

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

    Josef Šimánek <josef.simanek@gmail.com> — 2021-10-10T21:36:08Z

    po 4. 10. 2021 v 16:17 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:
>
> Hi,
>
> I took a look at this patch again to see if I can get it polished and
> fixed. Per the discussion, I've removed the rd_indexattr list and
> replaced it with a simple flag. While doing so, I noticed a couple of
> places that should have consider (init or free) rd_hotblockingattr.

Thanks for finishing this. I can confirm both patches do apply without
problems. I did some simple testing locally and everything worked as
intended.

> Patch 0001 is the v2, 0002 removes the rd_indexattr etc.
>
> regards
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-11-06T02:16:02Z

    Hi,

I've polished the patch a bit, with the goal to get it committed. I've 
added the new amhotblocking flag to indexam.sgml (and I'm wondering if 
there's another place in docs for more details).

But then I realized there's an issue in handling the predicate. Consider 
this example:

   drop table if exists t;
   create table t (a int, b int);
   insert into t values (1, 100);
   create index on t using brin (b) where a = 2;

   update t set a = 2;

   explain analyze select * from t where a = 2 and b = 100;
   set enable_seqscan = off;
   explain analyze select * from t where a = 2 and b = 100;

With the previous version of the patch, the explains are this:

                                 QUERY PLAN
   ----------------------------------------------------------------------
    Seq Scan on t  (cost=0.00..1.01 rows=1 width=8)
                   (actual time=0.006..0.007 rows=1 loops=1)
      Filter: ((a = 2) AND (b = 100))
    Planning Time: 0.040 ms
    Execution Time: 0.018 ms
   (4 rows)

                                 QUERY PLAN
   ----------------------------------------------------------------------
    Bitmap Heap Scan on t  (cost=12.03..16.05 rows=1 width=8)
                                (actual time=0.007..0.009 rows=0 loops=1)
      Recheck Cond: ((b = 100) AND (a = 2))
      ->  Bitmap Index Scan on t_b_idx  (cost=0.00..12.03 rows=1 width=0)
                                (actual time=0.006..0.006 rows=0 loops=1)
            Index Cond: (b = 100)
    Planning Time: 0.041 ms
    Execution Time: 0.026 ms
   (6 rows)

Notice that the second plan (using the brin index) produces 0 rows, 
which is obviously wrong. Clearly, the index was not updated.

I think this is caused by simple thinko in RelationGetIndexAttrBitmap, 
which did this:

     /* Collect all attributes in the index predicate, too */
     if (indexDesc->rd_indam->amhotblocking)
         pull_varattnos(indexPredicate, 1, &hotblockingattrs);

I think this is wrong - we should not ignore the predicate based on 
amhotblocking, because then we'll fail to notice an update making the 
tuple match the index predicate (as in the example).

The way I understand heap_update() it does not try to determine if the 
update makes the tuple indexable, it just disables HOT when it might 
happen. The attached patch just calls pull_varattnos every time.

I wonder if we might be a bit smarter about the predicates vs. HOT, and 
disable HOT only when the tuple becomes indexable after the update.


regards

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

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-11-30T19:11:03Z

    OK,

I've polished the last version of the patch a bit (added a regression 
test with update of attribute in index predicate and docs about the new 
flag into indexam.sgml) and pushed.

I wonder if we could/should improve handling of index predicates. In 
particular, it seems to me we could simply ignore indexes when the new 
row does not match the index predicate. For example, if there's an index

   CREATE INDEX ON t (a) WHERE b = 1;

and the update does:

   UPDATE t SET b = 2 WHERE ...;

then we'll not add the tuple pointer to this index anyway, and we could 
simply ignore this index when considering HOT. But I might be missing 
something important about HOT ...

The main problem I see with this is it requires evaluating the index 
predicate for each tuple, which makes it incompatible with the caching 
in RelationGetIndexAttrBitmap. Just ditching the caching seems like a 
bad idea, so we'd probably have to do this in two phases:

1) Do what we do now, i.e. RelationGetIndexAttrBitmap considering all 
indexes / attributes. If this says HOT is possible, great - we're done.

2) If (1) says HOT is not possible, we need to look whether it's because 
of regular or partial index. For regular indexes it's clear, for partial 
indexes we could ignore this if the predicate evaluates to false for the 
new row.

But even if such optimization is possible, it's way out of scope of this 
patch and it's not clear to me it's actually a sensible trade-off.


regards

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

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

    Josef Šimánek <josef.simanek@gmail.com> — 2021-12-01T10:31:15Z

    út 30. 11. 2021 v 20:11 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:
>
> OK,
>
> I've polished the last version of the patch a bit (added a regression
> test with update of attribute in index predicate and docs about the new
> flag into indexam.sgml) and pushed.

Thanks a lot for taking over this, improving and pushing!

> I wonder if we could/should improve handling of index predicates. In
> particular, it seems to me we could simply ignore indexes when the new
> row does not match the index predicate. For example, if there's an index
>
>    CREATE INDEX ON t (a) WHERE b = 1;
>
> and the update does:
>
>    UPDATE t SET b = 2 WHERE ...;
>
> then we'll not add the tuple pointer to this index anyway, and we could
> simply ignore this index when considering HOT. But I might be missing
> something important about HOT ...
>
> The main problem I see with this is it requires evaluating the index
> predicate for each tuple, which makes it incompatible with the caching
> in RelationGetIndexAttrBitmap. Just ditching the caching seems like a
> bad idea, so we'd probably have to do this in two phases:
>
> 1) Do what we do now, i.e. RelationGetIndexAttrBitmap considering all
> indexes / attributes. If this says HOT is possible, great - we're done.
>
> 2) If (1) says HOT is not possible, we need to look whether it's because
> of regular or partial index. For regular indexes it's clear, for partial
> indexes we could ignore this if the predicate evaluates to false for the
> new row.
>
> But even if such optimization is possible, it's way out of scope of this
> patch and it's not clear to me it's actually a sensible trade-off.
>
>
> regards
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

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

    Justin Pryzby <pryzby@telsasoft.com> — 2021-12-05T16:47:31Z

    On Tue, Nov 30, 2021 at 08:11:03PM +0100, Tomas Vondra wrote:
> OK,
> 
> I've polished the last version of the patch a bit (added a regression test
> with update of attribute in index predicate and docs about the new flag into
> indexam.sgml) and pushed.

brin.sql's new brin_hot test is failing sometimes.

I saw a local failure and then found this.
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus&dt=2021-12-01%2003%3A00%3A07

 SELECT pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid);
  pg_stat_get_tuples_hot_updated 
 --------------------------------
-                              1
+                              0
 (1 row)

Evidently because:
| 2021-12-01 04:02:01.096 CET [61a6e587.3106b1:4] LOG:  wait_for_hot_stats delayed 33.217301 seconds

It seems like maybe the UDP packet lost to the stats collector got lost ?
It fails less than 10% of the time here, probably depending on load.

BTW there's a typo in brin.sql: precicates

-- 
Justin

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-05T20:16:02Z

    Justin Pryzby <pryzby@telsasoft.com> writes:
> brin.sql's new brin_hot test is failing sometimes.
> Evidently because:
> | 2021-12-01 04:02:01.096 CET [61a6e587.3106b1:4] LOG:  wait_for_hot_stats delayed 33.217301 seconds
> It seems like maybe the UDP packet lost to the stats collector got lost ?
> It fails less than 10% of the time here, probably depending on load.

Oh, geez.  *Please* let us not add another regression failure mode
like the ones that afflict stats.sql.  We do not need a doubling
of that failure rate.  I suggest just removing this test.

			regards, tom lane

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-12-06T00:51:47Z

    On 12/5/21 21:16, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
>> brin.sql's new brin_hot test is failing sometimes.
>> Evidently because:
>> | 2021-12-01 04:02:01.096 CET [61a6e587.3106b1:4] LOG:  wait_for_hot_stats delayed 33.217301 seconds
>> It seems like maybe the UDP packet lost to the stats collector got lost ?
>> It fails less than 10% of the time here, probably depending on load.
> 
> Oh, geez.  *Please* let us not add another regression failure mode
> like the ones that afflict stats.sql.  We do not need a doubling
> of that failure rate.  I suggest just removing this test.
> 

Whooops. Agreed, I'll get rid of that test.


regards

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-06T01:47:30Z

    Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> On 12/5/21 21:16, Tom Lane wrote:
>> Oh, geez.  *Please* let us not add another regression failure mode
>> like the ones that afflict stats.sql.  We do not need a doubling
>> of that failure rate.  I suggest just removing this test.

> Whooops. Agreed, I'll get rid of that test.

Another idea, perhaps, is to shove that test into stats.sql,
where people would know to ignore it?  (Actually, I've thought
more than once that we should mark stats.sql as ignorable
in the schedule ...)

			regards, tom lane

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

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-12-11T04:44:22Z

    On 12/6/21 02:47, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
>> On 12/5/21 21:16, Tom Lane wrote:
>>> Oh, geez.  *Please* let us not add another regression failure mode
>>> like the ones that afflict stats.sql.  We do not need a doubling
>>> of that failure rate.  I suggest just removing this test.
> 
>> Whooops. Agreed, I'll get rid of that test.
> 
> Another idea, perhaps, is to shove that test into stats.sql,
> where people would know to ignore it?  (Actually, I've thought
> more than once that we should mark stats.sql as ignorable
> in the schedule ...)
> 

Yep. I've moved the test to stats.sql - that seems better than just 
ditching it, because we're experimenting with maybe relaxing the HOT 
rules for BRIN a bit further and not having tests for that would be 
unfortunate.

I haven't marked the test as ignorable. I wonder if we should make that 
customizable, so that some animals (like serinus, which fails because of 
stats.sql from time to time) could run ignore it. But if it fails 
elsewhere it would still be considered a proper failure.

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