From: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
---|---|
To: | Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: pg_stat_statements issue with parallel maintenance (Was Re: WAL usage calculation patch) |
Date: | 2020-03-31 06:50:44 |
Message-ID: | CAFiTN-u1q-QLUD0rQ3Fk0R08q9ryhjDJYFiLKyRS5kP1zdeCSQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Mar 31, 2020 at 10:44 AM Masahiko Sawada
<masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
>
> On Tue, 31 Mar 2020 at 12:58, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Mar 30, 2020 at 12:31 PM Masahiko Sawada
> > <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> > >
> > > The patch for vacuum conflicts with recent changes in vacuum. So I've
> > > attached rebased one.
> > >
> >
> > + /*
> > + * 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]);
> > +
> >
> > This should be done for launched workers aka
> > lps->pcxt->nworkers_launched. I think a similar problem exists in
> > create index related patch.
>
> You're right. Fixed in the new patches.
>
> On Mon, 30 Mar 2020 at 17:00, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> >
> > 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?
>
> We can do that but I was not sure if it's good since other codes
> around there don't use that. So I'd like to leave it for committers.
> It's a trivial change.
I have reviewed the patch and the patch looks fine to me.
One minor comment
/+ /* Points to buffer usage are in DSM */
+ BufferUsage *buffer_usage;
+
/buffer usage are in DSM / buffer usage area in DSM
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2020-03-31 06:51:44 | Re: INSERT ... OVERRIDING USER VALUE vs GENERATED ALWAYS identity columns |
Previous Message | Michael Paquier | 2020-03-31 06:48:21 | Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line |