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. Revert changes in HOT handling of BRIN indexes

  3. Ignore BRIN indexes when checking for HOT udpates

  1. Ignoring BRIN for HOT udpates seems broken

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2022-05-28T14:50:54Z

    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

  2. Re: Ignoring BRIN for HOT udpates seems broken

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2022-05-28T19:24:59Z

    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

  3. Re: Ignoring BRIN for HOT udpates seems broken

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2022-05-28T20:50:59Z

    
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

  4. Re: Ignoring BRIN for HOT udpates seems broken

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2022-05-30T15:22:35Z

    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

  5. Re: Ignoring BRIN for HOT udpates seems broken

    Andres Freund <andres@anarazel.de> — 2022-05-30T19:57:58Z

    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

  6. Re: Ignoring BRIN for HOT udpates seems broken

    Robert Haas <robertmhaas@gmail.com> — 2022-06-01T20:38:16Z

    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

  7. Re: Ignoring BRIN for HOT udpates seems broken

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2022-06-06T07:08:08Z

    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

  8. Re: Ignoring BRIN for HOT udpates seems broken

    Michael Paquier <michael@paquier.xyz> — 2022-06-06T07:28:52Z

    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

  9. Re: Ignoring BRIN for HOT udpates seems broken

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2022-06-16T13:05:06Z

    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

  10. Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2023-02-19T01:03:21Z

    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

  11. Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2023-02-19T15:04:16Z

    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

  12. Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2023-02-20T18:15:56Z

    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

  13. Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2023-02-22T12:15:07Z

    
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

  14. Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2023-02-22T13:14:02Z

    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.

  15. Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2023-03-08T22:31:58Z

    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

  16. Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2023-03-14T13:49:08Z

    
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

  17. Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2023-03-14T14:41:58Z

    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

  18. Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2023-03-20T10:11:27Z

    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

  19. Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2023-03-20T10:24:20Z

    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.

  20. Re: Ignoring BRIN for HOT updates (was: -udpates seems broken)

    Josef Šimánek <josef.simanek@gmail.com> — 2023-03-20T10:44:15Z

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