Re: Performance degradation of REFRESH MATERIALIZED VIEW

On Mon, Apr 19, 2021 at 1:57 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Mon, Apr 19, 2021 at 5:04 PM Kyotaro Horiguchi
> <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> >
> > At Mon, 19 Apr 2021 13:32:31 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in
> > > On Fri, Apr 16, 2021 at 12:16 PM Kyotaro Horiguchi
> > > <horikyota(dot)ntt(at)gmail(dot)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