Обсуждение: Re: Performance degradation of REFRESH MATERIALIZED VIEW

.

On Thu, Mar 11, 2021 at 5:44 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Hi,
>
> While discussing freezing tuples during CTAS[1], we found that
> heap_insert() with HEAP_INSERT_FROZEN brings performance degradation.
> For instance, with Paul's patch that sets HEAP_INSERT_FROZEN to CTAS,
> it took 12 sec whereas the code without the patch took 10 sec with the
> following query:
>
> create table t1 (a, b, c, d) as select i,i,i,i from
> generate_series(1,20000000) i;
>
> I've done a simple benchmark of REFRESH MATERIALIZED VIEW with the
> following queries:
>
> create table source as select generate_series(1, 50000000);
> create materialized view mv as select * from source;
> refresh materialized view mv;
>
> The execution time of REFRESH MATERIALIZED VIEW are:
>
> w/ HEAP_INSERT_FROZEN flag : 42 sec
> w/o HEAP_INSERT_FROZEN flag : 33 sec
>
> After investigation, I found that such performance degradation happens
> on only HEAD code. It seems to me that commit 39b66a91b (and
> 7db0cd2145) is relevant that has heap_insert() set VM bits and
> PD_ALL_VISIBLE if HEAP_INSERT_FROZEN is specified (so CCing Tomas
> Vondra and authors). Since heap_insert() sets PD_ALL_VISIBLE to the
> page when inserting a tuple for the first time on the page (around
> L2133 in heapam.c), every subsequent heap_insert() on the page reads
> and pins a VM buffer (see RelationGetBufferForTuple()).

IIUC RelationGetBufferForTuple() pins vm buffer if the page is
all-visible since the caller might clear vm bit during operation. But
it's not necessarily true in HEAP_FROZEN_INSERT case. When inserting
HEAP_FROZEN_INSERT, we might set PD_ALL_VISIBLE flag and all-visible
bit but never clear those flag and bit during insertion. Therefore to
fix this issue, I think we can have RelationGetBufferForTuple() not to
pin vm buffer if we're inserting a frozen tuple (i.g.,
HEAP_FROZEN_INSERT case) and the target page is already all-visible.
In HEAP_FROZEN_INSERT, the cases where we need to pin vm buffer would
be the table is empty. That way, we will pin vm buffer only for the
first time of inserting frozen tuple into the empty page, then set
PD_ALL_VISIBLE to the page and all-frozen bit on vm. Also set
XLH_INSERT_ALL_FROZEN_SET to WAL. At further insertions, we would not
pin vm buffer as long as we’re inserting a frozen tuple into the same
page.

If the target page is neither empty nor all-visible we will not pin vm
buffer, which is fine because if the page has non-frozen tuple we
cannot set bit on vm during heap_insert(). If all tuples on the page
are already frozen but PD_ALL_VISIBLE is not set for some reason, we
would be able to set all-frozen bit on vm but it seems not a good idea
since it requires checking during insertion if all existing tuples are
frozen or not.

The attached patch does the above idea. With this patch, the same
performance tests took 33 sec.

Also, I've measured the number of page read during REFRESH
MATERIALIZED VIEW using by pg_stat_statements. There were big
different on shared_blks_hit on pg_stat_statements:

1. w/ HEAP_INSERT_FROZEN flag (HEAD) : 50221781
2. w/ HEAP_INSERT_FROZEN flag (HEAD) : 221782
3. Patched: 443014

Since the 'source' table has 50000000 and each heap_insert() read vm
buffer, test 1 read pages as many as the number of insertion tuples.
The value of test 3 is about twice as much as the one of test 2. This
is because heap_insert() read the vm buffer for each first insertion
to the page. The table has 221239 blocks.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

At Mon, 12 Apr 2021 15:20:41 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in 
> .
> 
> On Thu, Mar 11, 2021 at 5:44 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Hi,
> >
> > While discussing freezing tuples during CTAS[1], we found that
> > heap_insert() with HEAP_INSERT_FROZEN brings performance degradation.
> > For instance, with Paul's patch that sets HEAP_INSERT_FROZEN to CTAS,
> > it took 12 sec whereas the code without the patch took 10 sec with the
> > following query:
> >
> > create table t1 (a, b, c, d) as select i,i,i,i from
> > generate_series(1,20000000) i;
> >
> > I've done a simple benchmark of REFRESH MATERIALIZED VIEW with the
> > following queries:
> >
> > create table source as select generate_series(1, 50000000);
> > create materialized view mv as select * from source;
> > refresh materialized view mv;
> >
> > The execution time of REFRESH MATERIALIZED VIEW are:
> >
> > w/ HEAP_INSERT_FROZEN flag : 42 sec
> > w/o HEAP_INSERT_FROZEN flag : 33 sec
> >
> > After investigation, I found that such performance degradation happens
> > on only HEAD code. It seems to me that commit 39b66a91b (and
> > 7db0cd2145) is relevant that has heap_insert() set VM bits and
> > PD_ALL_VISIBLE if HEAP_INSERT_FROZEN is specified (so CCing Tomas
> > Vondra and authors). Since heap_insert() sets PD_ALL_VISIBLE to the
> > page when inserting a tuple for the first time on the page (around
> > L2133 in heapam.c), every subsequent heap_insert() on the page reads
> > and pins a VM buffer (see RelationGetBufferForTuple()).
> 
> IIUC RelationGetBufferForTuple() pins vm buffer if the page is
> all-visible since the caller might clear vm bit during operation. But
> it's not necessarily true in HEAP_FROZEN_INSERT case. When inserting
> HEAP_FROZEN_INSERT, we might set PD_ALL_VISIBLE flag and all-visible
> bit but never clear those flag and bit during insertion. Therefore to
> fix this issue, I think we can have RelationGetBufferForTuple() not to
> pin vm buffer if we're inserting a frozen tuple (i.g.,
> HEAP_FROZEN_INSERT case) and the target page is already all-visible.

It seems right to me.

> In HEAP_FROZEN_INSERT, the cases where we need to pin vm buffer would
> be the table is empty. That way, we will pin vm buffer only for the
> first time of inserting frozen tuple into the empty page, then set
> PD_ALL_VISIBLE to the page and all-frozen bit on vm. Also set
> XLH_INSERT_ALL_FROZEN_SET to WAL. At further insertions, we would not
> pin vm buffer as long as we’re inserting a frozen tuple into the same
> page.
> 
> If the target page is neither empty nor all-visible we will not pin vm
> buffer, which is fine because if the page has non-frozen tuple we
> cannot set bit on vm during heap_insert(). If all tuples on the page
> are already frozen but PD_ALL_VISIBLE is not set for some reason, we
> would be able to set all-frozen bit on vm but it seems not a good idea
> since it requires checking during insertion if all existing tuples are
> frozen or not.
> 
> The attached patch does the above idea. With this patch, the same
> performance tests took 33 sec.

Great! The direction of the patch looks fine to me.

+         * If we're inserting frozen entry into empty page, we will set
+         * all-visible to page and all-frozen on visibility map.
+         */
+        if (PageGetMaxOffsetNumber(page) == 0)
             all_frozen_set = true;

AFAICS the page is always empty when RelationGetBufferForTuple
returned a valid vmbuffer.  So the "if" should be an "assert" instead.

And, the patch changes the value of all_frozen_set to false when the
page was already all-frozen (thus not empty). It would be fine since
we don't need to change the visibility of the page in that case but
the variable name is no longer correct.  set_all_visible or such?

> Also, I've measured the number of page read during REFRESH
> MATERIALIZED VIEW using by pg_stat_statements. There were big
> different on shared_blks_hit on pg_stat_statements:
> 
> 1. w/ HEAP_INSERT_FROZEN flag (HEAD) : 50221781
> 2. w/ HEAP_INSERT_FROZEN flag (HEAD) : 221782
> 3. Patched: 443014
> 
> Since the 'source' table has 50000000 and each heap_insert() read vm
> buffer, test 1 read pages as many as the number of insertion tuples.
> The value of test 3 is about twice as much as the one of test 2. This
> is because heap_insert() read the vm buffer for each first insertion
> to the page. The table has 221239 blocks.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Apr 16, 2021 at 12:16 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Mon, 12 Apr 2021 15:20:41 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
> > .
> >
> > On Thu, Mar 11, 2021 at 5:44 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > While discussing freezing tuples during CTAS[1], we found that
> > > heap_insert() with HEAP_INSERT_FROZEN brings performance degradation.
> > > For instance, with Paul's patch that sets HEAP_INSERT_FROZEN to CTAS,
> > > it took 12 sec whereas the code without the patch took 10 sec with the
> > > following query:
> > >
> > > create table t1 (a, b, c, d) as select i,i,i,i from
> > > generate_series(1,20000000) i;
> > >
> > > I've done a simple benchmark of REFRESH MATERIALIZED VIEW with the
> > > following queries:
> > >
> > > create table source as select generate_series(1, 50000000);
> > > create materialized view mv as select * from source;
> > > refresh materialized view mv;
> > >
> > > The execution time of REFRESH MATERIALIZED VIEW are:
> > >
> > > w/ HEAP_INSERT_FROZEN flag : 42 sec
> > > w/o HEAP_INSERT_FROZEN flag : 33 sec
> > >
> > > After investigation, I found that such performance degradation happens
> > > on only HEAD code. It seems to me that commit 39b66a91b (and
> > > 7db0cd2145) is relevant that has heap_insert() set VM bits and
> > > PD_ALL_VISIBLE if HEAP_INSERT_FROZEN is specified (so CCing Tomas
> > > Vondra and authors). Since heap_insert() sets PD_ALL_VISIBLE to the
> > > page when inserting a tuple for the first time on the page (around
> > > L2133 in heapam.c), every subsequent heap_insert() on the page reads
> > > and pins a VM buffer (see RelationGetBufferForTuple()).
> >
> > IIUC RelationGetBufferForTuple() pins vm buffer if the page is
> > all-visible since the caller might clear vm bit during operation. But
> > it's not necessarily true in HEAP_FROZEN_INSERT case. When inserting
> > HEAP_FROZEN_INSERT, we might set PD_ALL_VISIBLE flag and all-visible
> > bit but never clear those flag and bit during insertion. Therefore to
> > fix this issue, I think we can have RelationGetBufferForTuple() not to
> > pin vm buffer if we're inserting a frozen tuple (i.g.,
> > HEAP_FROZEN_INSERT case) and the target page is already all-visible.
>
> It seems right to me.
>
> > In HEAP_FROZEN_INSERT, the cases where we need to pin vm buffer would
> > be the table is empty. That way, we will pin vm buffer only for the
> > first time of inserting frozen tuple into the empty page, then set
> > PD_ALL_VISIBLE to the page and all-frozen bit on vm. Also set
> > XLH_INSERT_ALL_FROZEN_SET to WAL. At further insertions, we would not
> > pin vm buffer as long as we’re inserting a frozen tuple into the same
> > page.
> >
> > If the target page is neither empty nor all-visible we will not pin vm
> > buffer, which is fine because if the page has non-frozen tuple we
> > cannot set bit on vm during heap_insert(). If all tuples on the page
> > are already frozen but PD_ALL_VISIBLE is not set for some reason, we
> > would be able to set all-frozen bit on vm but it seems not a good idea
> > since it requires checking during insertion if all existing tuples are
> > frozen or not.
> >
> > The attached patch does the above idea. With this patch, the same
> > performance tests took 33 sec.

Thank you for the comments.

>
> Great! The direction of the patch looks fine to me.
>
> +                * If we're inserting frozen entry into empty page, we will set
> +                * all-visible to page and all-frozen on visibility map.
> +                */
> +               if (PageGetMaxOffsetNumber(page) == 0)
>                         all_frozen_set = true;
>
> AFAICS the page is always empty when RelationGetBufferForTuple
> returned a valid vmbuffer.  So the "if" should be an "assert" instead.

There is a chance that RelationGetBufferForTuple() returns a valid
vmbuffer but the page is not empty, since RelationGetBufferForTuple()
checks without a lock if the page is empty. But when it comes to
HEAP_INSERT_FROZEN cases it actually doesn’t happen at least for now
since only one process inserts tuples into the relation. Will fix.

>
> And, the patch changes the value of all_frozen_set to false when the
> page was already all-frozen (thus not empty). It would be fine since
> we don't need to change the visibility of the page in that case but
> the variable name is no longer correct.  set_all_visible or such?

It seems to me that the variable name all_frozen_set corresponds to
XLH_INSERT_ALL_FROZEN_SET but I see your point. How about
set_all_frozen instead since we set all-frozen bits (also implying
setting all-visible)?

BTW I found the following description of XLH_INSERT_ALL_FROZEN_SET but
there is no all_visible_set anywhere:

/* all_frozen_set always implies all_visible_set */
#define XLH_INSERT_ALL_FROZEN_SET               (1<<5)

I'll update those comments as well.


Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

At Mon, 19 Apr 2021 13:32:31 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in 
> On Fri, Apr 16, 2021 at 12:16 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > AFAICS the page is always empty when RelationGetBufferForTuple
> > returned a valid vmbuffer.  So the "if" should be an "assert" instead.
> 
> There is a chance that RelationGetBufferForTuple() returns a valid
> vmbuffer but the page is not empty, since RelationGetBufferForTuple()
> checks without a lock if the page is empty. But when it comes to
> HEAP_INSERT_FROZEN cases it actually doesn’t happen at least for now
> since only one process inserts tuples into the relation. Will fix.

Yes.  It seems to me that it is cleaner that RelationGetBufferForTuple
returns vmbuffer only when the caller needs to change vm state.
Thanks.

> > And, the patch changes the value of all_frozen_set to false when the
> > page was already all-frozen (thus not empty). It would be fine since
> > we don't need to change the visibility of the page in that case but
> > the variable name is no longer correct.  set_all_visible or such?
> 
> It seems to me that the variable name all_frozen_set corresponds to
> XLH_INSERT_ALL_FROZEN_SET but I see your point. How about
> set_all_frozen instead since we set all-frozen bits (also implying
> setting all-visible)?

Right. However, "if (set_all_frozen) then "set all_visible" looks like
a bug^^;.  all_frozen_set looks better in that context than
set_all_frozen. So I withdraw the comment.

> BTW I found the following description of XLH_INSERT_ALL_FROZEN_SET but
> there is no all_visible_set anywhere:
> 
> /* all_frozen_set always implies all_visible_set */
> #define XLH_INSERT_ALL_FROZEN_SET               (1<<5)
> 
> I'll update those comments as well.

