Thread: [PATCH] Don't block HOT update by BRIN index
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
Attachment
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
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
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
Attachment
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
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)
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
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
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
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)
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
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"
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
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)
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
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
Attachment
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
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
Attachment
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
ú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
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
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
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
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