Horton

Thread

Commits

  1. Improve docs and error messages for parallel vacuum.

  1. parallel vacuum - few questions on docs, comments and code

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-11T12:07:50Z

    Hi,

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".
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.
3) Should the Assert(nindexes > 0); in begin_parallel_vacuum just be
Assert(nindexes > 1); as this function is entered only when indexes
are > 1?
4) IIUC, below comment says that even if PARALLEL 0 is specified with
VACUUM command, there are chances that the indexes are vacuumed in
parallel. Isn't it a bit unusual that a user specified 0 workers but
still the system is picking up parallelism? I'm sure this would have
been discussed, but I'm curious to know the reason.
 * nrequested is the number of parallel workers that user requested.  If
 * nrequested is 0, we compute the parallel degree based on nindexes, that is
 * the number of indexes that support parallel vacuum.
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)
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
7) I think we need a comma after "if any" .
>From the docs: which is limited by the number of workers specified
with PARALLEL option if any which is further limited by
8) Is it still true that if parallel workers are specified as 0 the
parallelism will not be picked up?
>From the docs: This feature is known as parallel vacuum. To disable
this feature, one can use PARALLEL option and specify parallel workers
as zero.

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

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

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-11T13:01:47Z

    On Tue, May 11, 2021 at 05:37:50PM +0530, Bharath Rupireddy wrote:
> 3) Should the Assert(nindexes > 0); in begin_parallel_vacuum just be
> Assert(nindexes > 1); as this function is entered only when indexes
> are > 1?

I think you're right, at least with the current implementation that
parallelization is done across indexes.  Same in parallel_vacuum_main.

> 4) IIUC, below comment says that even if PARALLEL 0 is specified with
> VACUUM command, there are chances that the indexes are vacuumed in
> parallel. Isn't it a bit unusual that a user specified 0 workers but
> still the system is picking up parallelism? I'm sure this would have
> been discussed, but I'm curious to know the reason.
>  * nrequested is the number of parallel workers that user requested.  If
>  * nrequested is 0, we compute the parallel degree based on nindexes, that is
>  * the number of indexes that support parallel vacuum.

No - nrequested is not actually the number of workers requested - it seems like
a poor choice of name.

This is the key part:

src/include/commands/vacuum.h
         * The number of parallel vacuum workers.  0 by default which means choose
         * based on the number of indexes.  -1 indicates parallel vacuum is
         * disabled.
         */
        int                     nworkers;
} VacuumParams;

The parsing code is in src/backend/commands/vacuum.c.

> 8) Is it still true that if parallel workers are specified as 0 the
> parallelism will not be picked up?
> From the docs: This feature is known as parallel vacuum. To disable
> this feature, one can use PARALLEL option and specify parallel workers
> as zero.

I think it's the same answer as above.

-- 
Justin

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

    Amit Kapila <amit.kapila16@gmail.com> — 2021-05-12T04:17:58Z

    On Tue, May 11, 2021 at 6:31 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Tue, May 11, 2021 at 05:37:50PM +0530, Bharath Rupireddy wrote:
> > 3) Should the Assert(nindexes > 0); in begin_parallel_vacuum just be
> > Assert(nindexes > 1); as this function is entered only when indexes
> > are > 1?
>
> I think you're right, at least with the current implementation that
> parallelization is done across indexes.  Same in parallel_vacuum_main.
>

Yeah, as code stands both of you are right. However, it can be helpful
to test parallelism even with one index say if we implement something
like force_parallel_mode = regress or parallel_leader_participation =
off.

-- 
With Regards,
Amit Kapila.

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

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-12T05:26:38Z

    On Tue, May 11, 2021 at 5:38 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> 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".

I think using "parallel workers" will be more consistent.

> 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.

+1

> 3) Should the Assert(nindexes > 0); in begin_parallel_vacuum just be
> Assert(nindexes > 1); as this function is entered only when indexes
> are > 1?
> 4) IIUC, below comment says that even if PARALLEL 0 is specified with
> VACUUM command, there are chances that the indexes are vacuumed in
> parallel. Isn't it a bit unusual that a user specified 0 workers but
> still the system is picking up parallelism? I'm sure this would have
> been discussed, but I'm curious to know the reason.
>  * nrequested is the number of parallel workers that user requested.  If
>  * nrequested is 0, we compute the parallel degree based on nindexes, that is
>  * the number of indexes that support parallel vacuum.
> 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)

