Re: parallel vacuum - few questions on docs, comments and code

On Wed, May 12, 2021 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, May 11, 2021 at 5:38 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > I was going through the parallel vacuum docs and code. I found below
> > things, please someone clarify:
> >
> > 1) I see that  a term "parallel degree" is used in the docs, code
> > comments, error messages "parallel vacuum degree must be a
> > non-negative integer", "parallel vacuum degree must be between 0 and
> > %d". Is there any specific reason to use the term "parallel degree"?
> > In the docs and code comments we generally use "parallel workers".
> >
>
> The parallel degree term is used here to indicate that we compute how
> much parallelism we can achieve based on the indexes.

Yeah, I get it. Even if users don't specify a parallel option there
are chances that parallelism is picked. So the parallel degree is the
final number of workers that are chosen by the server for vacuuming
indexes. And, I think that parallel degree is something internal to
the server, and it's better we replace it in the vacuumdb.sgml, change
PARALLEL_DEGREE to PARALLEL_WORKERS in vacuumdb.c and change the error
message "parallel vacuum degree must be a non-negative integer" to
"parallel workers for vacuum must be greater than or equal to zero".

Thoughts?

> > 2) The error messages "parallel vacuum degree must be between 0 and
> > %d" and "parallel option requires a value between 0 and %d" look
> > inconsistent.
> >
>
> I think we can make them consistent.

How about only one message "parallel option requires a value between 0
and %d" for both cases below? IMO they essentially mean the same
thing.

postgres=# vacuum (parallel ) t1;
ERROR:  parallel option requires a value between 0 and 1024
postgres=# vacuum (parallel -4) t1;
ERROR:  parallel vacuum degree must be between 0 and 1024

> > 5) Can the parallel_workers in below condition ever be negative in
> > begin_parallel_vacuum? I think we can just have if (parallel_workers
> > == 0).
> >     /* Can't perform vacuum in parallel */
> >     if (parallel_workers <= 0)
>
> Even if it can't go negative in the current code, I don't see a
> problem with the current code. It seems safe like this.

Okay.

> > 6) I think, instead of saying "using integer background workers", we
> > can just say "using specified or lesser number of background workers".
> > From the docs: Perform index vacuum and index cleanup phases of VACUUM
> > in parallel using integer background workers
> > We can say "workers specified will be used during execution"
> > From the docs: workers specified in integer will be used during execution
> >
> The docs here refer to "PARALLEL integer" specified in specs, so not
> sure if the proposed text is better.

IMO, "using the number of background workers specified with the
option" looks better than "using integer background workers".
Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com