FWIW, it seems like a shorthand of "ALL_FROZEN_SET implies ALL_VISIBLE
to be set together". The current comment is working to me.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Apr 19, 2021 at 5:04 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Mon, 19 Apr 2021 13:32:31 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
> > On Fri, Apr 16, 2021 at 12:16 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > > AFAICS the page is always empty when RelationGetBufferForTuple
> > > returned a valid vmbuffer.  So the "if" should be an "assert" instead.
> >
> > There is a chance that RelationGetBufferForTuple() returns a valid
> > vmbuffer but the page is not empty, since RelationGetBufferForTuple()
> > checks without a lock if the page is empty. But when it comes to
> > HEAP_INSERT_FROZEN cases it actually doesn’t happen at least for now
> > since only one process inserts tuples into the relation. Will fix.
>
> Yes.  It seems to me that it is cleaner that RelationGetBufferForTuple
> returns vmbuffer only when the caller needs to change vm state.
> Thanks.
>
> > > And, the patch changes the value of all_frozen_set to false when the
> > > page was already all-frozen (thus not empty). It would be fine since
> > > we don't need to change the visibility of the page in that case but
> > > the variable name is no longer correct.  set_all_visible or such?
> >
> > It seems to me that the variable name all_frozen_set corresponds to
> > XLH_INSERT_ALL_FROZEN_SET but I see your point. How about
> > set_all_frozen instead since we set all-frozen bits (also implying
> > setting all-visible)?
>
> Right. However, "if (set_all_frozen) then "set all_visible" looks like
> a bug^^;.  all_frozen_set looks better in that context than
> set_all_frozen. So I withdraw the comment.
>
> > BTW I found the following description of XLH_INSERT_ALL_FROZEN_SET but
> > there is no all_visible_set anywhere:
> >
> > /* all_frozen_set always implies all_visible_set */
> > #define XLH_INSERT_ALL_FROZEN_SET               (1<<5)
> >
> > I'll update those comments as well.
>
> FWIW, it seems like a shorthand of "ALL_FROZEN_SET implies ALL_VISIBLE
> to be set together". The current comment is working to me.
>

Okay, I've updated the patch accordingly. Please review it.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Mon, Apr 19, 2021 at 1:57 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Apr 19, 2021 at 5:04 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > At Mon, 19 Apr 2021 13:32:31 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
> > > On Fri, Apr 16, 2021 at 12:16 PM Kyotaro Horiguchi
> > > <horikyota.ntt@gmail.com> wrote:
> > > > AFAICS the page is always empty when RelationGetBufferForTuple
> > > > returned a valid vmbuffer.  So the "if" should be an "assert" instead.
> > >
> > > There is a chance that RelationGetBufferForTuple() returns a valid
> > > vmbuffer but the page is not empty, since RelationGetBufferForTuple()
> > > checks without a lock if the page is empty. But when it comes to
> > > HEAP_INSERT_FROZEN cases it actually doesn’t happen at least for now
> > > since only one process inserts tuples into the relation. Will fix.
> >
> > Yes.  It seems to me that it is cleaner that RelationGetBufferForTuple
> > returns vmbuffer only when the caller needs to change vm state.
> > Thanks.
> >
> > > > And, the patch changes the value of all_frozen_set to false when the
> > > > page was already all-frozen (thus not empty). It would be fine since
> > > > we don't need to change the visibility of the page in that case but
> > > > the variable name is no longer correct.  set_all_visible or such?
> > >
> > > It seems to me that the variable name all_frozen_set corresponds to
> > > XLH_INSERT_ALL_FROZEN_SET but I see your point. How about
> > > set_all_frozen instead since we set all-frozen bits (also implying
> > > setting all-visible)?
> >
> > Right. However, "if (set_all_frozen) then "set all_visible" looks like
> > a bug^^;.  all_frozen_set looks better in that context than
> > set_all_frozen. So I withdraw the comment.
> >
> > > BTW I found the following description of XLH_INSERT_ALL_FROZEN_SET but
> > > there is no all_visible_set anywhere:
> > >
> > > /* all_frozen_set always implies all_visible_set */
> > > #define XLH_INSERT_ALL_FROZEN_SET               (1<<5)
> > >
> > > I'll update those comments as well.
> >
> > FWIW, it seems like a shorthand of "ALL_FROZEN_SET implies ALL_VISIBLE
> > to be set together". The current comment is working to me.
> >
>
> Okay, I've updated the patch accordingly. Please review it.

I was reading the patch, just found some typos: it should be "a frozen
tuple" not "an frozen tuple".

+     * Also pin visibility map page if we're inserting an frozen tuple into
+                 * If we're inserting an frozen entry into empty page, pin the

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 19, 2021 at 8:04 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Apr 19, 2021 at 1:57 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Apr 19, 2021 at 5:04 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > >
> > > At Mon, 19 Apr 2021 13:32:31 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
> > > > On Fri, Apr 16, 2021 at 12:16 PM Kyotaro Horiguchi
> > > > <horikyota.ntt@gmail.com> wrote:
> > > > > AFAICS the page is always empty when RelationGetBufferForTuple
> > > > > returned a valid vmbuffer.  So the "if" should be an "assert" instead.
> > > >
> > > > There is a chance that RelationGetBufferForTuple() returns a valid
> > > > vmbuffer but the page is not empty, since RelationGetBufferForTuple()
> > > > checks without a lock if the page is empty. But when it comes to
> > > > HEAP_INSERT_FROZEN cases it actually doesn’t happen at least for now
> > > > since only one process inserts tuples into the relation. Will fix.
> > >
> > > Yes.  It seems to me that it is cleaner that RelationGetBufferForTuple
> > > returns vmbuffer only when the caller needs to change vm state.
> > > Thanks.
> > >
> > > > > And, the patch changes the value of all_frozen_set to false when the
> > > > > page was already all-frozen (thus not empty). It would be fine since
> > > > > we don't need to change the visibility of the page in that case but
> > > > > the variable name is no longer correct.  set_all_visible or such?
> > > >
> > > > It seems to me that the variable name all_frozen_set corresponds to
> > > > XLH_INSERT_ALL_FROZEN_SET but I see your point. How about
> > > > set_all_frozen instead since we set all-frozen bits (also implying
> > > > setting all-visible)?
> > >
> > > Right. However, "if (set_all_frozen) then "set all_visible" looks like
> > > a bug^^;.  all_frozen_set looks better in that context than
> > > set_all_frozen. So I withdraw the comment.
> > >
> > > > BTW I found the following description of XLH_INSERT_ALL_FROZEN_SET but
> > > > there is no all_visible_set anywhere:
> > > >
> > > > /* all_frozen_set always implies all_visible_set */
> > > > #define XLH_INSERT_ALL_FROZEN_SET               (1<<5)
> > > >
> > > > I'll update those comments as well.
> > >
> > > FWIW, it seems like a shorthand of "ALL_FROZEN_SET implies ALL_VISIBLE
> > > to be set together". The current comment is working to me.
> > >
> >
> > Okay, I've updated the patch accordingly. Please review it.
>
> I was reading the patch, just found some typos: it should be "a frozen
> tuple" not "an frozen tuple".
>
> +     * Also pin visibility map page if we're inserting an frozen tuple into
> +                 * If we're inserting an frozen entry into empty page, pin the

Thank you for the comment.

I’ve updated the patch including the above comment.


Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Mon, Apr 19, 2021 at 7:21 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I’ve updated the patch including the above comment.

Thanks for the patch.

I was trying to understand below statements:
+                 * we check without a buffer lock if the page is empty but the
+                 * caller doesn't need to recheck that since we assume that in
+                 * HEAP_INSERT_FROZEN case, only one process is inserting a
+                 * frozen tuple into this relation.
+                 *

And earlier comments from upthread:

>> AFAICS the page is always empty when RelationGetBufferForTuple
>> returned a valid vmbuffer.  So the "if" should be an "assert" instead.

> There is a chance that RelationGetBufferForTuple() returns a valid
> vmbuffer but the page is not empty, since RelationGetBufferForTuple()
> checks without a lock if the page is empty. But when it comes to
> HEAP_INSERT_FROZEN cases it actually doesn’t happen at least for now
> since only one process inserts tuples into the relation. Will fix."

I'm not sure whether it is safe to assume "at least for now since only
one process inserts tuples into the relation". What if we allow
parallel inserts for HEAP_INSERT_FROZEN cases, I don't know whether we
can do that or not. Correct me if I'm wrong.

While we are modifying something in heap_insert:
1) Can we adjust the comment below in heap_insert to the 80char limit?
      * If we're inserting frozen entry into an empty page,
      * set visibility map bits and PageAllVisible() hint.
2) I'm thinking whether we can do page = BufferGetPage(buffer); after
RelationGetBufferForTuple and use in all the places where currently
BufferGetPage(buffer) is being used:
if (PageIsAllVisible(BufferGetPage(buffer)),
PageClearAllVisible(BufferGetPage(buffer)); and we could even remove
the local variable page of if (RelationNeedsWAL(relation)).
3) We could as well get the block number once and use it in all the
places in heap_insert, thus we can remove extra calls of
BufferGetBlockNumber(buffer).

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 20, 2021 at 11:04 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Apr 19, 2021 at 7:21 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > I’ve updated the patch including the above comment.
>
> Thanks for the patch.
>
> I was trying to understand below statements:
> +                 * we check without a buffer lock if the page is empty but the
> +                 * caller doesn't need to recheck that since we assume that in
> +                 * HEAP_INSERT_FROZEN case, only one process is inserting a
> +                 * frozen tuple into this relation.
> +                 *
>
> And earlier comments from upthread:
>
> >> AFAICS the page is always empty when RelationGetBufferForTuple
> >> returned a valid vmbuffer.  So the "if" should be an "assert" instead.
>
> > There is a chance that RelationGetBufferForTuple() returns a valid
> > vmbuffer but the page is not empty, since RelationGetBufferForTuple()
> > checks without a lock if the page is empty. But when it comes to
> > HEAP_INSERT_FROZEN cases it actually doesn’t happen at least for now
> > since only one process inserts tuples into the relation. Will fix."
>
> I'm not sure whether it is safe to assume "at least for now since only
> one process inserts tuples into the relation". What if we allow
> parallel inserts for HEAP_INSERT_FROZEN cases, I don't know whether we
> can do that or not. Correct me if I'm wrong.

I think if my assumption is wrong or we allow parallel insert for
HEAP_INSERT_FROZEN cases in the future, we need to deal with the case
where frozen tuples are concurrently inserted into the same page. For
example, we can release vmbuffer when we see the page is no longer
empty, or we can return a valid buffer but require the caller to
re-check if the page is still empty. The previous version patch took
the former approach. More concretely, heap_insert() rechecked if the
page is still empty in HEAP_INSERT_FROZEN case and set all_frozen_set
if so. But AFAICS concurrently inserting frozen tuples into the same
page doesn’t happen for now (COPY FREEZE and REFRESH MATERIALIZED VIEW
are users of HEAP_INSERT_FROZEN), also pointed out by Horiguchi-san.
So I added comments and assertions rather than addressing the case
that never happens with the current code. If concurrently inserting
frozen tuples into the same page happens, we should get the assertion
failure that I added in RelationGetBufferForTuple().

>
> While we are modifying something in heap_insert:
> 1) Can we adjust the comment below in heap_insert to the 80char limit?
>       * If we're inserting frozen entry into an empty page,
>       * set visibility map bits and PageAllVisible() hint.
> 2) I'm thinking whether we can do page = BufferGetPage(buffer); after
> RelationGetBufferForTuple and use in all the places where currently
> BufferGetPage(buffer) is being used:
> if (PageIsAllVisible(BufferGetPage(buffer)),
> PageClearAllVisible(BufferGetPage(buffer)); and we could even remove
> the local variable page of if (RelationNeedsWAL(relation)).
> 3) We could as well get the block number once and use it in all the
> places in heap_insert, thus we can remove extra calls of
> BufferGetBlockNumber(buffer).

All points are reasonable to me. I'll incorporate them in the next version.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Tue, Apr 20, 2021 at 11:20 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Apr 20, 2021 at 11:04 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Mon, Apr 19, 2021 at 7:21 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > I’ve updated the patch including the above comment.
> >
> > Thanks for the patch.
> >
> > I was trying to understand below statements:
> > +                 * we check without a buffer lock if the page is empty but the
> > +                 * caller doesn't need to recheck that since we assume that in
> > +                 * HEAP_INSERT_FROZEN case, only one process is inserting a
> > +                 * frozen tuple into this relation.
> > +                 *
> >
> > And earlier comments from upthread:
> >
> > >> AFAICS the page is always empty when RelationGetBufferForTuple
> > >> returned a valid vmbuffer.  So the "if" should be an "assert" instead.
> >
> > > There is a chance that RelationGetBufferForTuple() returns a valid
> > > vmbuffer but the page is not empty, since RelationGetBufferForTuple()
> > > checks without a lock if the page is empty. But when it comes to
> > > HEAP_INSERT_FROZEN cases it actually doesn’t happen at least for now
> > > since only one process inserts tuples into the relation. Will fix."
> >
> > I'm not sure whether it is safe to assume "at least for now since only
> > one process inserts tuples into the relation". What if we allow
> > parallel inserts for HEAP_INSERT_FROZEN cases, I don't know whether we
> > can do that or not. Correct me if I'm wrong.
>
> I think if my assumption is wrong or we allow parallel insert for
> HEAP_INSERT_FROZEN cases in the future, we need to deal with the case
> where frozen tuples are concurrently inserted into the same page. For
> example, we can release vmbuffer when we see the page is no longer
> empty, or we can return a valid buffer but require the caller to
> re-check if the page is still empty. The previous version patch took
> the former approach. More concretely, heap_insert() rechecked if the
> page is still empty in HEAP_INSERT_FROZEN case and set all_frozen_set
> if so. But AFAICS concurrently inserting frozen tuples into the same
> page doesn’t happen for now (COPY FREEZE and REFRESH MATERIALIZED VIEW
> are users of HEAP_INSERT_FROZEN), also pointed out by Horiguchi-san.
> So I added comments and assertions rather than addressing the case
> that never happens with the current code. If concurrently inserting
> frozen tuples into the same page happens, we should get the assertion
> failure that I added in RelationGetBufferForTuple().

Upon thinking further, concurrent insertions into the same page are
not possible while we are in heap_insert in between
RelationGetBufferForTuple and UnlockReleaseBuffer(buffer);.
RelationGetBufferForTuple will lock the buffer in exclusive mode, see
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); and comment " *    Returns
pinned and exclusive-locked buffer of a page in given relation". Even
if parallel insertions are allowed in HEAP_INSERT_FROZEN cases, then
each worker will separately acquire pages, insert into them and they
skip getting visibility map page pin if the page is set all-visible by
another worker.