Yes it should if (parallel_workers == 0)

> 8) Is it still true that if parallel workers are specified as 0 the
> parallelism will not be picked up?
> From the docs: This feature is known as parallel vacuum. To disable
> this feature, one can use PARALLEL option and specify parallel workers
> as zero.

Yes, by default this is enabled so for disabling user need to give
PARALLEL as 0.


--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

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

    Amit Kapila <amit.kapila16@gmail.com> — 2021-05-12T05:40:12Z

    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.

> 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.

> 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.

> 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.

-- 
With Regards,
Amit Kapila.

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-12T12:58:05Z

    On Tue, May 11, 2021 at 6:31 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > 4) IIUC, below comment says that even if PARALLEL 0 is specified with
> > VACUUM command, there are chances that the indexes are vacuumed in
> > parallel. Isn't it a bit unusual that a user specified 0 workers but
> > still the system is picking up parallelism? I'm sure this would have
> > been discussed, but I'm curious to know the reason.
> >  * nrequested is the number of parallel workers that user requested.  If
> >  * nrequested is 0, we compute the parallel degree based on nindexes, that is
> >  * the number of indexes that support parallel vacuum.
>
> No - nrequested is not actually the number of workers requested - it seems like
> a poor choice of name.
>
> This is the key part:
>
> src/include/commands/vacuum.h
>          * The number of parallel vacuum workers.  0 by default which means choose
>          * based on the number of indexes.  -1 indicates parallel vacuum is
>          * disabled.
>          */
>         int                     nworkers;
> } VacuumParams;

Thanks. The name "nworkers" looks fine to me after reading the comment
above it.  And the parallelism will be chosen by default.
    /* By default parallel vacuum is enabled */
    params.nworkers = 0;

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

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-12T13:00:17Z

    On Wed, May 12, 2021 at 9:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, May 11, 2021 at 6:31 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > On Tue, May 11, 2021 at 05:37:50PM +0530, Bharath Rupireddy wrote:
> > > 3) Should the Assert(nindexes > 0); in begin_parallel_vacuum just be
> > > Assert(nindexes > 1); as this function is entered only when indexes
> > > are > 1?
> >
> > I think you're right, at least with the current implementation that
> > parallelization is done across indexes.  Same in parallel_vacuum_main.
> >
>
> Yeah, as code stands both of you are right. However, it can be helpful
> to test parallelism even with one index say if we implement something
> like force_parallel_mode = regress or parallel_leader_participation =
> off.

I see that currently we don't have it yet. Is it worth implementing
them? Something like 1) when force_parallel_mode = regress, spawn one
parallel worker, send the relation information to it, so that it
performs vacuuming both the relation and it's indexes. 2)
parallel_leader_participation = off, spawn workers as specified, but
don't let the leader to vacuum index, so that any worker can pick it
up. I'm not sure of the complexity though.

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

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-12T13:07:30Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2021-05-13T06:25:05Z

    On Wed, May 12, 2021 at 6:37 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> 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.
>

I am fine with changing what you are proposing in the above two
points. Sawada-San, any thoughts?

>
> > > 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?
>

I am not too sure about this point. I guess we can leave it for now.


-- 
With Regards,
Amit Kapila.

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

    Amit Kapila <amit.kapila16@gmail.com> — 2021-05-13T06:28:29Z

    On Wed, May 12, 2021 at 6:30 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, May 12, 2021 at 9:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, May 11, 2021 at 6:31 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > On Tue, May 11, 2021 at 05:37:50PM +0530, Bharath Rupireddy wrote:
