Re: parallel vacuum comments

On Fri, Nov 5, 2021 at 4:42 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Wed, Nov 3, 2021 at 10:25 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > I've attached a draft patch. The patch incorporated all comments from
> > Andres except for the last comment that moves parallel related code to
> > another file. I'd like to discuss how we split vacuumlazy.c.
>
> This looks great!
>
> I wonder if this is okay, though:
>
> >     /* Process the indexes that can be processed by only leader process */
> > -   do_serial_processing_for_unsafe_indexes(vacrel, lps->lvshared);
> > +   lazy_serial_process_indexes(vacrel);
> >
> >     /*
> > -    * Join as a parallel worker.  The leader process alone processes all the
> > -    * indexes in the case where no workers are launched.
> > +    * Join as a parallel worker.  The leader process alone processes all
> > +    * parallel-safe indexes in the case where no workers are launched.
> >      */
> > -   do_parallel_processing(vacrel, lps->lvshared);
> > +   lazy_parallel_process_indexes(vacrel, lps->lvshared, vacrel->lps->lvsharedindstats);
> >
> >     /*
> >      * Next, accumulate buffer and WAL usage.  (This must wait for the workers
> > @@ -2786,6 +2734,18 @@ do_parallel_vacuum_or_cleanup(LVRelState *vacrel, int nworkers)
> >             InstrAccumParallelQuery(&lps->buffer_usage[i], &lps->wal_usage[i]);
> >     }
>
> Since "The leader process alone processes all parallel-safe indexes in
> the case where no workers are launched" (no change there), I wonder:
> how does the leader *know* that it's the leader (and so can always
> process any indexes) inside its call to
> lazy_parallel_process_indexes()? Or, does the leader actually process
> all indexes inside lazy_serial_process_indexes() in this edge case?
> (The edge case where a parallel VACUUM has no workers at all, because
> they couldn't be launched by the core parallel infrastructure.)

lazy_serial_process_indexes() handles only parallel-unsafe (i.g.,
non-parallel-supported or too small indexes) indexes whereas
lazy_parallel_process_indexes() does that only parallel-safe indexes.
Therefore in the edge case, the leader process all indexes by using
both functions.

>
> One small thing: the new "LVSharedIndStats.parallel_safe" field seems
> to be slightly misnamed. Isn't it more like
> "LVSharedIndStats.parallel_workers_can_process"? The index might
> actually be parallel safe *in principle*, while nevertheless being
> deliberately skipped over by workers due to a deliberate up-front
> choice made earlier, in compute_parallel_vacuum_workers(). Most
> obvious example of this is the choice to not use parallelism for a
> smaller index (say a partial index whose size is <
> min_parallel_index_scan_size).

Agreed.

>
> Another small thing, which is closely related to the last one:
>
> >  typedef struct LVSharedIndStats
> >  {
> > -   bool        updated;        /* are the stats updated? */
> > +   LVIndVacStatus status;
> > +
> > +   /*
> > +    * True if both leader and worker can process the index, otherwise only
> > +    * leader can process it.
> > +    */
> > +   bool    parallel_safe;
> > +
> > +   bool    istat_updated;      /* are the stats updated? */
> >     IndexBulkDeleteResult istat;
> >  } LVSharedIndStats;
>
> It would be nice to point out that the new
> "LVSharedIndStats.parallel_safe" field (or whatever we end up calling
> it) had comments that pointed out that it isn't a fixed thing for the
> entire VACUUM operation -- it's only fixed for an individual call to
> parallel_lazy_vacuum_or_cleanup_all_indexes() (i.e., it's only fixed
> for the "ambulkdelete portion" or the "amvacuumcleanup portion" of the
> entire VACUUM).

Agreed.

>
> Alternatively, you could just have two booleans, I think. You know,
> one for the "ambulkdelete portion", another for the "amvacuumcleanup
> portion". As I've said before, it would be nice if we only called
> parallel_vacuum_main() once per VACUUM operation (and not once per
> "portion"), but that's a harder and more invasive change. But I don't
> think you necessarily have to go that far for it to make sense to have
> two bools. Having two might allow you to make both of them immutable,
> which is useful.

If we want to make booleans immutable, we need three booleans since
parallel index cleanup behaves differently depending on whether
bulk-deletion has been called once. Anyway, if I understand your
suggestion correctly, it probably means to have booleans corresponding
to VACUUM_OPTION_PARALLEL_XXX flags. Does the worker itself need to
decide whether to skip conditionally-parallel-index-cleanup-safe
indexes?

>
> > Regarding tests, I’d like to add tests to check if a vacuum with
> > multiple index scans (i.g., due to small maintenance_work_mem) works
> > fine. But a problem is that we need at least about 200,000 garbage
> > tuples to perform index scan twice even with the minimum
> > maintenance_work_mem. Which takes a time to load tuples.
>
> Maybe that's okay. Do you notice that it takes a lot longer now? I did
> try to keep the runtime down when I committed the fixup to the
> parallel VACUUM related bug.

It took 6s on my laptop (was 400ms).

Regards,

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