Some more comments on v3 patch:
1) Isn't it good to specify here that what we gain by avoiding pinning
visibility map page something like: gain a few seconds/avoid extra
function calls/or some other better wording?
+                 * If the page already is non-empty and all-visible, we skip to
+                 * pin on a visibility map buffer since we never clear and set
+                 * all-frozen bit on visibility map during inserting a frozen
+                 * tuple.
+                 */

2) Isn't it good to put PageIsAllVisible(BufferGetPage(buffer))) in
the if clause instead of else if clause, because this is going to be
hitting most of the time, we could avoid page empty check every time?
+                if (PageGetMaxOffsetNumber(BufferGetPage(buffer)) == 0)
+                    visibilitymap_pin(relation, targetBlock, vmbuffer);
+                else if (PageIsAllVisible(BufferGetPage(buffer)))
+                    skip_vmbuffer = true;

3) I found another typo in v3 - it is "will set" not "will sets":
+ *    In HEAP_INSERT_FROZEN cases, we handle the possibility that the
caller will
+ *    sets all-frozen bit on the visibility map page.  We pin on the visibility

4) I think a commit message can be added to the upcoming patch.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Hi,

I took a look at this today, as I committed 39b66a91b back in January. I 
can reproduce the issue, with just 1M rows the before/after timings are 
roughly 480ms and 620ms on my hardware.

Unfortunately, the v3 patch does not really fix the issue for me. The 
timing with it applied is ~610ms so the improvement is only minimal.

I'm not sure what to do about this :-( I don't have any ideas about how 
to eliminate this overhead, so the only option I see is reverting the 
changes in heap_insert. Unfortunately, that'd mean inserts into TOAST 
tables won't be frozen ...


regards

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

Hi,

On 2021-04-26 15:31:02 +0200, Tomas Vondra wrote:
> I'm not sure what to do about this :-( I don't have any ideas about how to
> eliminate this overhead, so the only option I see is reverting the changes
> in heap_insert. Unfortunately, that'd mean inserts into TOAST tables won't
> be frozen ...

ISTM that the fundamental issue here is not that we acquire pins that we
shouldn't, but that we do so at a much higher frequency than needed.

It's probably too invasive for 14, but I think it might be worth exploring
passing down a BulkInsertState in nodeModifyTable.c's table_tuple_insert() iff
the input will be more than one row.

And then add the vm buffer of the target page to BulkInsertState, so that
hio.c can avoid re-pinning the buffer.

Greetings,

Andres Freund

On 4/26/21 9:27 PM, Andres Freund wrote:
> Hi,
> 
> On 2021-04-26 15:31:02 +0200, Tomas Vondra wrote:
>> I'm not sure what to do about this :-( I don't have any ideas about how to
>> eliminate this overhead, so the only option I see is reverting the changes
>> in heap_insert. Unfortunately, that'd mean inserts into TOAST tables won't
>> be frozen ...
> 
> ISTM that the fundamental issue here is not that we acquire pins that we
> shouldn't, but that we do so at a much higher frequency than needed.
> 
> It's probably too invasive for 14, but I think it might be worth exploring
> passing down a BulkInsertState in nodeModifyTable.c's table_tuple_insert() iff
> the input will be more than one row.
> 
> And then add the vm buffer of the target page to BulkInsertState, so that
> hio.c can avoid re-pinning the buffer.
> 

Yeah. The question still is what to do about 14, though. Shall we leave 
the code as it is now, or should we change it somehow? It seem a bit 
unfortunate that a COPY FREEZE optimization should negatively influence 
other (more) common use cases, so I guess we can't just keep the current 
code ...

regards

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

Hi,

On 2021-04-26 23:59:17 +0200, Tomas Vondra wrote:
> On 4/26/21 9:27 PM, Andres Freund wrote:
> > On 2021-04-26 15:31:02 +0200, Tomas Vondra wrote:
> > > I'm not sure what to do about this :-( I don't have any ideas about how to
> > > eliminate this overhead, so the only option I see is reverting the changes
> > > in heap_insert. Unfortunately, that'd mean inserts into TOAST tables won't
> > > be frozen ...
> > 
> > ISTM that the fundamental issue here is not that we acquire pins that we
> > shouldn't, but that we do so at a much higher frequency than needed.
> > 
> > It's probably too invasive for 14, but I think it might be worth exploring
> > passing down a BulkInsertState in nodeModifyTable.c's table_tuple_insert() iff
> > the input will be more than one row.
> > 
> > And then add the vm buffer of the target page to BulkInsertState, so that
> > hio.c can avoid re-pinning the buffer.
> > 
> 
> Yeah. The question still is what to do about 14, though. Shall we leave the
> code as it is now, or should we change it somehow? It seem a bit unfortunate
> that a COPY FREEZE optimization should negatively influence other (more)
> common use cases, so I guess we can't just keep the current code ...

I'd suggest prototyping the use of BulkInsertState in nodeModifyTable.c
and see whether that fixes the regression. If it does, then we can
analyze whether that's possibly the best way forward. Or whether we
revert, live with the regression or find yet another path.

Greetings,

Andres Freund

On Mon, Apr 26, 2021 at 10:31 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> Hi,
>
> I took a look at this today, as I committed 39b66a91b back in January. I
> can reproduce the issue, with just 1M rows the before/after timings are
> roughly 480ms and 620ms on my hardware.
>
> Unfortunately, the v3 patch does not really fix the issue for me. The
> timing with it applied is ~610ms so the improvement is only minimal.

Since the reading vmbuffer is likely to hit on the shared buffer
during inserting frozen tuples, I think the improvement would not be
visible with a few million tuples depending on hardware. But it might
not be as fast as before commit 39b66a91b since we read vmbuffer at
least per insertion.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Tue, Apr 27, 2021 at 8:07 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2021-04-26 23:59:17 +0200, Tomas Vondra wrote:
> > On 4/26/21 9:27 PM, Andres Freund wrote:
> > > On 2021-04-26 15:31:02 +0200, Tomas Vondra wrote:
> > > > I'm not sure what to do about this :-( I don't have any ideas about how to
> > > > eliminate this overhead, so the only option I see is reverting the changes
> > > > in heap_insert. Unfortunately, that'd mean inserts into TOAST tables won't
> > > > be frozen ...
> > >
> > > ISTM that the fundamental issue here is not that we acquire pins that we
> > > shouldn't, but that we do so at a much higher frequency than needed.
> > >
> > > It's probably too invasive for 14, but I think it might be worth exploring
> > > passing down a BulkInsertState in nodeModifyTable.c's table_tuple_insert() iff
> > > the input will be more than one row.
> > >
> > > And then add the vm buffer of the target page to BulkInsertState, so that
> > > hio.c can avoid re-pinning the buffer.
> > >
> >
> > Yeah. The question still is what to do about 14, though. Shall we leave the
> > code as it is now, or should we change it somehow? It seem a bit unfortunate
> > that a COPY FREEZE optimization should negatively influence other (more)
> > common use cases, so I guess we can't just keep the current code ...
>
> I'd suggest prototyping the use of BulkInsertState in nodeModifyTable.c
> and see whether that fixes the regression.

Is this idea to have RelationGetBufferForTuple() skip re-pinning
vmbuffer? If so, is this essentially the same as the one in the v3
patch?

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On 4/27/21 7:34 AM, Masahiko Sawada wrote:
> On Tue, Apr 27, 2021 at 8:07 AM Andres Freund <andres@anarazel.de> wrote:
>>
>> Hi,
>>
>> On 2021-04-26 23:59:17 +0200, Tomas Vondra wrote:
>>> On 4/26/21 9:27 PM, Andres Freund wrote:
>>>> On 2021-04-26 15:31:02 +0200, Tomas Vondra wrote:
>>>>> I'm not sure what to do about this :-( I don't have any ideas about how to
>>>>> eliminate this overhead, so the only option I see is reverting the changes
>>>>> in heap_insert. Unfortunately, that'd mean inserts into TOAST tables won't
>>>>> be frozen ...
>>>>
>>>> ISTM that the fundamental issue here is not that we acquire pins that we
>>>> shouldn't, but that we do so at a much higher frequency than needed.
>>>>
>>>> It's probably too invasive for 14, but I think it might be worth exploring
>>>> passing down a BulkInsertState in nodeModifyTable.c's table_tuple_insert() iff
>>>> the input will be more than one row.
>>>>
>>>> And then add the vm buffer of the target page to BulkInsertState, so that
>>>> hio.c can avoid re-pinning the buffer.
>>>>
>>>
>>> Yeah. The question still is what to do about 14, though. Shall we leave the
>>> code as it is now, or should we change it somehow? It seem a bit unfortunate
>>> that a COPY FREEZE optimization should negatively influence other (more)
>>> common use cases, so I guess we can't just keep the current code ...
>>
>> I'd suggest prototyping the use of BulkInsertState in nodeModifyTable.c
>> and see whether that fixes the regression.
> 
> Is this idea to have RelationGetBufferForTuple() skip re-pinning
> vmbuffer? If so, is this essentially the same as the one in the v3
> patch?
> 

I don't think it is the same approach - it's a bit hard to follow what 
exactly happens in RelationGetBufferForTuple, but AFAICS it always 
starts with vmbuffer = InvalidBuffer, so it may pin the vmbuffer quite 
often, no?

What Andres is suggesting (I think) is to modify ExecInsert() to pass a 
valid bistate to table_tuple_insert, instead of just NULL, and store the 
vmbuffer in it. Not sure how to identify when inserting more than just a 
single row, though ...


regards

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

On Tue, Apr 27, 2021 at 7:13 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
> What Andres is suggesting (I think) is to modify ExecInsert() to pass a
> valid bistate to table_tuple_insert, instead of just NULL, and store the
> vmbuffer in it. Not sure how to identify when inserting more than just a
> single row, though ...

I think the thread "should INSERT SELECT use a BulkInsertState?" [1],
has a simple dynamic mechanism [with a GUC defining the threshold
tuples] to switch over to using BulkInsertState. Maybe it's worth
having a look at the patch -
0001-INSERT-SELECT-to-use-BulkInsertState-and-multi_i.patch?

+    /* Use bulk insert after a threshold number of tuples */
+    // XXX: maybe this should only be done if it's not a partitioned table or
+    // if the partitions don't support miinfo, which uses its own bistates
+    mtstate->ntuples++;
+    if (mtstate->bistate == NULL &&
+            mtstate->operation == CMD_INSERT &&
+            mtstate->ntuples > bulk_insert_ntuples &&
+            bulk_insert_ntuples >= 0)
+    {
+        elog(DEBUG1, "enabling bulk insert");
+        mtstate->bistate = GetBulkInsertState();
+    }

[1] https://www.postgresql.org/message-id/20210222030158.GS14772%40telsasoft.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 27, 2021 at 03:43:07PM +0200, Tomas Vondra wrote:
> On 4/27/21 7:34 AM, Masahiko Sawada wrote:
> > On Tue, Apr 27, 2021 at 8:07 AM Andres Freund <andres@anarazel.de> wrote:
> > > On 2021-04-26 23:59:17 +0200, Tomas Vondra wrote:
> > > > On 4/26/21 9:27 PM, Andres Freund wrote:
> > > > > On 2021-04-26 15:31:02 +0200, Tomas Vondra wrote:
> > > > > > I'm not sure what to do about this :-( I don't have any ideas about how to
> > > > > > eliminate this overhead, so the only option I see is reverting the changes
> > > > > > in heap_insert. Unfortunately, that'd mean inserts into TOAST tables won't
> > > > > > be frozen ...
> > > > > 
> > > > > ISTM that the fundamental issue here is not that we acquire pins that we
> > > > > shouldn't, but that we do so at a much higher frequency than needed.
> > > > > 
> > > > > It's probably too invasive for 14, but I think it might be worth exploring
> > > > > passing down a BulkInsertState in nodeModifyTable.c's table_tuple_insert() iff
> > > > > the input will be more than one row.
> > > > > 
> > > > > And then add the vm buffer of the target page to BulkInsertState, so that
> > > > > hio.c can avoid re-pinning the buffer.
> > > > > 
> > > > 
> > > > Yeah. The question still is what to do about 14, though. Shall we leave the
> > > > code as it is now, or should we change it somehow? It seem a bit unfortunate
> > > > that a COPY FREEZE optimization should negatively influence other (more)
> > > > common use cases, so I guess we can't just keep the current code ...
> > > 
> > > I'd suggest prototyping the use of BulkInsertState in nodeModifyTable.c
> > > and see whether that fixes the regression.
> 
> What Andres is suggesting (I think) is to modify ExecInsert() to pass a
> valid bistate to table_tuple_insert, instead of just NULL, and store the
> vmbuffer in it. Not sure how to identify when inserting more than just a
> single row, though ...

Maybe this is relevant.
https://commitfest.postgresql.org/33/2553/
| INSERT SELECT: BulkInsertState and table_multi_insert

The biistate part is small - Simon requested to also use table_multi_insert,
which makes the patch much bigger, and there's probably lots of restrictions I
haven't even thought to check.

