Thread: Ignoring BRIN for HOT udpates seems broken
Hi,
while working on some BRIN stuff, I realized (my) commit 5753d4ee320b
ignoring BRIN indexes for HOT is likely broken. Consider this example:
----------------------------------------------------------------------
CREATE TABLE t (a INT) WITH (fillfactor = 10);
INSERT INTO t SELECT i
FROM generate_series(0,100000) s(i);
CREATE INDEX ON t USING BRIN (a);
UPDATE t SET a = 0 WHERE random() < 0.01;
SET enable_seqscan = off;
EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0;
SET enable_seqscan = on;
EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0;
----------------------------------------------------------------------
which unfortunately produces this:
QUERY PLAN
---------------------------------------------------------------
Bitmap Heap Scan on t (actual rows=23 loops=1)
Recheck Cond: (a = 0)
Rows Removed by Index Recheck: 2793
Heap Blocks: lossy=128
-> Bitmap Index Scan on t_a_idx (actual rows=1280 loops=1)
Index Cond: (a = 0)
Planning Time: 0.049 ms
Execution Time: 0.424 ms
(8 rows)
SET
QUERY PLAN
-----------------------------------------
Seq Scan on t (actual rows=995 loops=1)
Filter: (a = 0)
Rows Removed by Filter: 99006
Planning Time: 0.027 ms
Execution Time: 7.670 ms
(5 rows)
That is, the index fails to return some of the rows :-(
I don't remember the exact reasoning behind the commit, but the commit
message justifies the change like this:
There are no index pointers to individual tuples in BRIN, and the
page range summary will be updated anyway as it relies on visibility
info.
AFAICS that's a misunderstanding of how BRIN uses visibility map, or
rather does not use. In particular, bringetbitmap() does not look at the
vm at all, so it'll produce incomplete bitmap.
So it seems I made a boo boo here. Luckily, this is a PG15 commit, not a
live issue. I don't quite see if this can be salvaged - I'll think about
this a bit more, but it'll probably end with a revert.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment
On Sat, 28 May 2022 at 16:51, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > > Hi, > > while working on some BRIN stuff, I realized (my) commit 5753d4ee320b > ignoring BRIN indexes for HOT is likely broken. Consider this example: > > ---------------------------------------------------------------------- > CREATE TABLE t (a INT) WITH (fillfactor = 10); > > INSERT INTO t SELECT i > FROM generate_series(0,100000) s(i); > > CREATE INDEX ON t USING BRIN (a); > > UPDATE t SET a = 0 WHERE random() < 0.01; > > SET enable_seqscan = off; > EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0; > > SET enable_seqscan = on; > EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0; > ---------------------------------------------------------------------- > > which unfortunately produces this: > > QUERY PLAN > --------------------------------------------------------------- > Bitmap Heap Scan on t (actual rows=23 loops=1) > Recheck Cond: (a = 0) > Rows Removed by Index Recheck: 2793 > Heap Blocks: lossy=128 > -> Bitmap Index Scan on t_a_idx (actual rows=1280 loops=1) > Index Cond: (a = 0) > Planning Time: 0.049 ms > Execution Time: 0.424 ms > (8 rows) > > SET > QUERY PLAN > ----------------------------------------- > Seq Scan on t (actual rows=995 loops=1) > Filter: (a = 0) > Rows Removed by Filter: 99006 > Planning Time: 0.027 ms > Execution Time: 7.670 ms > (5 rows) > > That is, the index fails to return some of the rows :-( > > I don't remember the exact reasoning behind the commit, but the commit > message justifies the change like this: > > There are no index pointers to individual tuples in BRIN, and the > page range summary will be updated anyway as it relies on visibility > info. > > AFAICS that's a misunderstanding of how BRIN uses visibility map, or > rather does not use. In particular, bringetbitmap() does not look at the > vm at all, so it'll produce incomplete bitmap. > > So it seems I made a boo boo here. Luckily, this is a PG15 commit, not a > live issue. I don't quite see if this can be salvaged - I'll think about > this a bit more, but it'll probably end with a revert. The principle of 'amhotblocking' for only blocking HOT updates seems correct, except for the fact that the HOT flag bit is also used as a way to block the propagation of new values to existing indexes. A better abstraction would be "amSummarizes[Block]', in which updates that only modify columns that are only included in summarizing indexes still allow HOT, but still will see an update call to all (relevant?) summarizing indexes. That should still improve performance significantly for the relevant cases. -Matthias
On 5/28/22 21:24, Matthias van de Meent wrote: > On Sat, 28 May 2022 at 16:51, Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: >> >> Hi, >> >> while working on some BRIN stuff, I realized (my) commit 5753d4ee320b >> ignoring BRIN indexes for HOT is likely broken. Consider this example: >> >> ---------------------------------------------------------------------- >> CREATE TABLE t (a INT) WITH (fillfactor = 10); >> >> INSERT INTO t SELECT i >> FROM generate_series(0,100000) s(i); >> >> CREATE INDEX ON t USING BRIN (a); >> >> UPDATE t SET a = 0 WHERE random() < 0.01; >> >> SET enable_seqscan = off; >> EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0; >> >> SET enable_seqscan = on; >> EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0; >> ---------------------------------------------------------------------- >> >> which unfortunately produces this: >> >> QUERY PLAN >> --------------------------------------------------------------- >> Bitmap Heap Scan on t (actual rows=23 loops=1) >> Recheck Cond: (a = 0) >> Rows Removed by Index Recheck: 2793 >> Heap Blocks: lossy=128 >> -> Bitmap Index Scan on t_a_idx (actual rows=1280 loops=1) >> Index Cond: (a = 0) >> Planning Time: 0.049 ms >> Execution Time: 0.424 ms >> (8 rows) >> >> SET >> QUERY PLAN >> ----------------------------------------- >> Seq Scan on t (actual rows=995 loops=1) >> Filter: (a = 0) >> Rows Removed by Filter: 99006 >> Planning Time: 0.027 ms >> Execution Time: 7.670 ms >> (5 rows) >> >> That is, the index fails to return some of the rows :-( >> >> I don't remember the exact reasoning behind the commit, but the commit >> message justifies the change like this: >> >> There are no index pointers to individual tuples in BRIN, and the >> page range summary will be updated anyway as it relies on visibility >> info. >> >> AFAICS that's a misunderstanding of how BRIN uses visibility map, or >> rather does not use. In particular, bringetbitmap() does not look at the >> vm at all, so it'll produce incomplete bitmap. >> >> So it seems I made a boo boo here. Luckily, this is a PG15 commit, not a >> live issue. I don't quite see if this can be salvaged - I'll think about >> this a bit more, but it'll probably end with a revert. > > The principle of 'amhotblocking' for only blocking HOT updates seems > correct, except for the fact that the HOT flag bit is also used as a > way to block the propagation of new values to existing indexes. > > A better abstraction would be "amSummarizes[Block]', in which updates > that only modify columns that are only included in summarizing indexes > still allow HOT, but still will see an update call to all (relevant?) > summarizing indexes. That should still improve performance > significantly for the relevant cases. > Yeah, I think that might/should work. We could still create the HOT chain, but we'd have to update the BRIN indexes. But that seems like a fairly complicated change to be done this late for PG15. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, 28 May 2022 at 22:51, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > > > > On 5/28/22 21:24, Matthias van de Meent wrote: > > On Sat, 28 May 2022 at 16:51, Tomas Vondra > > <tomas.vondra@enterprisedb.com> wrote: > >> > >> Hi, > >> > >> while working on some BRIN stuff, I realized (my) commit 5753d4ee320b > >> ignoring BRIN indexes for HOT is likely broken. Consider this example: > >> > >> ---------------------------------------------------------------------- > >> CREATE TABLE t (a INT) WITH (fillfactor = 10); > >> > >> INSERT INTO t SELECT i > >> FROM generate_series(0,100000) s(i); > >> > >> CREATE INDEX ON t USING BRIN (a); > >> > >> UPDATE t SET a = 0 WHERE random() < 0.01; > >> > >> SET enable_seqscan = off; > >> EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0; > >> > >> SET enable_seqscan = on; > >> EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF) SELECT * FROM t WHERE a = 0; > >> ---------------------------------------------------------------------- > >> > >> which unfortunately produces this: > >> > >> QUERY PLAN > >> --------------------------------------------------------------- > >> Bitmap Heap Scan on t (actual rows=23 loops=1) > >> Recheck Cond: (a = 0) > >> Rows Removed by Index Recheck: 2793 > >> Heap Blocks: lossy=128 > >> -> Bitmap Index Scan on t_a_idx (actual rows=1280 loops=1) > >> Index Cond: (a = 0) > >> Planning Time: 0.049 ms > >> Execution Time: 0.424 ms > >> (8 rows) > >> > >> SET > >> QUERY PLAN > >> ----------------------------------------- > >> Seq Scan on t (actual rows=995 loops=1) > >> Filter: (a = 0) > >> Rows Removed by Filter: 99006 > >> Planning Time: 0.027 ms > >> Execution Time: 7.670 ms > >> (5 rows) > >> > >> That is, the index fails to return some of the rows :-( > >> > >> I don't remember the exact reasoning behind the commit, but the commit > >> message justifies the change like this: > >> > >> There are no index pointers to individual tuples in BRIN, and the > >> page range summary will be updated anyway as it relies on visibility > >> info. > >> > >> AFAICS that's a misunderstanding of how BRIN uses visibility map, or > >> rather does not use. In particular, bringetbitmap() does not look at the > >> vm at all, so it'll produce incomplete bitmap. > >> > >> So it seems I made a boo boo here. Luckily, this is a PG15 commit, not a > >> live issue. I don't quite see if this can be salvaged - I'll think about > >> this a bit more, but it'll probably end with a revert. > > > > The principle of 'amhotblocking' for only blocking HOT updates seems > > correct, except for the fact that the HOT flag bit is also used as a > > way to block the propagation of new values to existing indexes. > > > > A better abstraction would be "amSummarizes[Block]', in which updates > > that only modify columns that are only included in summarizing indexes > > still allow HOT, but still will see an update call to all (relevant?) > > summarizing indexes. That should still improve performance > > significantly for the relevant cases. > > > > Yeah, I think that might/should work. We could still create the HOT > chain, but we'd have to update the BRIN indexes. But that seems like a > fairly complicated change to be done this late for PG15. Here's an example patch for that (based on a branch derived from master @ 5bb2b6ab). A nod to the authors of the pHOT patch, as that is a related patch and was informative in how this could/should impact AM APIs -- this is doing things similar (but not exactly the same) to that by only updating select indexes. Note that this is an ABI change in some critical places -- I'm not sure it's OK to commit a fix like this into PG15 unless we really don't want to revert 5753d4ee320b. Also of note is that this still updates _all_ summarizing indexes, not only those involved in the tuple update. Better performance is up to a different implementation. The patch includes a new regression test based on your example, which fails on master but succeeds after applying the patch. -Matthias
Attachment
Hi, On 2022-05-30 17:22:35 +0200, Matthias van de Meent wrote: > > Yeah, I think that might/should work. We could still create the HOT > > chain, but we'd have to update the BRIN indexes. But that seems like a > > fairly complicated change to be done this late for PG15. > > Here's an example patch for that (based on a branch derived from > master @ 5bb2b6ab). A nod to the authors of the pHOT patch, as that is > a related patch and was informative in how this could/should impact AM > APIs -- this is doing things similar (but not exactly the same) to > that by only updating select indexes. > > Note that this is an ABI change in some critical places -- I'm not > sure it's OK to commit a fix like this into PG15 unless we really > don't want to revert 5753d4ee320b. > > Also of note is that this still updates _all_ summarizing indexes, not > only those involved in the tuple update. Better performance is up to a > different implementation. > > The patch includes a new regression test based on your example, which > fails on master but succeeds after applying the patch. This seems like a pretty clear cut case for reverting and retrying in 16. There's plenty subtlety in this area (as evidenced by this thread and the index/reindex concurrently breakage), and building infrastructure post beta1 isn't exactly conducive to careful analysis and testing. Greetings, Andres Freund
On Sat, May 28, 2022 at 4:51 PM Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > Yeah, I think that might/should work. We could still create the HOT > chain, but we'd have to update the BRIN indexes. But that seems like a > fairly complicated change to be done this late for PG15. Yeah, I think a revert is better for now. But I agree that the basic idea seems salvageable. I think that the commit message is correct when it states that "When determining whether an index update may be skipped by using HOT, we can ignore attributes indexed only by BRIN indexes." However, that doesn't mean that we can ignore the need to update those indexes. In that regard, the commit message makes it sound like all is well, because it states that "the page range summary will be updated anyway" which reads to me like the indexes are in fact getting updated. Your example, however, seems to show that the indexes are not getting updated. -- Robert Haas EDB: http://www.enterprisedb.com
On 6/1/22 22:38, Robert Haas wrote: > On Sat, May 28, 2022 at 4:51 PM Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: >> Yeah, I think that might/should work. We could still create the HOT >> chain, but we'd have to update the BRIN indexes. But that seems like a >> fairly complicated change to be done this late for PG15. > > Yeah, I think a revert is better for now. But I agree that the basic > idea seems salvageable. I think that the commit message is correct > when it states that "When determining whether an index update may be > skipped by using HOT, we can ignore attributes indexed only by BRIN > indexes." However, that doesn't mean that we can ignore the need to > update those indexes. In that regard, the commit message makes it > sound like all is well, because it states that "the page range summary > will be updated anyway" which reads to me like the indexes are in fact > getting updated. Your example, however, seems to show that the indexes > are not getting updated. > Yeah, agreed :-( I agree we can probably salvage some of the idea, but it's far too late for major reworks in PG15. Attached is a patch reverting both commits (5753d4ee32 and fe60b67250). This changes the IndexAmRoutine struct, so it's an ABI break. That's not great post-beta :-( In principle we might also leave amhotblocking in the struct but ignore it in the code (and treat it as false), but that seems weird and it's going to be a pain when backpatching. Opinions? regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Mon, Jun 06, 2022 at 09:08:08AM +0200, Tomas Vondra wrote: > Attached is a patch reverting both commits (5753d4ee32 and fe60b67250). > This changes the IndexAmRoutine struct, so it's an ABI break. That's not > great post-beta :-( In principle we might also leave amhotblocking in > the struct but ignore it in the code (and treat it as false), but that > seems weird and it's going to be a pain when backpatching. Opinions? I don't think that you need to worry about ABI breakages now in beta, because that's the period of time where we can still change things and shape the code in its best way for prime time. It depends on the change, of course, but what you are doing, by removing the field, looks right to me here. -- Michael
Attachment
On 6/6/22 09:28, Michael Paquier wrote: > On Mon, Jun 06, 2022 at 09:08:08AM +0200, Tomas Vondra wrote: >> Attached is a patch reverting both commits (5753d4ee32 and fe60b67250). >> This changes the IndexAmRoutine struct, so it's an ABI break. That's not >> great post-beta :-( In principle we might also leave amhotblocking in >> the struct but ignore it in the code (and treat it as false), but that >> seems weird and it's going to be a pain when backpatching. Opinions? > > I don't think that you need to worry about ABI breakages now in beta, > because that's the period of time where we can still change things and > shape the code in its best way for prime time. It depends on the > change, of course, but what you are doing, by removing the field, > looks right to me here. I've pushed the revert. Let's try again for PG16. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, 16 Jun 2022 at 15:05, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > > I've pushed the revert. Let's try again for PG16. As we discussed in person at the developer meeting, here's a patch to try again for PG16. It combines the committed patches with my fix, and adds some additional comments and polish. I am confident the code is correct, but not that it is clean (see the commit message of the patch for details). Kind regards, Matthias van de Meent PS. I'm adding this to the commitfest Original patch thread: https://www.postgresql.org/message-id/flat/CAFp7QwpMRGcDAQumN7onN9HjrJ3u4X3ZRXdGFT0K5G2JWvnbWg%40mail.gmail.com Other relevant: https://www.postgresql.org/message-id/flat/CA%2BTgmoZOgdoAFH9HatRwuydOZkMdyPi%3D97rNhsu%3DhQBBYs%2BgXQ%40mail.gmail.com
Attachment
Hi, On 2/19/23 02:03, Matthias van de Meent wrote: > On Thu, 16 Jun 2022 at 15:05, Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: >> >> I've pushed the revert. Let's try again for PG16. > > As we discussed in person at the developer meeting, here's a patch to > try again for PG16. > > It combines the committed patches with my fix, and adds some > additional comments and polish. I am confident the code is correct, > but not that it is clean (see the commit message of the patch for > details). > Thanks for the patch. I took a quick look, and I agree it seems correct, and fairly clean too. Which places you think need cleanup/improvement? AFAICS some of the code comes from the original (reverted) patch, so that should be fairly non-controversial. The two new bits seem to be TU_UpdateIndexes and HEAP_TUPLE_SUMMARIZING_UPDATED. I have some minor review comments regarding TU_UpdateIndexes, but in principle it's fine - we need to track/pass the flag somehow, and this is reasonable IMHO. I'm not entirely sure about HEAP_TUPLE_SUMMARIZING_UPDATED yet. It's pretty much a counter-part to TU_UpdateIndexes - until now we've only had HOT vs. non-HOT, and one bit in header (HEAP_HOT_UPDATED) was sufficient for that. But now we need 3 states, so an extra bit is needed. That's fine, and using another bit in the header makes sense. The commit message says the bit is "otherwise unused" but after a while I realized it's just an "alias" for HEAP_HOT_UPDATED - I guess it means it's unused in the places that need to track set it, right? I wonder if something can be confused by this - thinking it's a regular HOT update, and doing something wrong. Do we have some precedent for using a header bit like this? Something that'd set a bit on in-memory tuple only to reset it shortly after? Does it make sense to add asserts that'd ensure we can't set the bit twice? Like a code setting both HEAP_HOT_UPDATED and the new flag? A couple more minor comments after eye-balling the patch: * I think heap_update would benefit from a couple more comments, e.g. the comment before calculating sum_attrs should probably mention the summarization optimization. * heapam_tuple_update is probably the one place that I find hard to read not quite readable. * I don't understand why the TU_UpdateIndexes fields are prefixed TUUI_? Why not to just use TU_? * indexam.sgml says: Access methods that do not point to individual tuples, but to (like I guess "page range" (or something like that) is missing. Note: I wonder how difficult would it be to also deal with attributes in predicates. IIRC if the predicate is false, we can ignore the index, but the consensus back then was it's too expensive as it can't be done using the bitmaps and requires evaluating the expression, etc. But maybe there are ways to work around that by first checking everything except for the index predicates, and only when we still think HOT is possible we would check the predicates. Tables usually have only a couple partial indexes, so this might be a win. Not something this patch should/needs to do, of course. * bikeshedding: rel.h says Bitmapset *rd_summarizedattr; /* cols indexed by block-or-larger summarizing indexes */ I think the "block-or-larger" bit is unnecessary. I think the crucial bit is the index does not contain pointers to individual tuples. Similarly for indexam.sgml, which talks about "at least all tuples in one block". regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On Sun, 19 Feb 2023 at 16:04, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > > Hi, > > On 2/19/23 02:03, Matthias van de Meent wrote: > > On Thu, 16 Jun 2022 at 15:05, Tomas Vondra > > <tomas.vondra@enterprisedb.com> wrote: > >> > >> I've pushed the revert. Let's try again for PG16. > > > > As we discussed in person at the developer meeting, here's a patch to > > try again for PG16. > > > > It combines the committed patches with my fix, and adds some > > additional comments and polish. I am confident the code is correct, > > but not that it is clean (see the commit message of the patch for > > details). > > > > Thanks for the patch. I took a quick look, and I agree it seems correct, > and fairly clean too. Thanks. Based on feedback, attached is v2 of the patch, with as significant changes: - We don't store the columns we mention in predicates of summarized indexes in the hotblocking column anymore, they are stored in the summarized columns bitmap instead. This further reduces the chance of failiing to apply HOT with summarizing indexes. - The heaptuple header bit for summarized update in inserted tuples is replaced with passing an out parameter. This simplifies the logic and decreases chances of accidentally storing incorrect data. Responses to feedback below. > Which places you think need cleanup/improvement? I wasn't confident about the use of HEAP_TUPLE_SUMMARIZING_UPDATED - it's not a nice way to signal what indexes to update. This has been updated in the attached patch. > AFAICS some of the code comes from the original (reverted) patch, so > that should be fairly non-controversial. The two new bits seem to be > TU_UpdateIndexes and HEAP_TUPLE_SUMMARIZING_UPDATED. Correct. > I have some minor review comments regarding TU_UpdateIndexes, but in > principle it's fine - we need to track/pass the flag somehow, and this > is reasonable IMHO. > > I'm not entirely sure about HEAP_TUPLE_SUMMARIZING_UPDATED yet. This is the part that I wasn't sure about either. I don't really like the way it was implemented (temporary in-memory only bits in the tuple header), but I also couldn't find an amazing alternative back in the v15 beta window when I wrote the original fix for the now-reverted commit. I've updated this to utilize 'out parameters' instead. Although this change requires some more function signature changes, I think it's better overall. > It's > pretty much a counter-part to TU_UpdateIndexes - until now we've only > had HOT vs. non-HOT, and one bit in header (HEAP_HOT_UPDATED) was > sufficient for that. But now we need 3 states, so an extra bit is > needed. That's fine, and using another bit in the header makes sense. > > The commit message says the bit is "otherwise unused" but after a while > I realized it's just an "alias" for HEAP_HOT_UPDATED - I guess it means > it's unused in the places that need to track set it, right? I wonder if > something can be confused by this - thinking it's a regular HOT update, > and doing something wrong. Yes. A newly inserted tuple, whether created from an update or a fresh insert, can't already have been HOT-updated, so the bit is only available (not in use for meaningful operations) in the in-memory tuple processing path of new tuple insertion (be it update or actual insert). > Do we have some precedent for using a header bit like this? Something > that'd set a bit on in-memory tuple only to reset it shortly after? I can't find any, but I also haven't looked very far. > Does it make sense to add asserts that'd ensure we can't set the bit > twice? Like a code setting both HEAP_HOT_UPDATED and the new flag? I'm doubtful of that; as this is basically a HOT chain intermediate tuple being returned (but only in memory), instead of the normal freshly inserted HOT tuple that's the end of a HOT chain. Anyway, that code has been removed in the attached patch. > A couple more minor comments after eye-balling the patch: > > * I think heap_update would benefit from a couple more comments, e.g. > the comment before calculating sum_attrs should probably mention the > summarization optimization. Done. > * heapam_tuple_update is probably the one place that I find hard to read > not quite readable. Updated. > * I don't understand why the TU_UpdateIndexes fields are prefixed TUUI_? > Why not to just use TU_? I was under the (after checking, mistaken) impression that we already had an enum that used the TU_* prefix. This has been updated. > * indexam.sgml says: > > Access methods that do not point to individual tuples, but to (like > > I guess "page range" (or something like that) is missing. Fixed > Note: I wonder how difficult would it be to also deal with attributes in > predicates. IIRC if the predicate is false, we can ignore the index, but > the consensus back then was it's too expensive as it can't be done using > the bitmaps and requires evaluating the expression, etc. But maybe there > are ways to work around that by first checking everything except for the > index predicates, and only when we still think HOT is possible we would > check the predicates. Tables usually have only a couple partial indexes, > so this might be a win. Not something this patch should/needs to do, of > course. Yes, I think that could be considered separately. > * bikeshedding: rel.h says > > Bitmapset *rd_summarizedattr; /* cols indexed by block-or-larger > summarizing indexes */ > > I think the "block-or-larger" bit is unnecessary. I think the crucial > bit is the index does not contain pointers to individual tuples. > Similarly for indexam.sgml, which talks about "at least all tuples in > one block". That makes sense, fixed. Kind regards, Matthias van de Meent
Attachment
On 2/20/23 19:15, Matthias van de Meent wrote: > Hi, > > On Sun, 19 Feb 2023 at 16:04, Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: >> >> Hi, >> >> On 2/19/23 02:03, Matthias van de Meent wrote: >>> On Thu, 16 Jun 2022 at 15:05, Tomas Vondra >>> <tomas.vondra@enterprisedb.com> wrote: >>>> >>>> I've pushed the revert. Let's try again for PG16. >>> >>> As we discussed in person at the developer meeting, here's a patch to >>> try again for PG16. >>> >>> It combines the committed patches with my fix, and adds some >>> additional comments and polish. I am confident the code is correct, >>> but not that it is clean (see the commit message of the patch for >>> details). >>> >> >> Thanks for the patch. I took a quick look, and I agree it seems correct, >> and fairly clean too. > > Thanks. Based on feedback, attached is v2 of the patch, with as > significant changes: > > - We don't store the columns we mention in predicates of summarized > indexes in the hotblocking column anymore, they are stored in the > summarized columns bitmap instead. This further reduces the chance of > failiing to apply HOT with summarizing indexes. Interesting idea. I need to think about the correctness, but AFAICS it should work. Do we have any tests covering such cases? I see both v1 and v2 had exactly this src/test/regress/expected/stats.out | 110 ++++++++++++++++++ src/test/regress/sql/stats.sql | 82 ++++++++++++- so I guess there are no new tests testing this for BRIN with predicates. We should probably add some ... > - The heaptuple header bit for summarized update in inserted tuples is > replaced with passing an out parameter. This simplifies the logic and > decreases chances of accidentally storing incorrect data. > OK. 0002 proposes a minor RelationGetIndexPredicate() tweak, getting rid of the repeated if/else branches. Feel free to discard, if you think the v2 approach is better. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Wed, 22 Feb 2023 at 13:15, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > > On 2/20/23 19:15, Matthias van de Meent wrote: > > Thanks. Based on feedback, attached is v2 of the patch, with as > > significant changes: > > > > - We don't store the columns we mention in predicates of summarized > > indexes in the hotblocking column anymore, they are stored in the > > summarized columns bitmap instead. This further reduces the chance of > > failiing to apply HOT with summarizing indexes. > > Interesting idea. I need to think about the correctness, but AFAICS it > should work. Do we have any tests covering such cases? There is a test that checks that an update to the predicated column does update the index (on table brin_hot_2). However, the description was out of date, so I've updated that in v4. > > - The heaptuple header bit for summarized update in inserted tuples is > > replaced with passing an out parameter. This simplifies the logic and > > decreases chances of accidentally storing incorrect data. > > > > OK. > > 0002 proposes a minor RelationGetIndexPredicate() tweak, getting rid of > the repeated if/else branches. Feel free to discard, if you think the v2 > approach is better. I agree that this is better, it's included in v4 of the patch, as attached. Kind regards, Matthias van de Meent.
Attachment
On Wed, 22 Feb 2023 at 14:14, Matthias van de Meent <boekewurm+postgres@gmail.com> wrote: > > On Wed, 22 Feb 2023 at 13:15, Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: > > > > On 2/20/23 19:15, Matthias van de Meent wrote: > > > Thanks. Based on feedback, attached is v2 of the patch, with as > > > significant changes: > > > > > > - We don't store the columns we mention in predicates of summarized > > > indexes in the hotblocking column anymore, they are stored in the > > > summarized columns bitmap instead. This further reduces the chance of > > > failiing to apply HOT with summarizing indexes. > > > > Interesting idea. I need to think about the correctness, but AFAICS it > > should work. Do we have any tests covering such cases? > > There is a test that checks that an update to the predicated column > does update the index (on table brin_hot_2). However, the description > was out of date, so I've updated that in v4. > > > > - The heaptuple header bit for summarized update in inserted tuples is > > > replaced with passing an out parameter. This simplifies the logic and > > > decreases chances of accidentally storing incorrect data. > > > > > > > OK. > > > > 0002 proposes a minor RelationGetIndexPredicate() tweak, getting rid of > > the repeated if/else branches. Feel free to discard, if you think the v2 > > approach is better. > > I agree that this is better, it's included in v4 of the patch, as attached. I think that the v4 patch solves all comments up to now; and considering that most of this patch was committed but then reverted due to an issue in v15, and that said issue is fixed in this patch, I'm marking this as ready for committer. Tomas, would you be up for that? Kind regards, Matthias van de Meent
On 3/8/23 23:31, Matthias van de Meent wrote:
> On Wed, 22 Feb 2023 at 14:14, Matthias van de Meent
> <boekewurm+postgres@gmail.com> wrote:
>>
>> On Wed, 22 Feb 2023 at 13:15, Tomas Vondra
>> <tomas.vondra@enterprisedb.com> wrote:
>>>
>>> On 2/20/23 19:15, Matthias van de Meent wrote:
>>>> Thanks. Based on feedback, attached is v2 of the patch, with as
>>>> significant changes:
>>>>
>>>> - We don't store the columns we mention in predicates of summarized
>>>> indexes in the hotblocking column anymore, they are stored in the
>>>> summarized columns bitmap instead. This further reduces the chance of
>>>> failiing to apply HOT with summarizing indexes.
>>>
>>> Interesting idea. I need to think about the correctness, but AFAICS it
>>> should work. Do we have any tests covering such cases?
>>
>> There is a test that checks that an update to the predicated column
>> does update the index (on table brin_hot_2). However, the description
>> was out of date, so I've updated that in v4.
>>
>>>> - The heaptuple header bit for summarized update in inserted tuples is
>>>> replaced with passing an out parameter. This simplifies the logic and
>>>> decreases chances of accidentally storing incorrect data.
>>>>
>>>
>>> OK.
>>>
>>> 0002 proposes a minor RelationGetIndexPredicate() tweak, getting rid of
>>> the repeated if/else branches. Feel free to discard, if you think the v2
>>> approach is better.
>>
>> I agree that this is better, it's included in v4 of the patch, as attached.
>
> I think that the v4 patch solves all comments up to now; and
> considering that most of this patch was committed but then reverted
> due to an issue in v15, and that said issue is fixed in this patch,
> I'm marking this as ready for committer.
>
> Tomas, would you be up for that?
>
Thanks for the patch. I started looking at it yesterday, and I think
it's 99% RFC. I think it's correct and I only have some minor comments,
(see the 0002 patch):
1) There were still a couple minor wording issues in the sgml docs.
2) bikeshedding: I added a bunch of "()" to various conditions, I think
it makes it clearer.
3) This seems a bit weird way to write a conditional Assert:
if (onlySummarized)
Assert(HeapTupleIsHeapOnly(heapTuple));
better to do a composed Assert(!(onlySummarized && !...)) or something?
4) A couple comments and minor tweaks.
5) Undoing a couple unnecessary changes (whitespace, ...).
6) Proper formatting of TU_UpdateIndexes enum.
7) Comment in RelationGetIndexAttrBitmap() is misleading, as it still
references hotblockingattrs, even though it may update summarizedattrs
in some cases.
If you agree with these changes, I'll get it committed.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment
On Tue, 14 Mar 2023 at 14:49, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > > > > On 3/8/23 23:31, Matthias van de Meent wrote: > > On Wed, 22 Feb 2023 at 14:14, Matthias van de Meent > > > > I think that the v4 patch solves all comments up to now; and > > considering that most of this patch was committed but then reverted > > due to an issue in v15, and that said issue is fixed in this patch, > > I'm marking this as ready for committer. > > > > Tomas, would you be up for that? > > > > Thanks for the patch. I started looking at it yesterday, and I think > it's 99% RFC. I think it's correct and I only have some minor comments, > (see the 0002 patch): > > > 1) There were still a couple minor wording issues in the sgml docs. > > 2) bikeshedding: I added a bunch of "()" to various conditions, I think > it makes it clearer. Sure > 3) This seems a bit weird way to write a conditional Assert: > > if (onlySummarized) > Assert(HeapTupleIsHeapOnly(heapTuple)); > > better to do a composed Assert(!(onlySummarized && !...)) or something? I don't like this double negation, as it adds significant parsing complexity to the statement. If I'd have gone with a single Assert() statement, I'd have used the following: Assert((!onlySummarized) || HeapTupleIsHeapOnly(heapTuple)); because in the code section above that the HOT + !onlySummarized case is an early exit. > 4) A couple comments and minor tweaks. > 5) Undoing a couple unnecessary changes (whitespace, ...). > 6) Proper formatting of TU_UpdateIndexes enum. Allright > + * > + * XXX Why do we assign explicit values to the members, instead of just letting > + * it up to the enum (just like for TM_Result)? This was from the v15 beta window, to reduce the difference between bool and TU_UpdateIndexes. With pg16, that can be dropped. > > 7) Comment in RelationGetIndexAttrBitmap() is misleading, as it still > references hotblockingattrs, even though it may update summarizedattrs > in some cases. How about Since we have covering indexes with non-key columns, we must handle them accurately here. Non-key columns must be added into the hotblocking or summarizing attrs bitmap, since they are in the index, and update shouldn't miss them. instead for that section? > If you agree with these changes, I'll get it committed. Yes, thanks! Kind regards, Matthias van de Meent
On 3/14/23 15:41, Matthias van de Meent wrote: > On Tue, 14 Mar 2023 at 14:49, Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: >> >>> ... > >> If you agree with these changes, I'll get it committed. > > Yes, thanks! > I've tweaked the patch per the last round of comments, cleaned up the commit message a bit (it still talked about unused bit in tuple header and so on), and pushed it. Thanks for fixing the issues that got the patch reverted last year! regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, 20 Mar 2023 at 11:11, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > > On 3/14/23 15:41, Matthias van de Meent wrote: > > On Tue, 14 Mar 2023 at 14:49, Tomas Vondra > > <tomas.vondra@enterprisedb.com> wrote: > >> > >>> ... > > > >> If you agree with these changes, I'll get it committed. > > > > Yes, thanks! > > > > I've tweaked the patch per the last round of comments, cleaned up the > commit message a bit (it still talked about unused bit in tuple header > and so on), and pushed it. > > Thanks for fixing the issues that got the patch reverted last year! Thanks for helping getting this in! Kind regards, Matthias van de Meent.
po 20. 3. 2023 v 11:24 odesílatel Matthias van de Meent <boekewurm+postgres@gmail.com> napsal: > > On Mon, 20 Mar 2023 at 11:11, Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: > > > > On 3/14/23 15:41, Matthias van de Meent wrote: > > > On Tue, 14 Mar 2023 at 14:49, Tomas Vondra > > > <tomas.vondra@enterprisedb.com> wrote: > > >> > > >>> ... > > > > > >> If you agree with these changes, I'll get it committed. > > > > > > Yes, thanks! > > > > > > > I've tweaked the patch per the last round of comments, cleaned up the > > commit message a bit (it still talked about unused bit in tuple header > > and so on), and pushed it. > > > > Thanks for fixing the issues that got the patch reverted last year! > > Thanks for helping getting this in! Thanks for fixing the problems! > > Kind regards, > > Matthias van de Meent. > >