Date: Mon, 30 Mar 2020 10:00:05 +0200
From: Julien Rouhaud <rjuju123@gmail.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
	Dilip Kumar <dilipbalaut@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL
 usage calculation patch)
Message-ID: <20200330080005.GC79261@nol>
References: <20200328133827.GA12854@nol>
 <20200328151721.GB12854@nol>
 <CAA4eK1Ke+LxUSWFdD=Q2=xRLvv0jC4i8EJYdfEYk9eM86yy-aw@mail.gmail.com>
 <CA+fd4k6XxNw8AMf0wHAwn9tmumZxShB2ue8pXYaVDc9fm1spyQ@mail.gmail.com>
 <CA+fd4k6DAAoCv+q_sGcHH2PPpN6AsHCS-LFYNJq9NaZtQHL2Ew@mail.gmail.com>
 <CAOBaU_Yc5OFKDYG_EOjvo3OGsP8DK_rHXj8bMQg_h6u0dc0zUw@mail.gmail.com>
 <CAA4eK1K1Znh7ZBP+Jgpue+hHraDi5=R7CD5dcm9Q9DL8b0vEyQ@mail.gmail.com>
 <CA+fd4k6YCYn9WT_BwbgxhmY8x7GLnks6cTeTutVBrdNW2K+Xbw@mail.gmail.com>
 <CA+fd4k6WS3h_y+OKAm5b=rYUzA3woRTEKp-_X6oqL71_Ac=iNQ@mail.gmail.com>
 <CA+fd4k5W+AXFFfAD=UucYzunxDOCY57qVH0qO_5dHhr7SXYecA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+fd4k5W+AXFFfAD=UucYzunxDOCY57qVH0qO_5dHhr7SXYecA@mail.gmail.com>
Precedence: bulk

On Mon, Mar 30, 2020 at 04:01:18PM +0900, Masahiko Sawada wrote:
> On Mon, 30 Mar 2020 at 15:46, Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > On Sun, 29 Mar 2020 at 20:44, Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> > >
> > > > > > I think we need to change parallel maintenance commands so that they
> > > > > > report buffer usage like what ParallelQueryMain() does; prepare to
> > > > > > track buffer usage during query execution by
> > > > > > InstrStartParallelQuery(), and report it by InstrEndParallelQuery()
> > > > > > after parallel maintenance command. To report buffer usage of parallel
> > > > > > maintenance command correctly, I'm thinking that we can (1) change
> > > > > > parallel create index and parallel vacuum so that they prepare
> > > > > > gathering buffer usage, or (2) have a common entry point for parallel
> > > > > > maintenance commands that is responsible for gathering buffer usage
> > > > > > and calling the entry functions for individual maintenance command.
> > > > > > I'll investigate it more in depth.
> > > > >
> > > [...]
> >
> > I've attached two patches fixing this issue for parallel index
> > creation and parallel vacuum. These approaches take the same approach;
> > we allocate DSM to share buffer usage and the leader gathers them,
> > described as approach (1) above. I think this is a straightforward
> > approach for this issue. We can create a common entry point for
> > parallel maintenance command that is responsible for gathering buffer
> > usage as well as sharing query text etc. But it will accompany
> > relatively big change and it might be overkill at this stage. We can
> > discuss that and it will become an item for PG14.
> >
> 
> The patch for vacuum conflicts with recent changes in vacuum. So I've
> attached rebased one.

Thanks Sawada-san!

Just minor nitpicking:

+   int         i;

    Assert(!IsParallelWorker());
    Assert(ParallelVacuumIsActive(lps));
@@ -2166,6 +2172,13 @@ lazy_parallel_vacuum_indexes(Relation *Irel, IndexBulkDeleteResult **stats,
    /* Wait for all vacuum workers to finish */
    WaitForParallelWorkersToFinish(lps->pcxt);

+   /*
+    * Next, accumulate buffer usage.  (This must wait for the workers to
+    * finish, or we might get incomplete data.)
+    */
+   for (i = 0; i < nworkers; i++)
+       InstrAccumParallelQuery(&lps->buffer_usage[i]);

We now allow declaring a variable in those loops, so it may be better to avoid
declaring i outside the for scope?

Other than that both patch looks good to me and a good fit for packpatching.  I
also did some testing on VACUUM and CREATE INDEX and it works as expected.