This uses a GUC threshold for bulk insert, but I'm still not sure it's really
problematic to use a biistate for a single row.

        /* Use bulk insert after a threshold number of tuples */
        // XXX: maybe this should only be done if it's not a partitioned table or
        // if the partitions don't support miinfo, which uses its own bistates
        mtstate->ntuples++;
        if (mtstate->bistate == NULL &&
                        mtstate->ntuples > bulk_insert_ntuples &&
                        bulk_insert_ntuples >= 0)
        {
                elog(DEBUG1, "enabling bulk insert");
                mtstate->bistate = GetBulkInsertState();

-- 
Justin

On Tue, Apr 27, 2021 at 10:43 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
>
>
> On 4/27/21 7:34 AM, Masahiko Sawada wrote:
> > On Tue, Apr 27, 2021 at 8:07 AM Andres Freund <andres@anarazel.de> wrote:
> >>
> >> Hi,
> >>
> >> On 2021-04-26 23:59:17 +0200, Tomas Vondra wrote:
> >>> On 4/26/21 9:27 PM, Andres Freund wrote:
> >>>> On 2021-04-26 15:31:02 +0200, Tomas Vondra wrote:
> >>>>> I'm not sure what to do about this :-( I don't have any ideas about how to
> >>>>> eliminate this overhead, so the only option I see is reverting the changes
> >>>>> in heap_insert. Unfortunately, that'd mean inserts into TOAST tables won't
> >>>>> be frozen ...
> >>>>
> >>>> ISTM that the fundamental issue here is not that we acquire pins that we
> >>>> shouldn't, but that we do so at a much higher frequency than needed.
> >>>>
> >>>> It's probably too invasive for 14, but I think it might be worth exploring
> >>>> passing down a BulkInsertState in nodeModifyTable.c's table_tuple_insert() iff
> >>>> the input will be more than one row.
> >>>>
> >>>> And then add the vm buffer of the target page to BulkInsertState, so that
> >>>> hio.c can avoid re-pinning the buffer.
> >>>>
> >>>
> >>> Yeah. The question still is what to do about 14, though. Shall we leave the
> >>> code as it is now, or should we change it somehow? It seem a bit unfortunate
> >>> that a COPY FREEZE optimization should negatively influence other (more)
> >>> common use cases, so I guess we can't just keep the current code ...
> >>
> >> I'd suggest prototyping the use of BulkInsertState in nodeModifyTable.c
> >> and see whether that fixes the regression.
> >
> > Is this idea to have RelationGetBufferForTuple() skip re-pinning
> > vmbuffer? If so, is this essentially the same as the one in the v3
> > patch?
> >
>
> I don't think it is the same approach - it's a bit hard to follow what
> exactly happens in RelationGetBufferForTuple, but AFAICS it always
> starts with vmbuffer = InvalidBuffer, so it may pin the vmbuffer quite
> often, no?

With that patch, we pin the vmbuffer only when inserting a frozen
tuple into an empty page. That is, when inserting a frozen tuple into
an empty page, we pin the vmbuffer and heap_insert() will mark the
page all-visible and set all-frozen bit on vm. And from the next
insertion (into the same page) until the page gets full, since the
page is already all-visible, we skip pinning the vmbuffer. IOW, if the
target page is not empty but all-visible, we skip pinning the
vmbuffer. We pin the vmbuffer only once per heap page used during
insertion.

>
> What Andres is suggesting (I think) is to modify ExecInsert() to pass a
> valid bistate to table_tuple_insert, instead of just NULL, and store the
> vmbuffer in it.

Understood. This approach keeps using the same vmbuffer until we need
another vm page corresponding to the target heap page, which seems
better.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Wed, Apr 28, 2021 at 12:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Apr 27, 2021 at 10:43 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
> >
> >
> >
> > On 4/27/21 7:34 AM, Masahiko Sawada wrote:
> > > On Tue, Apr 27, 2021 at 8:07 AM Andres Freund <andres@anarazel.de> wrote:
> > >>
> > >> Hi,
> > >>
> > >> On 2021-04-26 23:59:17 +0200, Tomas Vondra wrote:
> > >>> On 4/26/21 9:27 PM, Andres Freund wrote:
> > >>>> On 2021-04-26 15:31:02 +0200, Tomas Vondra wrote:
> > >>>>> I'm not sure what to do about this :-( I don't have any ideas about how to
> > >>>>> eliminate this overhead, so the only option I see is reverting the changes
> > >>>>> in heap_insert. Unfortunately, that'd mean inserts into TOAST tables won't
> > >>>>> be frozen ...
> > >>>>
> > >>>> ISTM that the fundamental issue here is not that we acquire pins that we
> > >>>> shouldn't, but that we do so at a much higher frequency than needed.
> > >>>>
> > >>>> It's probably too invasive for 14, but I think it might be worth exploring
> > >>>> passing down a BulkInsertState in nodeModifyTable.c's table_tuple_insert() iff
> > >>>> the input will be more than one row.
> > >>>>
> > >>>> And then add the vm buffer of the target page to BulkInsertState, so that
> > >>>> hio.c can avoid re-pinning the buffer.
> > >>>>
> > >>>
> > >>> Yeah. The question still is what to do about 14, though. Shall we leave the
> > >>> code as it is now, or should we change it somehow? It seem a bit unfortunate
> > >>> that a COPY FREEZE optimization should negatively influence other (more)
> > >>> common use cases, so I guess we can't just keep the current code ...
> > >>
> > >> I'd suggest prototyping the use of BulkInsertState in nodeModifyTable.c
> > >> and see whether that fixes the regression.
> > >
> > > Is this idea to have RelationGetBufferForTuple() skip re-pinning
> > > vmbuffer? If so, is this essentially the same as the one in the v3
> > > patch?
> > >
> >
> > I don't think it is the same approach - it's a bit hard to follow what
> > exactly happens in RelationGetBufferForTuple, but AFAICS it always
> > starts with vmbuffer = InvalidBuffer, so it may pin the vmbuffer quite
> > often, no?
>
> With that patch, we pin the vmbuffer only when inserting a frozen
> tuple into an empty page. That is, when inserting a frozen tuple into
> an empty page, we pin the vmbuffer and heap_insert() will mark the
> page all-visible and set all-frozen bit on vm. And from the next
> insertion (into the same page) until the page gets full, since the
> page is already all-visible, we skip pinning the vmbuffer. IOW, if the
> target page is not empty but all-visible, we skip pinning the
> vmbuffer. We pin the vmbuffer only once per heap page used during
> insertion.
>
> >
> > What Andres is suggesting (I think) is to modify ExecInsert() to pass a
> > valid bistate to table_tuple_insert, instead of just NULL, and store the
> > vmbuffer in it.
>
> Understood. This approach keeps using the same vmbuffer until we need
> another vm page corresponding to the target heap page, which seems
> better.

But how is ExecInsert() related to REFRESH MATERIALIZED VIEW?

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On 4/27/21 5:44 PM, Masahiko Sawada wrote:
> On Wed, Apr 28, 2021 at 12:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>
>> On Tue, Apr 27, 2021 at 10:43 PM Tomas Vondra
>> <tomas.vondra@enterprisedb.com> wrote:
>>>
>>>
>>>
>>> On 4/27/21 7:34 AM, Masahiko Sawada wrote:
>>>> On Tue, Apr 27, 2021 at 8:07 AM Andres Freund <andres@anarazel.de> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 2021-04-26 23:59:17 +0200, Tomas Vondra wrote:
>>>>>> On 4/26/21 9:27 PM, Andres Freund wrote:
>>>>>>> On 2021-04-26 15:31:02 +0200, Tomas Vondra wrote:
>>>>>>>> I'm not sure what to do about this :-( I don't have any ideas about how to
>>>>>>>> eliminate this overhead, so the only option I see is reverting the changes
>>>>>>>> in heap_insert. Unfortunately, that'd mean inserts into TOAST tables won't
>>>>>>>> be frozen ...
>>>>>>>
>>>>>>> ISTM that the fundamental issue here is not that we acquire pins that we
>>>>>>> shouldn't, but that we do so at a much higher frequency than needed.
>>>>>>>
>>>>>>> It's probably too invasive for 14, but I think it might be worth exploring
>>>>>>> passing down a BulkInsertState in nodeModifyTable.c's table_tuple_insert() iff
>>>>>>> the input will be more than one row.
>>>>>>>
>>>>>>> And then add the vm buffer of the target page to BulkInsertState, so that
>>>>>>> hio.c can avoid re-pinning the buffer.
>>>>>>>
>>>>>>
>>>>>> Yeah. The question still is what to do about 14, though. Shall we leave the
>>>>>> code as it is now, or should we change it somehow? It seem a bit unfortunate
>>>>>> that a COPY FREEZE optimization should negatively influence other (more)
>>>>>> common use cases, so I guess we can't just keep the current code ...
>>>>>
>>>>> I'd suggest prototyping the use of BulkInsertState in nodeModifyTable.c
>>>>> and see whether that fixes the regression.
>>>>
>>>> Is this idea to have RelationGetBufferForTuple() skip re-pinning
>>>> vmbuffer? If so, is this essentially the same as the one in the v3
>>>> patch?
>>>>
>>>
>>> I don't think it is the same approach - it's a bit hard to follow what
>>> exactly happens in RelationGetBufferForTuple, but AFAICS it always
>>> starts with vmbuffer = InvalidBuffer, so it may pin the vmbuffer quite
>>> often, no?
>>
>> With that patch, we pin the vmbuffer only when inserting a frozen
>> tuple into an empty page. That is, when inserting a frozen tuple into
>> an empty page, we pin the vmbuffer and heap_insert() will mark the
>> page all-visible and set all-frozen bit on vm. And from the next
>> insertion (into the same page) until the page gets full, since the
>> page is already all-visible, we skip pinning the vmbuffer. IOW, if the
>> target page is not empty but all-visible, we skip pinning the
>> vmbuffer. We pin the vmbuffer only once per heap page used during
>> insertion.
>>
>>>
>>> What Andres is suggesting (I think) is to modify ExecInsert() to pass a
>>> valid bistate to table_tuple_insert, instead of just NULL, and store the
>>> vmbuffer in it.
>>
>> Understood. This approach keeps using the same vmbuffer until we need
>> another vm page corresponding to the target heap page, which seems
>> better.
> 
> But how is ExecInsert() related to REFRESH MATERIALIZED VIEW?
> 

TBH I haven't looked into the details, but Andres talked about 
nodeModifyTable and table_tuple_insert, and ExecInsert is the only place 
calling it. But maybe I'm just confused and Andres meant something else?


regards

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

Hi,

On 2021-04-28 00:44:47 +0900, Masahiko Sawada wrote:
> On Wed, Apr 28, 2021 at 12:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > What Andres is suggesting (I think) is to modify ExecInsert() to pass a
> > > valid bistate to table_tuple_insert, instead of just NULL, and store the
> > > vmbuffer in it.
> >
> > Understood. This approach keeps using the same vmbuffer until we need
> > another vm page corresponding to the target heap page, which seems
> > better.
> 
> But how is ExecInsert() related to REFRESH MATERIALIZED VIEW?

I was thinking of the CONCURRENTLY path for REFRESH MATERIALIZED VIEW I
think. Or something.

That actually makes it easier - we already pass in a bistate in the relevant
paths. So if we add a current_vmbuf to BulkInsertStateData, we can avoid
needing to pin so often. It seems that'd end up with a good bit cleaner and
less risky code than the skip_vmbuffer_for_frozen_tuple_insertion_v3.patch
approach.

The current RelationGetBufferForTuple() interface / how it's used in heapam.c
doesn't make this quite as trivial as it could be... Attached is a quick hack
implementing this. For me it reduces the overhead noticably:

REFRESH MATERIALIZED VIEW mv;
before:
Time: 26542.333 ms (00:26.542)
after:
Time: 23105.047 ms (00:23.105)

Greetings,

Andres Freund

On 4/27/21 8:22 PM, Andres Freund wrote:
> Hi,
> 
> On 2021-04-28 00:44:47 +0900, Masahiko Sawada wrote:
>> On Wed, Apr 28, 2021 at 12:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>> What Andres is suggesting (I think) is to modify ExecInsert() to pass a
>>>> valid bistate to table_tuple_insert, instead of just NULL, and store the
>>>> vmbuffer in it.
>>>
>>> Understood. This approach keeps using the same vmbuffer until we need
>>> another vm page corresponding to the target heap page, which seems
>>> better.
>>
>> But how is ExecInsert() related to REFRESH MATERIALIZED VIEW?
> 
> I was thinking of the CONCURRENTLY path for REFRESH MATERIALIZED VIEW I
> think. Or something.
> 
> That actually makes it easier - we already pass in a bistate in the relevant
> paths. So if we add a current_vmbuf to BulkInsertStateData, we can avoid
> needing to pin so often. It seems that'd end up with a good bit cleaner and
> less risky code than the skip_vmbuffer_for_frozen_tuple_insertion_v3.patch
> approach.
> 
> The current RelationGetBufferForTuple() interface / how it's used in heapam.c
> doesn't make this quite as trivial as it could be... Attached is a quick hack
> implementing this. For me it reduces the overhead noticably:
> 
> REFRESH MATERIALIZED VIEW mv;
> before:
> Time: 26542.333 ms (00:26.542)
> after:
> Time: 23105.047 ms (00:23.105)
> 

Thanks, that looks promising. I repeated the tests I did on 26/4, and 
the results look like this:

old (0c7d3bb99): 497ms
master:          621ms
patched:         531ms

So yeah, that's a bit improvement - it does not remove the regression 
entirely, but +5% is much better than +25%.


regards

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

On Wed, May 05, 2021 at 03:04:53PM +0200, Tomas Vondra wrote:
> Thanks, that looks promising. I repeated the tests I did on 26/4, and the
> results look like this:
>
> old (0c7d3bb99): 497ms
> master:          621ms
> patched:         531ms
>
> So yeah, that's a bit improvement - it does not remove the regression
> entirely, but +5% is much better than +25%.

Hmm.  Is that really something we should do after feature freeze?  A
25% degradation for matview refresh may be a problem for a lot of
users and could be an upgrade stopper.  Another thing we could do is
also to revert 7db0cd2 and 39b66a9 from the v14 tree, and work on a
proper solution for this performance problem for matviews for 15~.

Thoughts?
--
Michael

On Tue, May 11, 2021 at 4:37 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, May 05, 2021 at 03:04:53PM +0200, Tomas Vondra wrote:
> > Thanks, that looks promising. I repeated the tests I did on 26/4, and the
> > results look like this:
> >
> > old (0c7d3bb99): 497ms
> > master:          621ms
> > patched:         531ms
> >
> > So yeah, that's a bit improvement - it does not remove the regression
> > entirely, but +5% is much better than +25%.
>
> Hmm.  Is that really something we should do after feature freeze?  A
> 25% degradation for matview refresh may be a problem for a lot of
> users and could be an upgrade stopper.  Another thing we could do is
> also to revert 7db0cd2 and 39b66a9 from the v14 tree, and work on a
> proper solution for this performance problem for matviews for 15~.

I think the approach proposed by Andres eliminates the extra vmbuffer
reads as much as possible. But even with the patch, there still is 5%
degradation (and there is no way to disable inserting frozen tuples at
matview refresh). Which could be a problem for some users. I think
it’s hard to completely eliminate the overhead so we might need to
consider another approach like having matview refresh use
heap_multi_insert() instead of heap_insert().

I think the changes for heap_multi_insert() are fine so we can revert
only heap_insert() part if we revert something from the v14 tree,
although we will end up not inserting frozen tuples into toast tables.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Tue, May 11, 2021 at 2:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> I think the approach proposed by Andres eliminates the extra vmbuffer
> reads as much as possible. But even with the patch, there still is 5%
> degradation (and there is no way to disable inserting frozen tuples at
> matview refresh). Which could be a problem for some users. I think
> it’s hard to completely eliminate the overhead so we might need to
> consider another approach like having matview refresh use
> heap_multi_insert() instead of heap_insert().

I may not have understood what's being discussed here completely, but
if you want to use multi inserts for refresh matview code, maybe the
"New Table Access Methods for Multi and Single Inserts" patches at
[1], can help.

[1] - https://www.postgresql.org/message-id/CALj2ACXdrOmB6Na9amHWZHKvRT3Z0nwTRsCwoMT-npOBtmXLXg%40mail.gmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 5/11/21 12:58 PM, Bharath Rupireddy wrote:
> On Tue, May 11, 2021 at 2:34 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> I think the approach proposed by Andres eliminates the extra vmbuffer
>> reads as much as possible. But even with the patch, there still is 5%
>> degradation (and there is no way to disable inserting frozen tuples at
>> matview refresh). Which could be a problem for some users. I think
>> it’s hard to completely eliminate the overhead so we might need to
>> consider another approach like having matview refresh use
>> heap_multi_insert() instead of heap_insert().
> 
> I may not have understood what's being discussed here completely, but
> if you want to use multi inserts for refresh matview code, maybe the
> "New Table Access Methods for Multi and Single Inserts" patches at
> [1], can help.
> 

Maybe, but I think the main question is what to do for v14, so the 
uncommitted patch is kinda irrelevant.


regards

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

On 5/11/21 11:04 AM, Masahiko Sawada wrote:
> On Tue, May 11, 2021 at 4:37 PM Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Wed, May 05, 2021 at 03:04:53PM +0200, Tomas Vondra wrote:
>>> Thanks, that looks promising. I repeated the tests I did on 26/4, and the
>>> results look like this:
>>>
>>> old (0c7d3bb99): 497ms
>>> master:          621ms
>>> patched:         531ms
>>>
>>> So yeah, that's a bit improvement - it does not remove the regression
>>> entirely, but +5% is much better than +25%.
>>
>> Hmm.  Is that really something we should do after feature freeze?  A
>> 25% degradation for matview refresh may be a problem for a lot of
>> users and could be an upgrade stopper.  Another thing we could do is
>> also to revert 7db0cd2 and 39b66a9 from the v14 tree, and work on a
>> proper solution for this performance problem for matviews for 15~.
> 
> I think the approach proposed by Andres eliminates the extra vmbuffer
> reads as much as possible. But even with the patch, there still is 5%
> degradation (and there is no way to disable inserting frozen tuples at
> matview refresh). Which could be a problem for some users. I think
> it’s hard to completely eliminate the overhead so we might need to
> consider another approach like having matview refresh use
> heap_multi_insert() instead of heap_insert().
> 

I think it's way too late to make such significant change (switching to 
heap_multi_insert) for v14 :-( Moreover, I doubt it affects just matview 
refresh - why wouldn't it affect other similar use cases? More likely 
it's just the case that was discovered.

> I think the changes for heap_multi_insert() are fine so we can revert
> only heap_insert() part if we revert something from the v14 tree,
> although we will end up not inserting frozen tuples into toast tables.
> 

I'd be somewhat unhappy about reverting just this bit, because it'd mean 
that we freeze rows in the main table but not rows in the TOAST tables 
(that was kinda why we concluded we need the heap_insert part too).

I'm still a bit puzzled where does the extra overhead (in cases when 
freeze is not requested) come from, TBH. Intuitively, I'd hope there's a 
way to eliminate that entirely, and only pay the cost when requested 
(with the expectation that it's cheaper than freezing it that later).


regards

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

On Tue, May 11, 2021 at 11:07 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> On 5/11/21 11:04 AM, Masahiko Sawada wrote:
> > On Tue, May 11, 2021 at 4:37 PM Michael Paquier <michael@paquier.xyz> wrote:
> >>
> >> On Wed, May 05, 2021 at 03:04:53PM +0200, Tomas Vondra wrote:
> >>> Thanks, that looks promising. I repeated the tests I did on 26/4, and the
> >>> results look like this:
> >>>
> >>> old (0c7d3bb99): 497ms
> >>> master:          621ms
> >>> patched:         531ms
> >>>
> >>> So yeah, that's a bit improvement - it does not remove the regression
> >>> entirely, but +5% is much better than +25%.
> >>
> >> Hmm.  Is that really something we should do after feature freeze?  A
> >> 25% degradation for matview refresh may be a problem for a lot of
> >> users and could be an upgrade stopper.  Another thing we could do is
> >> also to revert 7db0cd2 and 39b66a9 from the v14 tree, and work on a
> >> proper solution for this performance problem for matviews for 15~.
> >
> > I think the approach proposed by Andres eliminates the extra vmbuffer
> > reads as much as possible. But even with the patch, there still is 5%
> > degradation (and there is no way to disable inserting frozen tuples at
> > matview refresh). Which could be a problem for some users. I think
> > it’s hard to completely eliminate the overhead so we might need to
> > consider another approach like having matview refresh use
> > heap_multi_insert() instead of heap_insert().
> >
>
> I think it's way too late to make such significant change (switching to
> heap_multi_insert) for v14 :-(

Right.

> Moreover, I doubt it affects just matview
> refresh - why wouldn't it affect other similar use cases? More likely
> it's just the case that was discovered.

I've not tested yet but I guess COPY FROM …  FREEZE using heap_insert
would similarly be affected since it also uses heap_insert() with
TABLE_INSERT_FROZEN.

>
> > I think the changes for heap_multi_insert() are fine so we can revert
> > only heap_insert() part if we revert something from the v14 tree,
> > although we will end up not inserting frozen tuples into toast tables.
> >
>
> I'd be somewhat unhappy about reverting just this bit, because it'd mean
> that we freeze rows in the main table but not rows in the TOAST tables
> (that was kinda why we concluded we need the heap_insert part too).
>
> I'm still a bit puzzled where does the extra overhead (in cases when
> freeze is not requested) come from, TBH.

