parallel vacuum - few questions on docs, comments and code
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
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
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.
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
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.
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
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
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 executionThe 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
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 executionThe 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.
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.
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/
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
Attachments:
v1-0001-Parallel-Vacuum-Reword-Error-Messages-and-Docs.patchapplication/x-patch; name=v1-0001-Parallel-Vacuum-Reword-Error-Messages-and-Docs.patchDownload
From 514128927d51a5b0578e9b1ce969a68fe8d62b1c Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Thu, 13 May 2021 20:29:38 +0530
Subject: [PATCH v1] Parallel Vacuum-Reword Error Messages and Docs
---
doc/src/sgml/ref/vacuumdb.sgml | 2 +-
src/backend/commands/vacuum.c | 2 +-
src/bin/scripts/vacuumdb.c | 4 ++--
src/test/regress/expected/vacuum.out | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index 843a82e871..05249c4772 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -279,7 +279,7 @@ PostgreSQL documentation
<term><option>--parallel=<replaceable class="parameter">parallel_degree</replaceable></option></term>
<listitem>
<para>
- Specify the parallel degree of <firstterm>parallel vacuum</firstterm>.
+ Specify the number of parallel workers for <firstterm>parallel vacuum</firstterm>.
This allows the vacuum to leverage multiple CPUs to process indexes.
See <xref linkend="sql-vacuum"/>.
</para>
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index d549d0d86f..7421d7cfbd 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -165,7 +165,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
if (nworkers < 0 || nworkers > MAX_PARALLEL_WORKER_LIMIT)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("parallel vacuum degree must be between 0 and %d",
+ errmsg("parallel workers for vacuum must be between 0 and %d",
MAX_PARALLEL_WORKER_LIMIT),
parser_errposition(pstate, opt->location)));
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 7901c41f16..069a861aab 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -200,7 +200,7 @@ main(int argc, char *argv[])
vacopts.parallel_workers = atoi(optarg);
if (vacopts.parallel_workers < 0)
{
- 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);
}
break;
@@ -1000,7 +1000,7 @@ help(const char *progname)
printf(_(" --no-index-cleanup don't remove index entries that point to dead tuples\n"));
printf(_(" --no-process-toast skip the TOAST table associated with the table to vacuum\n"));
printf(_(" --no-truncate don't truncate empty pages at the end of the table\n"));
- 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"));
printf(_(" -q, --quiet don't write any messages\n"));
printf(_(" --skip-locked skip relations that cannot be immediately locked\n"));
printf(_(" -t, --table='TABLE[(COLUMNS)]' vacuum specific table(s) only\n"));
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 90cea6caa8..5e657849aa 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -110,7 +110,7 @@ VACUUM (PARALLEL 2) pvactst;
UPDATE pvactst SET i = i WHERE i < 1000;
VACUUM (PARALLEL 0) pvactst; -- disable parallel vacuum
VACUUM (PARALLEL -1) pvactst; -- error
-ERROR: parallel vacuum degree must be between 0 and 1024
+ERROR: parallel workers for vacuum must be between 0 and 1024
LINE 1: VACUUM (PARALLEL -1) pvactst;
^
VACUUM (PARALLEL 2, INDEX_CLEANUP FALSE) pvactst;
--
2.25.1
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]/messages/by-id/OS0PR01MB5716415335A06B489F1B3A8194569@OS0PR01MB5716.jpnprd01.prod.outlook.com, 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]: /messages/by-id/OS0PR01MB5716415335A06B489F1B3A8194569@OS0PR01MB5716.jpnprd01.prod.outlook.com
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
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] /messages/by-id/OS0PR01MB5716415335A06B489F1B3A8194569@OS0PR01MB5716.jpnprd01.prod.outlook.com
Yeah. Tom proposed if (foo <= 0) { error:"foo must be greater than
zero" } at [1]/messages/by-id/621822.1620655780@sss.pgh.pa.us. 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]: /messages/by-id/621822.1620655780@sss.pgh.pa.us
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
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] /messages/by-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.
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
Attachments:
v2-master-Parallel-Vacuum-Reword-Error-Messages-and-Docs.patchapplication/octet-stream; name=v2-master-Parallel-Vacuum-Reword-Error-Messages-and-Docs.patchDownload
From 46b56d11578e2c1ecaab7153e15011af4d598358 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Fri, 21 May 2021 13:15:45 +0530
Subject: [PATCH v2] Parallel Vacuum-Reword Error Messages and Docs - master
---
doc/src/sgml/ref/vacuumdb.sgml | 6 +++---
src/backend/commands/vacuum.c | 2 +-
src/bin/scripts/vacuumdb.c | 4 ++--
src/test/regress/expected/vacuum.out | 2 +-
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index 843a82e871..3e7f7ed68f 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -275,11 +275,11 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
- <term><option>-P <replaceable class="parameter">parallel_degree</replaceable></option></term>
- <term><option>--parallel=<replaceable class="parameter">parallel_degree</replaceable></option></term>
+ <term><option>-P <replaceable class="parameter">parallel_workers</replaceable></option></term>
+ <term><option>--parallel=<replaceable class="parameter">parallel_workers</replaceable></option></term>
<listitem>
<para>
- Specify the parallel degree of <firstterm>parallel vacuum</firstterm>.
+ Specify the number of parallel workers for <firstterm>parallel vacuum</firstterm>.
This allows the vacuum to leverage multiple CPUs to process indexes.
See <xref linkend="sql-vacuum"/>.
</para>
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index d549d0d86f..7421d7cfbd 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -165,7 +165,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
if (nworkers < 0 || nworkers > MAX_PARALLEL_WORKER_LIMIT)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("parallel vacuum degree must be between 0 and %d",
+ errmsg("parallel workers for vacuum must be between 0 and %d",
MAX_PARALLEL_WORKER_LIMIT),
parser_errposition(pstate, opt->location)));
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 7901c41f16..069a861aab 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -200,7 +200,7 @@ main(int argc, char *argv[])
vacopts.parallel_workers = atoi(optarg);
if (vacopts.parallel_workers < 0)
{
- 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);
}
break;
@@ -1000,7 +1000,7 @@ help(const char *progname)
printf(_(" --no-index-cleanup don't remove index entries that point to dead tuples\n"));
printf(_(" --no-process-toast skip the TOAST table associated with the table to vacuum\n"));
printf(_(" --no-truncate don't truncate empty pages at the end of the table\n"));
- 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"));
printf(_(" -q, --quiet don't write any messages\n"));
printf(_(" --skip-locked skip relations that cannot be immediately locked\n"));
printf(_(" -t, --table='TABLE[(COLUMNS)]' vacuum specific table(s) only\n"));
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 90cea6caa8..5e657849aa 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -110,7 +110,7 @@ VACUUM (PARALLEL 2) pvactst;
UPDATE pvactst SET i = i WHERE i < 1000;
VACUUM (PARALLEL 0) pvactst; -- disable parallel vacuum
VACUUM (PARALLEL -1) pvactst; -- error
-ERROR: parallel vacuum degree must be between 0 and 1024
+ERROR: parallel workers for vacuum must be between 0 and 1024
LINE 1: VACUUM (PARALLEL -1) pvactst;
^
VACUUM (PARALLEL 2, INDEX_CLEANUP FALSE) pvactst;
--
2.25.1
v2-V13-Parallel-Vacuum-Reword-Error-Messages-and-Docs.patchapplication/octet-stream; name=v2-V13-Parallel-Vacuum-Reword-Error-Messages-and-Docs.patchDownload
From a3d87e852837cdf706c0c97d0a10a3293b89beb7 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Fri, 21 May 2021 13:27:45 +0530
Subject: [PATCH v2] Parallel Vacuum-Reword Error Messages and Docs - V13
---
doc/src/sgml/ref/vacuumdb.sgml | 6 +++---
src/backend/commands/vacuum.c | 2 +-
src/bin/scripts/vacuumdb.c | 4 ++--
src/test/regress/expected/vacuum.out | 2 +-
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index eeafed7fbe..61c2e7d237 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -230,11 +230,11 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
- <term><option>-P <replaceable class="parameter">parallel_degree</replaceable></option></term>
- <term><option>--parallel=<replaceable class="parameter">parallel_degree</replaceable></option></term>
+ <term><option>-P <replaceable class="parameter">parallel_workers</replaceable></option></term>
+ <term><option>--parallel=<replaceable class="parameter">parallel_workers</replaceable></option></term>
<listitem>
<para>
- Specify the parallel degree of <firstterm>parallel vacuum</firstterm>.
+ Specify the number of parallel workers for <firstterm>parallel vacuum</firstterm>.
This allows the vacuum to leverage multiple CPUs to process indexes.
See <xref linkend="sql-vacuum"/>.
</para>
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 92c4eb6a87..5ef6698790 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -160,7 +160,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
if (nworkers < 0 || nworkers > MAX_PARALLEL_WORKER_LIMIT)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("parallel vacuum degree must be between 0 and %d",
+ errmsg("parallel workers for vacuum must be between 0 and %d",
MAX_PARALLEL_WORKER_LIMIT),
parser_errposition(pstate, opt->location)));
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 3e57fa9a86..1cf689cd05 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -189,7 +189,7 @@ main(int argc, char *argv[])
vacopts.parallel_workers = atoi(optarg);
if (vacopts.parallel_workers < 0)
{
- 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);
}
break;
@@ -920,7 +920,7 @@ help(const char *progname)
printf(_(" -j, --jobs=NUM use this many concurrent connections to vacuum\n"));
printf(_(" --min-mxid-age=MXID_AGE minimum multixact ID age of tables to vacuum\n"));
printf(_(" --min-xid-age=XID_AGE minimum transaction ID age of tables to vacuum\n"));
- 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"));
printf(_(" -q, --quiet don't write any messages\n"));
printf(_(" --skip-locked skip relations that cannot be immediately locked\n"));
printf(_(" -t, --table='TABLE[(COLUMNS)]' vacuum specific table(s) only\n"));
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 3fccb183c0..d026847c15 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -110,7 +110,7 @@ VACUUM (PARALLEL 2) pvactst;
UPDATE pvactst SET i = i WHERE i < 1000;
VACUUM (PARALLEL 0) pvactst; -- disable parallel vacuum
VACUUM (PARALLEL -1) pvactst; -- error
-ERROR: parallel vacuum degree must be between 0 and 1024
+ERROR: parallel workers for vacuum must be between 0 and 1024
LINE 1: VACUUM (PARALLEL -1) pvactst;
^
VACUUM (PARALLEL 2, INDEX_CLEANUP FALSE) pvactst;
--
2.25.1
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_degreeChanged.
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.
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_degreeChanged.
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.