> > > > 3) Should the Assert(nindexes > 0); in begin_parallel_vacuum just be
> > > > Assert(nindexes > 1); as this function is entered only when indexes
> > > > are > 1?
> > >
> > > I think you're right, at least with the current implementation that
> > > parallelization is done across indexes.  Same in parallel_vacuum_main.
> > >
> >
> > Yeah, as code stands both of you are right. However, it can be helpful
> > to test parallelism even with one index say if we implement something
> > like force_parallel_mode = regress or parallel_leader_participation =
> > off.
>
> I see that currently we don't have it yet. Is it worth implementing
> them? Something like 1) when force_parallel_mode = regress, spawn one
> parallel worker, send the relation information to it, so that it
> performs vacuuming both the relation and it's indexes. 2)
> parallel_leader_participation = off, spawn workers as specified, but
> don't let the leader to vacuum index, so that any worker can pick it
> up. I'm not sure of the complexity though.
>

We had some patch on the above lines in the original development
thread which has been used for testing during development but not sure
how much useful it is now. However, I am fine if others think
something like that is useful.

-- 
With Regards,
Amit Kapila.

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

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-05-13T13:29:57Z

    On Thu, May 13, 2021 at 3:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, May 12, 2021 at 6:37 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > 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?

I'm fine with this change.

> >
> > > > 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.

The change looks good to me in terms of consistency but even the
current messages also make sense and are slightly clearer to me aside
from using the term "degree". If the user lacks an integer after
PARALLEL option, we say "parallel option requires a value between 0
and %d" and if the user specifies an invalid number to the option we
say "parallel vacuum degree must be between 0 and %d”. We use the
message something like “AAA must be between X and Y” also in other
places if users input an invalid value. I'm not sure the consistency
is important here but another idea to improve the error message would
be to change "parallel vacuum degree must be between 0 and %d” to "the
number of parallel workers must be between 0 and %d” (or using
“parallel workers for vacuum” instead of “the number of parallel
workers”) while leaving another message as it is.

Regards,

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

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-13T15:30:26Z

    On Thu, May 13, 2021 at 7:00 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > 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?
>
> I'm fine with this change.

Thanks.

> is important here but another idea to improve the error message would
> be to change "parallel vacuum degree must be between 0 and %d” to "the
> number of parallel workers must be between 0 and %d” (or using
> “parallel workers for vacuum” instead of “the number of parallel
> workers”) while leaving another message as it is.

Done that way.

PSA patch.

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

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

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-14T05:13:20Z

    On Thu, May 13, 2021 at 9:00 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Done that way.
>
> PSA patch.

Your changes look good.  About changing the "non-negative integer" to
"greater than or equal to zero", there is another thread [1], I am not
sure that have we concluded anything there yet.