Which cases do you mean? Doesn't matview refresh always request to
freeze tuples even after applying the patch proposed on this thread?

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Hi,

On 2021-05-11 16:07:44 +0200, Tomas Vondra wrote:
> On 5/11/21 11:04 AM, Masahiko Sawada wrote:
> > I think the changes for heap_multi_insert() are fine so we can revert
> > only heap_insert() part if we revert something from the v14 tree,
> > although we will end up not inserting frozen tuples into toast tables.
> > 
> 
> I'd be somewhat unhappy about reverting just this bit, because it'd mean
> that we freeze rows in the main table but not rows in the TOAST tables (that
> was kinda why we concluded we need the heap_insert part too).

Is there a reason not to apply a polished version of my proposal? And
then to look at the remaining difference?


> I'm still a bit puzzled where does the extra overhead (in cases when freeze
> is not requested) come from, TBH. Intuitively, I'd hope there's a way to
> eliminate that entirely, and only pay the cost when requested (with the
> expectation that it's cheaper than freezing it that later).

I'd like to see a profile comparison between those two cases. Best with
both profiles done in master, just once with the freeze path disabled...

Greetings,

Andres Freund

On 5/11/21 5:56 PM, Masahiko Sawada wrote:
> On Tue, May 11, 2021 at 11:07 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> On 5/11/21 11:04 AM, Masahiko Sawada wrote:
>>> On Tue, May 11, 2021 at 4:37 PM Michael Paquier <michael@paquier.xyz> wrote:
>>>>
>>>> On Wed, May 05, 2021 at 03:04:53PM +0200, Tomas Vondra wrote:
>>>>> Thanks, that looks promising. I repeated the tests I did on 26/4, and the
>>>>> results look like this:
>>>>>
>>>>> old (0c7d3bb99): 497ms
>>>>> master:          621ms
>>>>> patched:         531ms
>>>>>
>>>>> So yeah, that's a bit improvement - it does not remove the regression
>>>>> entirely, but +5% is much better than +25%.
>>>>
>>>> Hmm.  Is that really something we should do after feature freeze?  A
>>>> 25% degradation for matview refresh may be a problem for a lot of
>>>> users and could be an upgrade stopper.  Another thing we could do is
>>>> also to revert 7db0cd2 and 39b66a9 from the v14 tree, and work on a
>>>> proper solution for this performance problem for matviews for 15~.
>>>
>>> I think the approach proposed by Andres eliminates the extra vmbuffer
>>> reads as much as possible. But even with the patch, there still is 5%
>>> degradation (and there is no way to disable inserting frozen tuples at
>>> matview refresh). Which could be a problem for some users. I think
>>> it’s hard to completely eliminate the overhead so we might need to
>>> consider another approach like having matview refresh use
>>> heap_multi_insert() instead of heap_insert().
>>>
>>
>> I think it's way too late to make such significant change (switching to
>> heap_multi_insert) for v14 :-(
> 
> Right.
> 
>> Moreover, I doubt it affects just matview
>> refresh - why wouldn't it affect other similar use cases? More likely
>> it's just the case that was discovered.
> 
> I've not tested yet but I guess COPY FROM …  FREEZE using heap_insert
> would similarly be affected since it also uses heap_insert() with
> TABLE_INSERT_FROZEN.
> 

I'd say that's somewhat acceptable, as it's a trade-off between paying a 
bit of time during COPY vs. paying much more later (when freezing the 
rows eventually).

 From my POV the problem here is we've not asked to freeze the rows 
(unless I'm missing something and REFRESH freezes them?), but it's still 
a bit slower. However, 5% might also be just noise due to changes in 
layout of the binary.

>>
>>> I think the changes for heap_multi_insert() are fine so we can revert
>>> only heap_insert() part if we revert something from the v14 tree,
>>> although we will end up not inserting frozen tuples into toast tables.
>>>
>>
>> I'd be somewhat unhappy about reverting just this bit, because it'd mean
>> that we freeze rows in the main table but not rows in the TOAST tables
>> (that was kinda why we concluded we need the heap_insert part too).
>>
>> I'm still a bit puzzled where does the extra overhead (in cases when
>> freeze is not requested) come from, TBH.
> 
> Which cases do you mean? Doesn't matview refresh always request to
> freeze tuples even after applying the patch proposed on this thread?
> 

Oh, I didn't realize that! That'd make this much less of an issue, I'd 
say, because if we're intentionally freezing the rows it's reasonable to 
pay a bit of time (in exchange for not having to do it later). The 
original +25% was a bit too much, of course, but +5% seems reasonable.

FWIW I'm on vacation until the end of this week, I can't do much testing 
at the moment. Sorry.


regards

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

On 5/11/21 7:25 PM, Andres Freund wrote:
> Hi,
> 
> On 2021-05-11 16:07:44 +0200, Tomas Vondra wrote:
>> On 5/11/21 11:04 AM, Masahiko Sawada wrote:
>>> I think the changes for heap_multi_insert() are fine so we can revert
>>> only heap_insert() part if we revert something from the v14 tree,
>>> although we will end up not inserting frozen tuples into toast tables.
>>>
>>
>> I'd be somewhat unhappy about reverting just this bit, because it'd mean
>> that we freeze rows in the main table but not rows in the TOAST tables (that
>> was kinda why we concluded we need the heap_insert part too).
> 
> Is there a reason not to apply a polished version of my proposal? And
> then to look at the remaining difference?
> 

Probably not, I was just a little bit confused what exactly is going on, 
unsure what to do about it. But if RMV freezes the rows, that probably 
explains it and your patch is the way to go.

> 
>> I'm still a bit puzzled where does the extra overhead (in cases when freeze
>> is not requested) come from, TBH. Intuitively, I'd hope there's a way to
>> eliminate that entirely, and only pay the cost when requested (with the
>> expectation that it's cheaper than freezing it that later).
> 
> I'd like to see a profile comparison between those two cases. Best with
> both profiles done in master, just once with the freeze path disabled...
> 

OK. I'm mostly afk at the moment, I'll do that once I get back home, 
sometime over the weekend / maybe early next week.


regards

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

On 2021-May-11, Michael Paquier wrote:

> Hmm.  Is that really something we should do after feature freeze?  A
> 25% degradation for matview refresh may be a problem for a lot of
> users and could be an upgrade stopper.  Another thing we could do is
> also to revert 7db0cd2 and 39b66a9 from the v14 tree, and work on a
> proper solution for this performance problem for matviews for 15~.
> 
> Thoughts?

My main thought while reading this thread is about the rules of feature
freeze.  I mean, we are indeed in feature freeze, so no new features
should be added.  But that doesn't mean we are in code freeze.  For the
period starting now and until RC (which is a couple of months away
still) we should focus on ensuring that the features we do have are in
as good a shape as possible.  If that means adding more code to fix
problems/bugs/performance problems in the existing code, so be it.
I mean, reverting is not the only tool we have.

Yes, reverting has its place.  Moreover, threats of reversion have their
place.  People should definitely be working towards finding solutions to
the problems in their commits lest they be reverted.  However, freezing
*people* by saying that no fixes are acceptable other than reverts ...
is not good.

So I agree with what Andres is saying downthread: let's apply the fix he
proposed (it's not even that invasive anyway), and investigate the
remaining 5% and see if we can find a solution.  If by the end of the
beta process we can definitely find no solution to the problem, we can
revert the whole lot then.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

On 5/11/21 2:23 PM, Alvaro Herrera wrote:
> On 2021-May-11, Michael Paquier wrote:
>
>> Hmm.  Is that really something we should do after feature freeze?  A
>> 25% degradation for matview refresh may be a problem for a lot of
>> users and could be an upgrade stopper.  Another thing we could do is
>> also to revert 7db0cd2 and 39b66a9 from the v14 tree, and work on a
>> proper solution for this performance problem for matviews for 15~.
>>
>> Thoughts?
> My main thought while reading this thread is about the rules of feature
> freeze.  I mean, we are indeed in feature freeze, so no new features
> should be added.  But that doesn't mean we are in code freeze.  For the
> period starting now and until RC (which is a couple of months away
> still) we should focus on ensuring that the features we do have are in
> as good a shape as possible.  If that means adding more code to fix
> problems/bugs/performance problems in the existing code, so be it.
> I mean, reverting is not the only tool we have.
>
> Yes, reverting has its place.  Moreover, threats of reversion have their
> place.  People should definitely be working towards finding solutions to
> the problems in their commits lest they be reverted.  However, freezing
> *people* by saying that no fixes are acceptable other than reverts ...
> is not good.
>
> So I agree with what Andres is saying downthread: let's apply the fix he
> proposed (it's not even that invasive anyway), and investigate the
> remaining 5% and see if we can find a solution.  If by the end of the
> beta process we can definitely find no solution to the problem, we can
> revert the whole lot then.
>


I agree with all of this. Right now I'm only concerned if there isn't
work apparently being done on some issue.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Tue, May 11, 2021 at 02:46:35PM -0400, Andrew Dunstan wrote:
> On 5/11/21 2:23 PM, Alvaro Herrera wrote:
>> Yes, reverting has its place.  Moreover, threats of reversion have their
>> place.  People should definitely be working towards finding solutions to
>> the problems in their commits lest they be reverted.  However, freezing
>> *people* by saying that no fixes are acceptable other than reverts ...
>> is not good.

Well, that's an option on the table and a possibility, so I am listing
it as a possible exit path as a potential solution, as much as a
different optimization is another exit path to take care of this item
:)

>> So I agree with what Andres is saying downthread: let's apply the fix he
>> proposed (it's not even that invasive anyway), and investigate the
>> remaining 5% and see if we can find a solution.  If by the end of the
>> beta process we can definitely find no solution to the problem, we can
>> revert the whole lot then.
>
> I agree with all of this. Right now I'm only concerned if there isn't
> work apparently being done on some issue.

If that's the consensus reached, that's fine by me as long as we don't
keep a 25% performance regression.  Now, looking at the patch
proposed, I have to admit that this looks like some redesign of an
existing feature, so that stresses me a bit in a period when we are
aiming at making things stable, because this has a risk of making a
part of the code more unstable.  And I've had my share of calls over
the last years in such situations, not only with Postgres, FWIW, so I
may just sound like a conservative guy with a conservative hat.
--
Michael

Hi,

On 2021-05-13 11:12:43 +0900, Michael Paquier wrote:
> If that's the consensus reached, that's fine by me as long as we don't
> keep a 25% performance regression.  Now, looking at the patch
> proposed, I have to admit that this looks like some redesign of an
> existing feature, so that stresses me a bit in a period when we are
> aiming at making things stable, because this has a risk of making a
> part of the code more unstable.

You're referencing tracking the vm page in the bulk insert state? I
don't see how you get a less invasive fix that's not architecturally
worse than this. If that's over your level of comfort, I don't see an
alternative but to revert. But I also don't think it's particularly
invasive?

Greetings,

Andres Freund

On Tue, May 11, 2021 at 11:46 AM Andrew Dunstan <andrew@dunslane.net> wrote:
> > Yes, reverting has its place.  Moreover, threats of reversion have their
> > place.  People should definitely be working towards finding solutions to
> > the problems in their commits lest they be reverted.  However, freezing
> > *people* by saying that no fixes are acceptable other than reverts ...
> > is not good.
> >
> > So I agree with what Andres is saying downthread: let's apply the fix he
> > proposed (it's not even that invasive anyway), and investigate the
> > remaining 5% and see if we can find a solution.  If by the end of the
> > beta process we can definitely find no solution to the problem, we can
> > revert the whole lot then.
> >
>
>
> I agree with all of this. Right now I'm only concerned if there isn't
> work apparently being done on some issue.

+1. While reverting a patch is always on the table, it must be the
option of last resort. I don't have any specific reason to believe
that that's the point we're at just yet.

-- 
Peter Geoghegan

On Wed, May 12, 2021 at 2:32 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
>
>
> On 5/11/21 5:56 PM, Masahiko Sawada wrote:
> > On Tue, May 11, 2021 at 11:07 PM Tomas Vondra
> > <tomas.vondra@enterprisedb.com> wrote:
> >>
> >> On 5/11/21 11:04 AM, Masahiko Sawada wrote:
> >>> On Tue, May 11, 2021 at 4:37 PM Michael Paquier <michael@paquier.xyz> wrote:
> >>>>
> >>>> On Wed, May 05, 2021 at 03:04:53PM +0200, Tomas Vondra wrote:
> >>>>> Thanks, that looks promising. I repeated the tests I did on 26/4, and the
> >>>>> results look like this:
> >>>>>
> >>>>> old (0c7d3bb99): 497ms
> >>>>> master:          621ms
> >>>>> patched:         531ms
> >>>>>
> >>>>> So yeah, that's a bit improvement - it does not remove the regression
> >>>>> entirely, but +5% is much better than +25%.
> >>>>
> >>>> Hmm.  Is that really something we should do after feature freeze?  A
> >>>> 25% degradation for matview refresh may be a problem for a lot of
> >>>> users and could be an upgrade stopper.  Another thing we could do is
> >>>> also to revert 7db0cd2 and 39b66a9 from the v14 tree, and work on a
> >>>> proper solution for this performance problem for matviews for 15~.
> >>>
> >>> I think the approach proposed by Andres eliminates the extra vmbuffer
> >>> reads as much as possible. But even with the patch, there still is 5%
> >>> degradation (and there is no way to disable inserting frozen tuples at
> >>> matview refresh). Which could be a problem for some users. I think
> >>> it’s hard to completely eliminate the overhead so we might need to
> >>> consider another approach like having matview refresh use
> >>> heap_multi_insert() instead of heap_insert().
> >>>
> >>
> >> I think it's way too late to make such significant change (switching to
> >> heap_multi_insert) for v14 :-(
> >
> > Right.
> >
> >> Moreover, I doubt it affects just matview
> >> refresh - why wouldn't it affect other similar use cases? More likely
> >> it's just the case that was discovered.
> >
> > I've not tested yet but I guess COPY FROM …  FREEZE using heap_insert
> > would similarly be affected since it also uses heap_insert() with
> > TABLE_INSERT_FROZEN.
> >
>
> I'd say that's somewhat acceptable, as it's a trade-off between paying a
> bit of time during COPY vs. paying much more later (when freezing the
> rows eventually).
>
>  From my POV the problem here is we've not asked to freeze the rows
> (unless I'm missing something and REFRESH freezes them?), but it's still
> a bit slower. However, 5% might also be just noise due to changes in
> layout of the binary.
>
> >>
> >>> I think the changes for heap_multi_insert() are fine so we can revert
> >>> only heap_insert() part if we revert something from the v14 tree,
> >>> although we will end up not inserting frozen tuples into toast tables.
> >>>
> >>
> >> I'd be somewhat unhappy about reverting just this bit, because it'd mean
> >> that we freeze rows in the main table but not rows in the TOAST tables
> >> (that was kinda why we concluded we need the heap_insert part too).
> >>
> >> I'm still a bit puzzled where does the extra overhead (in cases when
> >> freeze is not requested) come from, TBH.
> >
> > Which cases do you mean? Doesn't matview refresh always request to
> > freeze tuples even after applying the patch proposed on this thread?
> >
>
> Oh, I didn't realize that! That'd make this much less of an issue, I'd
> say, because if we're intentionally freezing the rows it's reasonable to
> pay a bit of time (in exchange for not having to do it later). The
> original +25% was a bit too much, of course, but +5% seems reasonable.

Yes. It depends on how much the matview refresh gets slower but I
think the problem here is that users always are forced to pay the cost
for freezing tuple during refreshing the matview. There is no way to
disable it unlike FREEZE option of COPY command.

I’ve done benchmarks for matview refresh on my machine (FreeBSD 12.1,
AMD Ryzen 5 PRO 3400GE, 24GB RAM) with four codes: HEAD, HEAD +
Andres’s patch, one before 39b66a91b, and HEAD without
TABLE_INSERT_FROZEN.

The workload is to refresh the matview that simply selects 50M tuples
(about 1.7 GB). Here are the average execution times of three trials
for each code:

1) head: 42.263 sec
2) head w/ Andres’s patch: 40.194 sec
3) before 39b66a91b commit: 38.143 sec
4) head w/o freezing tuples: 32.413 sec

