Re: Performance degradation of REFRESH MATERIALIZED VIEW

On Tue, Apr 27, 2021 at 10:43 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>
>
>
> On 4/27/21 7:34 AM, Masahiko Sawada wrote:
> > On Tue, Apr 27, 2021 at 8:07 AM Andres Freund <andres(at)anarazel(dot)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/