Re: Ignoring BRIN for HOT udpates seems broken

On Sat, 28 May 2022 at 22:51, Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>
>
>
> On 5/28/22 21:24, Matthias van de Meent wrote:
> > On Sat, 28 May 2022 at 16:51, Tomas Vondra
> > <tomas(dot)vondra(at)enterprisedb(dot)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