I also observed 5% degradation by comparing 1 and 2 but am not sure
where the overhead came from. I agree with Andres’s proposal. It’s a
straightforward approach. I think it’s a reasonable degradation
comparing to the cost of freezing tuples later. But I’m concerned a
bit that it’s reasonable that we force all users to pay the cost
during matview refresh without any choice. So we need to find the
remaining differences after applying a polished version of the patch.

FYI I’ve attached flame graphs for each evaluation. Looking at
1_head.svg, we can see CPU spent much time on visibilittmap_pin() and
it disappeared in 2_head_w_Andreas_patch.svg. There is no big
difference at a glance between 2_head_w_Andreas_patch.svg and
3_before_39b66a91b.svg.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Hi,

On 2021-05-18 11:20:07 +0900, Masahiko Sawada wrote:
> Yes. It depends on how much the matview refresh gets slower but I
> think the problem here is that users always are forced to pay the cost
> for freezing tuple during refreshing the matview. There is no way to
> disable it unlike FREEZE option of COPY command.
> 
> I’ve done benchmarks for matview refresh on my machine (FreeBSD 12.1,
> AMD Ryzen 5 PRO 3400GE, 24GB RAM) with four codes: HEAD, HEAD +
> Andres’s patch, one before 39b66a91b, and HEAD without
> TABLE_INSERT_FROZEN.
> 
> The workload is to refresh the matview that simply selects 50M tuples
> (about 1.7 GB). Here are the average execution times of three trials
> for each code:
> 
> 1) head: 42.263 sec
> 2) head w/ Andres’s patch: 40.194 sec
> 3) before 39b66a91b commit: 38.143 sec
> 4) head w/o freezing tuples: 32.413 sec

I don't see such a big difference between andres-freeze/non-freeze. Is
there any chance there's some noise in there? I found that I need to
disable autovacuum and ensure that there's a checkpoint just before the
REFRESH to get halfway meaningful numbers, as well as a min/max_wal_size
ensuring that only recycled WAL is used.


> I also observed 5% degradation by comparing 1 and 2 but am not sure
> where the overhead came from. I agree with Andres’s proposal. It’s a
> straightforward approach.

What degradation are you referencing here?


I compared your case 2 with 4 - as far as I can see the remaining
performance difference is from the the difference in WAL records
emitted:

freeze-andres:

Type                                           N      (%)          Record size      (%)             FPI size      (%)
    Combined size      (%)
 
----                                           -      ---          -----------      ---             --------      ---
    -------------      ---
 
XLOG/CHECKPOINT_ONLINE                         1 (  0.00)                  114 (  0.00)                    0 (  0.00)
              114 (  0.00)
 
Transaction/COMMIT                             1 (  0.00)                  949 (  0.00)                    0 (  0.00)
              949 (  0.00)
 
Storage/CREATE                                 1 (  0.00)                   42 (  0.00)                    0 (  0.00)
               42 (  0.00)
 
Standby/LOCK                                   3 (  0.00)                  138 (  0.00)                    0 (  0.00)
              138 (  0.00)
 
Standby/RUNNING_XACTS                          2 (  0.00)                  104 (  0.00)                    0 (  0.00)
              104 (  0.00)
 
Heap2/VISIBLE                              44248 (  0.44)              2610642 (  0.44)                16384 ( 14.44)
          2627026 (  0.44)
 
Heap2/MULTI_INSERT                             5 (  0.00)                 1125 (  0.00)                 6696 (  5.90)
             7821 (  0.00)
 
Heap/INSERT                              9955755 ( 99.12)            587389836 ( 99.12)                 5128 (  4.52)
        587394964 ( 99.10)
 
Heap/DELETE                                   13 (  0.00)                  702 (  0.00)                    0 (  0.00)
              702 (  0.00)
 
Heap/UPDATE                                    2 (  0.00)                  202 (  0.00)                    0 (  0.00)
              202 (  0.00)
 
Heap/HOT_UPDATE                                1 (  0.00)                   65 (  0.00)                 4372 (  3.85)
             4437 (  0.00)
 
Heap/INSERT+INIT                           44248 (  0.44)              2610632 (  0.44)                    0 (  0.00)
          2610632 (  0.44)
 
Btree/INSERT_LEAF                             33 (  0.00)                 2030 (  0.00)                80864 ( 71.28)
            82894 (  0.01)
 
                                        --------                      --------                      --------
         --------
 
Total                                   10044313                     592616581 [99.98%]               113444 [0.02%]
        592730025 [100%]
 

nofreeze:

Type                                           N      (%)          Record size      (%)             FPI size      (%)
    Combined size      (%)
 
----                                           -      ---          -----------      ---             --------      ---
    -------------      ---
 
XLOG/NEXTOID                                   1 (  0.00)                   30 (  0.00)                    0 (  0.00)
               30 (  0.00)
 
Transaction/COMMIT                             1 (  0.00)                  949 (  0.00)                    0 (  0.00)
              949 (  0.00)
 
Storage/CREATE                                 1 (  0.00)                   42 (  0.00)                    0 (  0.00)
               42 (  0.00)
 
Standby/LOCK                                   3 (  0.00)                  138 (  0.00)                    0 (  0.00)
              138 (  0.00)
 
Standby/RUNNING_XACTS                          1 (  0.00)                   54 (  0.00)                    0 (  0.00)
               54 (  0.00)
 
Heap2/MULTI_INSERT                             5 (  0.00)                 1125 (  0.00)                 7968 (  7.32)
             9093 (  0.00)
 
Heap/INSERT                              9955755 ( 99.56)            587389836 ( 99.56)                 5504 (  5.06)
        587395340 ( 99.54)
 
Heap/DELETE                                   13 (  0.00)                  702 (  0.00)                    0 (  0.00)
              702 (  0.00)
 
Heap/UPDATE                                    2 (  0.00)                  202 (  0.00)                    0 (  0.00)
              202 (  0.00)
 
Heap/HOT_UPDATE                                1 (  0.00)                   65 (  0.00)                 5076 (  4.67)
             5141 (  0.00)
 
Heap/INSERT+INIT                           44248 (  0.44)              2610632 (  0.44)                    0 (  0.00)
          2610632 (  0.44)
 
Btree/INSERT_LEAF                             32 (  0.00)                 1985 (  0.00)                73476 ( 67.54)
            75461 (  0.01)
 
Btree/INSERT_UPPER                             1 (  0.00)                   61 (  0.00)                 1172 (  1.08)
             1233 (  0.00)
 
Btree/SPLIT_L                                  1 (  0.00)                 1549 (  0.00)                 7480 (  6.88)
             9029 (  0.00)
 
Btree/DELETE                                   1 (  0.00)                   59 (  0.00)                 8108 (  7.45)
             8167 (  0.00)
 
Btree/REUSE_PAGE                               1 (  0.00)                   50 (  0.00)                    0 (  0.00)
               50 (  0.00)
 
                                        --------                      --------                      --------
         --------
 
Total                                   10000067                     590007479 [99.98%]               108784 [0.02%]
        590116263 [100%]
 

I.e. the additional Heap2/VISIBLE records show up.

It's not particularly surprising that emitting an additional WAL record
for every page isn't free. It's particularly grating / unnecessary
because this is the REGBUF_WILL_INIT path - it's completely unnecessary
to emit a separate record.

I dimly remember that we explicitly discussed that we do *not* want to
emit WAL records here?

Greetings,

Andres Freund

On 5/18/21 4:20 AM, Masahiko Sawada wrote:
 > ...