- pg_log_error("parallel vacuum degree must be a non-negative integer");
+ pg_log_error("parallel workers for vacuum must be greater than or
equal to zero");
  exit(1);

[1] https://www.postgresql.org/message-id/OS0PR01MB5716415335A06B489F1B3A8194569@OS0PR01MB5716.jpnprd01.prod.outlook.com

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-14T12:30:04Z

    On Fri, May 14, 2021 at 10:43 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, May 13, 2021 at 9:00 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > Done that way.
> >
> > PSA patch.
>
> Your changes look good.  About changing the "non-negative integer" to
> "greater than or equal to zero", there is another thread [1], I am not
> sure that have we concluded anything there yet.
>
> - pg_log_error("parallel vacuum degree must be a non-negative integer");
> + pg_log_error("parallel workers for vacuum must be greater than or
> equal to zero");
>   exit(1);
>
> [1] https://www.postgresql.org/message-id/OS0PR01MB5716415335A06B489F1B3A8194569@OS0PR01MB5716.jpnprd01.prod.outlook.com

Yeah. Tom proposed if (foo <= 0) { error:"foo must be greater than
zero" } at [1]. In the subsequent messages both Michael and I agreed
with that. But we also have cases like  if (foo < 0) for which I think
{ error:"foo must be greater than or equal to zero" } would be better,
similar to what's proposed. Please feel free to provide your thoughts
there in that thread.

[1] - https://www.postgresql.org/message-id/621822.1620655780%40sss.pgh.pa.us

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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2021-05-21T05:40:15Z

    On Fri, May 14, 2021 at 6:00 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, May 14, 2021 at 10:43 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> >
> > Your changes look good.  About changing the "non-negative integer" to
> > "greater than or equal to zero", there is another thread [1], I am not
> > sure that have we concluded anything there yet.
> >
> > - pg_log_error("parallel vacuum degree must be a non-negative integer");
> > + pg_log_error("parallel workers for vacuum must be greater than or
> > equal to zero");
> >   exit(1);
> >
> > [1] https://www.postgresql.org/message-id/OS0PR01MB5716415335A06B489F1B3A8194569@OS0PR01MB5716.jpnprd01.prod.outlook.com
>
> Yeah. Tom proposed if (foo <= 0) { error:"foo must be greater than
> zero" } at [1]. In the subsequent messages both Michael and I agreed
> with that. But we also have cases like  if (foo < 0) for which I think
> { error:"foo must be greater than or equal to zero" } would be better,
> similar to what's proposed. Please feel free to provide your thoughts
> there in that thread.
>

I responded on that thread and it seems there is no object to the new
message. I have a minor comment on your patch:

- printf(_("  -P, --parallel=PARALLEL_DEGREE  use this many background
workers for vacuum, if available\n"));
+ printf(_("  -P, --parallel=PARALLEL_WORKERS use this many background
workers for vacuum, if available\n"));

If the patch changes the vacuumdb code as above then isn't it better
to change the vacuumdb docs to reflect the same. See below part of
vacuumdb docs:
-P parallel_degree
--parallel=parallel_degree

Also, can you please check if your patch works for PG-13 as well
because I think it is better to backpatch it?

-- 
With Regards,
Amit Kapila.

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-21T08:00:43Z

    On Fri, May 21, 2021 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> I responded on that thread and it seems there is no object to the new
> message. I have a minor comment on your patch:

Thanks Amit!

> - printf(_("  -P, --parallel=PARALLEL_DEGREE  use this many background
> workers for vacuum, if available\n"));
> + printf(_("  -P, --parallel=PARALLEL_WORKERS use this many background
> workers for vacuum, if available\n"));
>
> If the patch changes the vacuumdb code as above then isn't it better
> to change the vacuumdb docs to reflect the same. See below part of
> vacuumdb docs:
> -P parallel_degree
> --parallel=parallel_degree

Changed.

> Also, can you please check if your patch works for PG-13 as well
> because I think it is better to backpatch it?

I'm not sure about backpatching as it is not a critical bug fix. Since
the changes are user visible, I think that it's okay to backpatch.

Anyways, attaching patches for both master and v13 branch. Please
review it further.

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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2021-05-21T10:03:56Z

    On Fri, May 21, 2021 at 1:30 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, May 21, 2021 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > If the patch changes the vacuumdb code as above then isn't it better
> > to change the vacuumdb docs to reflect the same. See below part of
> > vacuumdb docs:
> > -P parallel_degree
> > --parallel=parallel_degree
>
> Changed.
>
> > Also, can you please check if your patch works for PG-13 as well
> > because I think it is better to backpatch it?
>
> I'm not sure about backpatching as it is not a critical bug fix. Since
> the changes are user visible, I think that it's okay to backpatch.
>

Yes, as it is a user-visible change (though minor) so I thought it
would be good to backpatch this. Does anyone else have any opinion on
this?

-- 
With Regards,
Amit Kapila.

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

    Amit Kapila <amit.kapila16@gmail.com> — 2021-05-25T09:29:56Z

    On Fri, May 21, 2021 at 3:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, May 21, 2021 at 1:30 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Fri, May 21, 2021 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > If the patch changes the vacuumdb code as above then isn't it better
> > > to change the vacuumdb docs to reflect the same. See below part of
> > > vacuumdb docs:
> > > -P parallel_degree
> > > --parallel=parallel_degree
> >
> > Changed.
> >
> > > Also, can you please check if your patch works for PG-13 as well
> > > because I think it is better to backpatch it?
> >
> > I'm not sure about backpatching as it is not a critical bug fix. Since
> > the changes are user visible, I think that it's okay to backpatch.
> >
>
> Yes, as it is a user-visible change (though minor) so I thought it
> would be good to backpatch this. Does anyone else have any opinion on
> this?
>

Pushed!

-- 
With Regards,
Amit Kapila.