>>>>
>>>>> I think the changes for heap_multi_insert() are fine so we can revert
>>>>> only heap_insert() part if we revert something from the v14 tree,
>>>>> although we will end up not inserting frozen tuples into toast tables.
>>>>>
>>>>
>>>> I'd be somewhat unhappy about reverting just this bit, because it'd mean
>>>> that we freeze rows in the main table but not rows in the TOAST tables
>>>> (that was kinda why we concluded we need the heap_insert part too).
>>>>
>>>> I'm still a bit puzzled where does the extra overhead (in cases when
>>>> freeze is not requested) come from, TBH.
>>>
>>> Which cases do you mean? Doesn't matview refresh always request to
>>> freeze tuples even after applying the patch proposed on this thread?
>>>
>>
>> Oh, I didn't realize that! That'd make this much less of an issue, I'd
>> say, because if we're intentionally freezing the rows it's reasonable to
>> pay a bit of time (in exchange for not having to do it later). The
>> original +25% was a bit too much, of course, but +5% seems reasonable.
> 
> Yes. It depends on how much the matview refresh gets slower but I
> think the problem here is that users always are forced to pay the cost
> for freezing tuple during refreshing the matview. There is no way to
> disable it unlike FREEZE option of COPY command.
> 

Yeah, I see your point. I agree it's unfortunate there's no way to 
disable freezing during REFRESH MV. For most users that trade-off is 
probably fine, but for some cases (matviews refreshed often, or cases 
where it's fine to pay more but later) it may be an issue.

 From this POV, however, it may not be enough to optimize the current 
freezing code - it's always going to be a bit slower than before. So the 
only *real* solution may be adding a FREEZE option to the REFRESH 
MATERIALIZED VIEW command.


regards

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

On 5/18/21 8:08 PM, Andres Freund wrote:
> Hi,
> 
> On 2021-05-18 11:20:07 +0900, Masahiko Sawada wrote:
>> Yes. It depends on how much the matview refresh gets slower but I
>> think the problem here is that users always are forced to pay the cost
>> for freezing tuple during refreshing the matview. There is no way to
>> disable it unlike FREEZE option of COPY command.
>>
>> I’ve done benchmarks for matview refresh on my machine (FreeBSD 12.1,
>> AMD Ryzen 5 PRO 3400GE, 24GB RAM) with four codes: HEAD, HEAD +
>> Andres’s patch, one before 39b66a91b, and HEAD without
>> TABLE_INSERT_FROZEN.
>>
>> The workload is to refresh the matview that simply selects 50M tuples
>> (about 1.7 GB). Here are the average execution times of three trials
>> for each code:
>>
>> 1) head: 42.263 sec
>> 2) head w/ Andres’s patch: 40.194 sec
>> 3) before 39b66a91b commit: 38.143 sec
>> 4) head w/o freezing tuples: 32.413 sec
> 
> I don't see such a big difference between andres-freeze/non-freeze. Is
> there any chance there's some noise in there? I found that I need to
> disable autovacuum and ensure that there's a checkpoint just before the
> REFRESH to get halfway meaningful numbers, as well as a min/max_wal_size
> ensuring that only recycled WAL is used.
> 
> 
>> I also observed 5% degradation by comparing 1 and 2 but am not sure
>> where the overhead came from. I agree with Andres’s proposal. It’s a
>> straightforward approach.
> 
> What degradation are you referencing here?
> 
> 
> I compared your case 2 with 4 - as far as I can see the remaining
> performance difference is from the the difference in WAL records
> emitted:
> 
> freeze-andres:
> 
> Type                                           N      (%)          Record size      (%)             FPI size      (%)
      Combined size      (%)
 
> ----                                           -      ---          -----------      ---             --------      ---
      -------------      ---
 
> XLOG/CHECKPOINT_ONLINE                         1 (  0.00)                  114 (  0.00)                    0 (  0.00)
                114 (  0.00)
 
> Transaction/COMMIT                             1 (  0.00)                  949 (  0.00)                    0 (  0.00)
                949 (  0.00)
 
> Storage/CREATE                                 1 (  0.00)                   42 (  0.00)                    0 (  0.00)
                 42 (  0.00)
 
> Standby/LOCK                                   3 (  0.00)                  138 (  0.00)                    0 (  0.00)
                138 (  0.00)
 
> Standby/RUNNING_XACTS                          2 (  0.00)                  104 (  0.00)                    0 (  0.00)
                104 (  0.00)
 
> Heap2/VISIBLE                              44248 (  0.44)              2610642 (  0.44)                16384 ( 14.44)
            2627026 (  0.44)
 
> Heap2/MULTI_INSERT                             5 (  0.00)                 1125 (  0.00)                 6696 (  5.90)
               7821 (  0.00)
 
> Heap/INSERT                              9955755 ( 99.12)            587389836 ( 99.12)                 5128 (  4.52)
          587394964 ( 99.10)
 
> Heap/DELETE                                   13 (  0.00)                  702 (  0.00)                    0 (  0.00)
                702 (  0.00)
 
> Heap/UPDATE                                    2 (  0.00)                  202 (  0.00)                    0 (  0.00)
                202 (  0.00)
 
> Heap/HOT_UPDATE                                1 (  0.00)                   65 (  0.00)                 4372 (  3.85)
               4437 (  0.00)
 
> Heap/INSERT+INIT                           44248 (  0.44)              2610632 (  0.44)                    0 (  0.00)
            2610632 (  0.44)
 
> Btree/INSERT_LEAF                             33 (  0.00)                 2030 (  0.00)                80864 ( 71.28)
              82894 (  0.01)
 
>                                          --------                      --------                      --------
            --------
 
> Total                                   10044313                     592616581 [99.98%]               113444 [0.02%]
          592730025 [100%]
 
> 
> nofreeze:
> 
> Type                                           N      (%)          Record size      (%)             FPI size      (%)
      Combined size      (%)
 
> ----                                           -      ---          -----------      ---             --------      ---
      -------------      ---
 
> XLOG/NEXTOID                                   1 (  0.00)                   30 (  0.00)                    0 (  0.00)
                 30 (  0.00)
 
> Transaction/COMMIT                             1 (  0.00)                  949 (  0.00)                    0 (  0.00)
                949 (  0.00)
 
> Storage/CREATE                                 1 (  0.00)                   42 (  0.00)                    0 (  0.00)
                 42 (  0.00)
 
> Standby/LOCK                                   3 (  0.00)                  138 (  0.00)                    0 (  0.00)
                138 (  0.00)
 
> Standby/RUNNING_XACTS                          1 (  0.00)                   54 (  0.00)                    0 (  0.00)
                 54 (  0.00)
 
> Heap2/MULTI_INSERT                             5 (  0.00)                 1125 (  0.00)                 7968 (  7.32)
               9093 (  0.00)
 
> Heap/INSERT                              9955755 ( 99.56)            587389836 ( 99.56)                 5504 (  5.06)
          587395340 ( 99.54)
 
> Heap/DELETE                                   13 (  0.00)                  702 (  0.00)                    0 (  0.00)
                702 (  0.00)
 
> Heap/UPDATE                                    2 (  0.00)                  202 (  0.00)                    0 (  0.00)
                202 (  0.00)
 
> Heap/HOT_UPDATE                                1 (  0.00)                   65 (  0.00)                 5076 (  4.67)
               5141 (  0.00)
 
> Heap/INSERT+INIT                           44248 (  0.44)              2610632 (  0.44)                    0 (  0.00)
            2610632 (  0.44)
 
> Btree/INSERT_LEAF                             32 (  0.00)                 1985 (  0.00)                73476 ( 67.54)
              75461 (  0.01)
 
> Btree/INSERT_UPPER                             1 (  0.00)                   61 (  0.00)                 1172 (  1.08)
               1233 (  0.00)
 
> Btree/SPLIT_L                                  1 (  0.00)                 1549 (  0.00)                 7480 (  6.88)
               9029 (  0.00)
 
> Btree/DELETE                                   1 (  0.00)                   59 (  0.00)                 8108 (  7.45)
               8167 (  0.00)
 
> Btree/REUSE_PAGE                               1 (  0.00)                   50 (  0.00)                    0 (  0.00)
                 50 (  0.00)
 
>                                          --------                      --------                      --------
            --------
 
> Total                                   10000067                     590007479 [99.98%]               108784 [0.02%]
          590116263 [100%]
 
> 
> I.e. the additional Heap2/VISIBLE records show up.
> 
> It's not particularly surprising that emitting an additional WAL record
> for every page isn't free. It's particularly grating / unnecessary
> because this is the REGBUF_WILL_INIT path - it's completely unnecessary
> to emit a separate record.
> 

Yeah, emitting WAL is not exactly cheap, although it's just a little bit 
more (0.44%). I haven't looked into the details, but I wonder why it has 
such disproportionate impact (although, the 32 vs. 40 sec may be off).

> I dimly remember that we explicitly discussed that we do *not* want to
> emit WAL records here?
> 

Ummm, in which thread?


regards

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

Hi,

On 2021-05-18 20:34:08 +0200, Tomas Vondra wrote:
> Yeah, I see your point. I agree it's unfortunate there's no way to disable
> freezing during REFRESH MV. For most users that trade-off is probably fine,
> but for some cases (matviews refreshed often, or cases where it's fine to
> pay more but later) it may be an issue.
>
> From this POV, however, it may not be enough to optimize the current
> freezing code - it's always going to be a bit slower than before.

But the intrinsic overhead is *tiny*. Setting a few bits, with the other
costs amortized over a lot of pages. As far as I can tell the measurable
overhead is that the increased WAL logging - which is not necessary.

Greetings,

Andres Freund

Hi,

On 2021-05-18 20:43:41 +0200, Tomas Vondra wrote:
> Yeah, emitting WAL is not exactly cheap, although it's just a little bit
> more (0.44%). I haven't looked into the details, but I wonder why it has
> such disproportionate impact (although, the 32 vs. 40 sec may be off).

I couldn't reproduce this large a performance difference - I saw more
like 10% instead of 25%.


> > I dimly remember that we explicitly discussed that we do *not* want to
> > emit WAL records here?

> Ummm, in which thread?

https://postgr.es/m/20190408010427.4l63qr7h2fjcyp77%40alap3.anarazel.de

On 2019-04-07 18:04:27 -0700, Andres Freund wrote:
> This avoids an extra WAL record for setting empty pages to all visible,
> by adding XLH_INSERT_ALL_VISIBLE_SET & XLH_INSERT_ALL_FROZEN_SET, and
> setting those when appropriate in heap_multi_insert.  Unfortunately
> currently visibilitymap_set() doesn't really properly allow to do this,
> as it has embedded WAL logging for heap.
>
> I think we should remove the WAL logging from visibilitymap_set(), and
> move it to a separate, heap specific, function.

It'd probably be sufficient for the current purpose to change
visibilitymap_set()'s documentation to say that recptr can also be
passed in if the action is already covered by a WAL record, and that
it's the callers responsibility to think through the correctness
issues. Here it's easy, because any error will just throw the relation
away.

We do need to to include all-visible / FSM change in the WAL, so
crash-recovery / standbys end up with the same result as a primary
running normally. We already have the information, via
XLH_INSERT_ALL_FROZEN_SET. I think all we need to do is to add a
visibilitymap_set() in the redo routines if XLH_INSERT_ALL_FROZEN_SET.

Greetings,

Andres Freund

On Wed, May 19, 2021 at 3:08 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2021-05-18 11:20:07 +0900, Masahiko Sawada wrote:
> > Yes. It depends on how much the matview refresh gets slower but I
> > think the problem here is that users always are forced to pay the cost
> > for freezing tuple during refreshing the matview. There is no way to
> > disable it unlike FREEZE option of COPY command.
> >
> > I’ve done benchmarks for matview refresh on my machine (FreeBSD 12.1,
> > AMD Ryzen 5 PRO 3400GE, 24GB RAM) with four codes: HEAD, HEAD +
> > Andres’s patch, one before 39b66a91b, and HEAD without
> > TABLE_INSERT_FROZEN.
> >
> > The workload is to refresh the matview that simply selects 50M tuples
> > (about 1.7 GB). Here are the average execution times of three trials
> > for each code:
> >
> > 1) head: 42.263 sec
> > 2) head w/ Andres’s patch: 40.194 sec
> > 3) before 39b66a91b commit: 38.143 sec
> > 4) head w/o freezing tuples: 32.413 sec
>
> I don't see such a big difference between andres-freeze/non-freeze. Is
> there any chance there's some noise in there? I found that I need to
> disable autovacuum and ensure that there's a checkpoint just before the
> REFRESH to get halfway meaningful numbers, as well as a min/max_wal_size
> ensuring that only recycled WAL is used.

I've ran the same benchmarks with the following parameters:

shared_buffers = 10GB
max_wal_size = 50GB
min_wal_size = 50GB
checkpoint_timeout = 1h
maintenance_work_mem = 1GB
work_mem = 512MB
autovacuum = off

1) head: 42.397 sec
2) head w/ Andres’s patch: 34.857 sec
3) before 39b66a91b commit: 32.556 sec
4) head w/o freezing tuples: 32.752 sec

There is 6% degradation between 2 and 4 but 2 is much better than the
previous tests.

>
>
> > I also observed 5% degradation by comparing 1 and 2 but am not sure
> > where the overhead came from. I agree with Andres’s proposal. It’s a
> > straightforward approach.
>
> What degradation are you referencing here?

Sorry, I meant comparing 2 to 3 and 4.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On 5/11/21 7:35 PM, Tomas Vondra wrote:
> 
> 
> On 5/11/21 7:25 PM, Andres Freund wrote:
>> Hi,
>>
>> On 2021-05-11 16:07:44 +0200, Tomas Vondra wrote:
>>> On 5/11/21 11:04 AM, Masahiko Sawada wrote:
>>>> I think the changes for heap_multi_insert() are fine so we can revert
>>>> only heap_insert() part if we revert something from the v14 tree,
>>>> although we will end up not inserting frozen tuples into toast tables.
>>>>
>>>
>>> I'd be somewhat unhappy about reverting just this bit, because it'd mean
>>> that we freeze rows in the main table but not rows in the TOAST 
>>> tables (that
>>> was kinda why we concluded we need the heap_insert part too).
>>
>> Is there a reason not to apply a polished version of my proposal? And
>> then to look at the remaining difference?
>>
> 
> Probably not, I was just a little bit confused what exactly is going on, 
> unsure what to do about it. But if RMV freezes the rows, that probably 
> explains it and your patch is the way to go.
> 
>>
>>> I'm still a bit puzzled where does the extra overhead (in cases when 
>>> freeze
>>> is not requested) come from, TBH. Intuitively, I'd hope there's a way to
>>> eliminate that entirely, and only pay the cost when requested (with the
>>> expectation that it's cheaper than freezing it that later).
>>
>> I'd like to see a profile comparison between those two cases. Best with
>> both profiles done in master, just once with the freeze path disabled...
>>
> 
> OK. I'm mostly afk at the moment, I'll do that once I get back home, 
> sometime over the weekend / maybe early next week.
> 

OK, so here are the flamegraphs, for all three cases - current master, 
0c7d3bb99 (i.e. before heap_insert changes) and with the pinning patch 
applied. I did this using the same test case as before (50M table), but 
with -fno-omit-frame-pointer to get better profiles. It may add some 
overhead, but hopefully that applies to all cases equally.

The first 10 runs for each case look like this:

     old    master  patched
     ----------------------
     55045   74284    58246
     53927   74283    57273
     54090   74114    57336
     54194   74059    57223
     54189   74186    57287
     54090   74113    57278
     54095   74036    57176
     53896   74215    57303
     54101   74060    57524
     54062   74021    57278
     ----------------------
     54168   74137    57392
             1.36x    1.05x

which is mostly in line with previous findings (the master overhead is a 
bit worse, possibly due to the frame pointers).

Attached are the flame graphs for all three cases. The change in master 
is pretty clearly visible, but I don't see any clear difference between 
old and patched code :-(

regards

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

Hi,

On 2021-05-21 18:17:01 +0200, Tomas Vondra wrote:
> OK, so here are the flamegraphs, for all three cases - current master,
> 0c7d3bb99 (i.e. before heap_insert changes) and with the pinning patch
> applied. I did this using the same test case as before (50M table), but with
> -fno-omit-frame-pointer to get better profiles. It may add some overhead,
> but hopefully that applies to all cases equally.
> 
> The first 10 runs for each case look like this:
> 
>     old    master  patched
>     ----------------------
>     55045   74284    58246
>     53927   74283    57273
>     54090   74114    57336
>     54194   74059    57223
>     54189   74186    57287
>     54090   74113    57278
>     54095   74036    57176
>     53896   74215    57303
>     54101   74060    57524
>     54062   74021    57278
>     ----------------------
>     54168   74137    57392
>             1.36x    1.05x
> 
> which is mostly in line with previous findings (the master overhead is a bit
> worse, possibly due to the frame pointers).
> 
> Attached are the flame graphs for all three cases. The change in master is
> pretty clearly visible, but I don't see any clear difference between old and
> patched code :-(

I'm pretty sure it's the additional WAL records?

Greetings,

Andres Freund

On 5/21/21 6:43 PM, Andres Freund wrote:
> Hi,
>
 > ...
 >
>> Attached are the flame graphs for all three cases. The change in master is
>> pretty clearly visible, but I don't see any clear difference between old and
>> patched code :-(
> 
> I'm pretty sure it's the additional WAL records?
> 

Not sure. If I understand what you suggested elsewhere in the thread, it 
should be fine to modify heap_insert to pass the page recptr to 
visibilitymap_set, roughly per the attached patch.

I'm not sure it's correct, but it does eliminate the Heap2/VISIBILITY 
records for me (when applied on top of your patch). Funnily enough it 
does make it a wee bit slower:

patch #1: 56941.505
patch #2: 58099.788

I wonder if this might be due to -fno-omit-frame-pointer, though, as 
without it I get these timings:

0c7d3bb99: 25540.417
master:    31868.236
patch #1:  26566.199
patch #2:  26487.943

So without the frame pointers there's no slowdown, but there's no clear 
improvement after removal of the WAL records either :-(


regards

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

On Sat, May 22, 2021 at 3:10 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> On 5/21/21 6:43 PM, Andres Freund wrote:
> > Hi,
> >
>  > ...
>  >
> >> Attached are the flame graphs for all three cases. The change in master is
> >> pretty clearly visible, but I don't see any clear difference between old and
> >> patched code :-(
> >
> > I'm pretty sure it's the additional WAL records?
> >
>
> Not sure. If I understand what you suggested elsewhere in the thread, it
> should be fine to modify heap_insert to pass the page recptr to
> visibilitymap_set, roughly per the attached patch.
>
> I'm not sure it's correct, but it does eliminate the Heap2/VISIBILITY
> records for me (when applied on top of your patch). Funnily enough it
> does make it a wee bit slower:
>
> patch #1: 56941.505
> patch #2: 58099.788
>
> I wonder if this might be due to -fno-omit-frame-pointer, though, as
> without it I get these timings:
>
> 0c7d3bb99: 25540.417
> master:    31868.236
> patch #1:  26566.199
> patch #2:  26487.943
>
> So without the frame pointers there's no slowdown, but there's no clear
> improvement after removal of the WAL records either :-(

Can we verify that the additional WAL records are the cause of this
difference by making the matview unlogged by manually updating
relpersistence = 'u'?

Here are the results of benchmarks with unlogged matviews on my environment:

1) head: 22.927 sec
2) head w/ Andres’s patch: 16.629 sec
3) before 39b66a91b commit: 15.377 sec
4) head w/o freezing tuples: 14.551 sec

And here are the results of logged matviews ICYMI:

1) head: 42.397 sec
2) head w/ Andres’s patch: 34.857 sec
3) before 39b66a91b commit: 32.556 sec
4) head w/o freezing tuples: 32.752 sec

There seems no difference in the tendency. Which means the additional
WAL is not the culprit?

Interestingly, my previously proposed patch[1] was a better
performance. With the patch, we skip all VM-related work on all
insertions except for when inserting a tuple into a page for the first
time.

logged matviews: 31.591 sec
unlogged matviews: 15.317 sec

Regards,

[1] https://www.postgresql.org/message-id/CAD21AoAaiPcgGRyJ7vpg05%3DNWqr6Vhaay_SEXyZBboQcZC8sFA%40mail.gmail.com

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On 5/24/21 9:53 AM, Masahiko Sawada wrote:
> On Sat, May 22, 2021 at 3:10 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> On 5/21/21 6:43 PM, Andres Freund wrote:
>>> Hi,
>>>
>>   > ...
>>   >
>>>> Attached are the flame graphs for all three cases. The change in master is
>>>> pretty clearly visible, but I don't see any clear difference between old and
>>>> patched code :-(
>>>
>>> I'm pretty sure it's the additional WAL records?
>>>
>>
>> Not sure. If I understand what you suggested elsewhere in the thread, it
>> should be fine to modify heap_insert to pass the page recptr to
>> visibilitymap_set, roughly per the attached patch.
>>
>> I'm not sure it's correct, but it does eliminate the Heap2/VISIBILITY
>> records for me (when applied on top of your patch). Funnily enough it
>> does make it a wee bit slower:
>>
>> patch #1: 56941.505
>> patch #2: 58099.788
>>
>> I wonder if this might be due to -fno-omit-frame-pointer, though, as
>> without it I get these timings:
>>
>> 0c7d3bb99: 25540.417
>> master:    31868.236
>> patch #1:  26566.199
>> patch #2:  26487.943
>>
>> So without the frame pointers there's no slowdown, but there's no clear
>> improvement after removal of the WAL records either :-(
> 
> Can we verify that the additional WAL records are the cause of this
> difference by making the matview unlogged by manually updating
> relpersistence = 'u'?
> 
> Here are the results of benchmarks with unlogged matviews on my environment:
> 
> 1) head: 22.927 sec
> 2) head w/ Andres’s patch: 16.629 sec
> 3) before 39b66a91b commit: 15.377 sec
> 4) head w/o freezing tuples: 14.551 sec
> 
> And here are the results of logged matviews ICYMI:
> 
> 1) head: 42.397 sec
> 2) head w/ Andres’s patch: 34.857 sec
> 3) before 39b66a91b commit: 32.556 sec
> 4) head w/o freezing tuples: 32.752 sec
> 
> There seems no difference in the tendency. Which means the additional
> WAL is not the culprit?
> 

Yeah, I agree the WAL does not seem to be the culprit here.

The patch I posted skips the WAL logging entirely (verified by 
pg_waldump, although I have not mentioned that), and there's no clear 
improvement. (FWIW I'm not sure the patch is 100% correct, but it does 
eliminate the the extra WAL.)

The patch however does not skip the whole visibilitymap_set, it still 
does the initial error checks. I wonder if that might play a role ...

Another option might be changes in the binary layout - 5% change is well 
within the range that could be attributed to this, but it feels very 
hand-wavy and more like an excuse than real analysis.

> Interestingly, my previously proposed patch[1] was a better
> performance. With the patch, we skip all VM-related work on all
> insertions except for when inserting a tuple into a page for the first
> time.
> 
> logged matviews: 31.591 sec
> unlogged matviews: 15.317 sec
> 

Hmmm, thanks for reminding us that patch. Why did we reject that 
approach in favor of the current one?

I think at this point we have these two options:

1) Revert the freeze patches, either completely or just the heap_insert 
part, which is what seems to be causing issues. And try again in PG15, 
perhaps using a different approach, allow disabling freezing in refresh, 
or something like that.

2) Polish and commit the pinning patch from Andres, which does reduce 
the slowdown quite a bit. And either call it a day, or continue with the 
investigation / analysis regarding the remaining ~5% (but I personally 
have no idea what might be the problem ...).


I'd like to keep the improvement, but I find the 5% regression rather 
annoying and hard to defend, considering how much we fight for every 
little improvement.


regards

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

Hi,

On 2021-05-24 12:37:18 +0200, Tomas Vondra wrote:
> Another option might be changes in the binary layout - 5% change is well
> within the range that could be attributed to this, but it feels very
> hand-wavy and more like an excuse than real analysis.

I don't think 5% is likely to be explained by binary layout unless you
look for an explicitly adverse layout.


> Hmmm, thanks for reminding us that patch. Why did we reject that approach in
> favor of the current one?

Don't know about others, but I think it's way too fragile.

Greetings,

Andres Freund

On 5/24/21 8:21 PM, Andres Freund wrote:
> Hi,
> 
> On 2021-05-24 12:37:18 +0200, Tomas Vondra wrote:
>> Another option might be changes in the binary layout - 5% change is well
>> within the range that could be attributed to this, but it feels very
>> hand-wavy and more like an excuse than real analysis.
> 
> I don't think 5% is likely to be explained by binary layout unless you
> look for an explicitly adverse layout.
> 

Yeah, true. But I'm out of ideas what might be causing the regression
and how to fix it :-(

> 
>> Hmmm, thanks for reminding us that patch. Why did we reject that approach in
>> favor of the current one?
> 
> Don't know about others, but I think it's way too fragile.
> 

Is it really that fragile? Any particular risks you have in mind? Maybe
we could protect against that somehow ... Anyway, that change would
certainly be for PG15.


regards

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

Hi,

Based on the investigation and (lack of) progress so far, I'll revert
part of the COPY FREEZE improvements shortly. I'll keep the initial
7db0cd2145 changes, tweaking heap_multi_insert, and remove most of
39b66a91bd (except for the heap_xlog_multi_insert bit).

This should address the small 5% regression in refresh matview. I have
no other ideas how to fix that, short of adding a user-level option to
REFRESH MATERIALIZED VIEW command so that the users can opt out/in.

Attached is the revert patch - I'll get it committed in the next day or
two, once the tests complete (running with CCA so it takes time).

regards

On 5/25/21 12:30 AM, Tomas Vondra wrote:
> On 5/24/21 8:21 PM, Andres Freund wrote:
>> Hi,
>>
>> On 2021-05-24 12:37:18 +0200, Tomas Vondra wrote:
>>> Another option might be changes in the binary layout - 5% change is well
>>> within the range that could be attributed to this, but it feels very
>>> hand-wavy and more like an excuse than real analysis.
>>
>> I don't think 5% is likely to be explained by binary layout unless you
>> look for an explicitly adverse layout.
>>
> 
> Yeah, true. But I'm out of ideas what might be causing the regression
> and how to fix it :-(
> 
>>
>>> Hmmm, thanks for reminding us that patch. Why did we reject that approach in
>>> favor of the current one?
>>
>> Don't know about others, but I think it's way too fragile.
>>
> 
> Is it really that fragile? Any particular risks you have in mind? Maybe
> we could protect against that somehow ... Anyway, that change would
> certainly be for PG15.
> 
> 
> regards
> 

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

OK,

As mentioned in the previous message, I've reverted most of 39b66a91bd.
It took a bit longer to test, because the revert patch I shared a couple
days ago was actually incorrect/buggy in one place.

I'm not entirely happy about the end result (as it does not really help
with TOAST tables), so hopefully we'll be able to do something about
that soon. I'm not sure what, though - we've spent quite a bit of time
trying to address the regression, and I don't envision some major
breakthrough.

As for the regression example, I think in practice the impact would be
much lower, because the queries are likely much more complex (not just a
seqscan from a table), so the query execution will be a much bigger part
of execution time.

I do think the optimization would be a win in most cases where freezing
is desirable. From this POV the problem is rather that REFRESH MV does
not allow not freezing the result, so it has to pay the price always. So
perhaps the way forward is to add "NO FREEZE" option to REFRESH MV, or
something like that.


regards

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

On Thu, Jun 3, 2021 at 8:02 AM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> OK,
>
> As mentioned in the previous message, I've reverted most of 39b66a91bd.
> It took a bit longer to test, because the revert patch I shared a couple
> days ago was actually incorrect/buggy in one place.
>
> I'm not entirely happy about the end result (as it does not really help
> with TOAST tables), so hopefully we'll be able to do something about
> that soon.

Me too and +1 for addressing the problem soon for PG15.

> I'm not sure what, though - we've spent quite a bit of time
> trying to address the regression, and I don't envision some major
> breakthrough.
>
> As for the regression example, I think in practice the impact would be
> much lower, because the queries are likely much more complex (not just a
> seqscan from a table), so the query execution will be a much bigger part
> of execution time.
>
> I do think the optimization would be a win in most cases where freezing
> is desirable. From this POV the problem is rather that REFRESH MV does
> not allow not freezing the result, so it has to pay the price always. So
> perhaps the way forward is to add "NO FREEZE" option to REFRESH MV, or
> something like that.

That could be an option. Is it worth analyzing the cause of overhead
and why my patch seemed to avoid it? If we can resolve the performance
problem by fixing heap_insert() and related codes, we can use
HEAP_INSERT_FROZEN for CREATE TABLE AS as well.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> As mentioned in the previous message, I've reverted most of 39b66a91bd.

Should this topic be removed from the open-items list now?

            regards, tom lane

On 6/3/21 7:30 PM, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
>> As mentioned in the previous message, I've reverted most of 39b66a91bd.
> Should this topic be removed from the open-items list now?
>
>             



Yep.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com