VACUUM (PARALLEL) option processing not using DefElem the way it was intended
I was just looking at the VACUUM option processing and I saw that the
code to process the PARALLEL option doesn't make use of the code in
defGetInt32() that's meant to handle empty and non-integer parameters.
ExecVacuum() has code to handle an empty PARALLEL parameters, but not
non-integer ones. That goes through to defGetInt32().
# vacuum (parallel 'bananas') pg_class;
ERROR: parallel requires an integer value
I feel if we're going to show that message for non-integer, then why
not the same one for empty parameter rather than handling that with a
custom message in the caller.
The attached is what I had in mind.
David
Attachments:
adjust_vacuum_parallel_option_handling_code.patchapplication/octet-stream; name=adjust_vacuum_parallel_option_handling_code.patchDownload
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 733ef40ae7c..027a64e41bf 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -266,35 +266,23 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
params.truncate = get_vacoptval_from_boolean(opt);
else if (strcmp(opt->defname, "parallel") == 0)
{
- if (opt->arg == NULL)
- {
+ int nworkers = defGetInt32(opt);
+
+ if (nworkers < 0 || nworkers > MAX_PARALLEL_WORKER_LIMIT)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("parallel option requires a value between 0 and %d",
+ errmsg("parallel workers for vacuum must be between 0 and %d",
MAX_PARALLEL_WORKER_LIMIT),
parser_errposition(pstate, opt->location)));
- }
- else
- {
- int nworkers;
-
- nworkers = defGetInt32(opt);
- if (nworkers < 0 || nworkers > MAX_PARALLEL_WORKER_LIMIT)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("parallel workers for vacuum must be between 0 and %d",
- MAX_PARALLEL_WORKER_LIMIT),
- parser_errposition(pstate, opt->location)));
- /*
- * Disable parallel vacuum, if user has specified parallel
- * degree as zero.
- */
- if (nworkers == 0)
- params.nworkers = -1;
- else
- params.nworkers = nworkers;
- }
+ /*
+ * Disable parallel vacuum, if user has specified parallel degree
+ * as zero.
+ */
+ if (nworkers == 0)
+ params.nworkers = -1;
+ else
+ params.nworkers = nworkers;
}
else if (strcmp(opt->defname, "skip_database_stats") == 0)
skip_database_stats = defGetBoolean(opt);
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 85c783e2e56..2dff5d623c1 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -168,9 +168,7 @@ VACUUM (PARALLEL 2, INDEX_CLEANUP FALSE) pvactst;
VACUUM (PARALLEL 2, FULL TRUE) pvactst; -- error, cannot use both PARALLEL and FULL
ERROR: VACUUM FULL cannot be performed in parallel
VACUUM (PARALLEL) pvactst; -- error, cannot use PARALLEL option without parallel degree
-ERROR: parallel option requires a value between 0 and 1024
-LINE 1: VACUUM (PARALLEL) pvactst;
- ^
+ERROR: parallel requires an integer value
-- Test parallel vacuum using the minimum maintenance_work_mem with and without
-- dead tuples.
SET maintenance_work_mem TO 64;
On 2025-Oct-08, David Rowley wrote:
I was just looking at the VACUUM option processing and I saw that the
code to process the PARALLEL option doesn't make use of the code in
defGetInt32() that's meant to handle empty and non-integer parameters.
ExecVacuum() has code to handle an empty PARALLEL parameters, but not
non-integer ones. That goes through to defGetInt32().
Yeah, that change makes sense to me. With an eye towards not forcing
the translators to understand the message context or forced to translate
the word "parallel", I would suggest to take the option name out of the
sentence, maybe something like
value for VACUUM option \"%s\" must be between 0 and %d
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On Thu, 9 Oct 2025 at 00:36, Álvaro Herrera <alvherre@kurilemu.de> wrote:
Yeah, that change makes sense to me. With an eye towards not forcing
the translators to understand the message context or forced to translate
the word "parallel", I would suggest to take the option name out of the
sentence, maybe something likevalue for VACUUM option \"%s\" must be between 0 and %d
Just looking at the other error messages. They all seems to put the
option in upper case but not in quotes. Following along with those,
we'd end up with:
PARALLEL option must be between 0 and %d
Would that be enough to help the translator understand not to
translate the option name?
David
On 2025-Oct-09, David Rowley wrote:
On Thu, 9 Oct 2025 at 00:36, Álvaro Herrera <alvherre@kurilemu.de> wrote:
Yeah, that change makes sense to me. With an eye towards not forcing
the translators to understand the message context or forced to translate
the word "parallel", I would suggest to take the option name out of the
sentence, maybe something likevalue for VACUUM option \"%s\" must be between 0 and %d
Just looking at the other error messages. They all seems to put the
option in upper case but not in quotes. Following along with those,
we'd end up with:PARALLEL option must be between 0 and %d
Would that be enough to help the translator understand not to
translate the option name?
This works for me, yeah. Though I'd still do "%s option must be between
0 and %d" (ie. make the option a separate string) and then they don't
need to understand that -- and also if there's another option elsewhere
whose value needs to be "between 0 and %d", then this string can be
reused. I don't see any such places right now though.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"All rings of power are equal,
But some rings of power are more equal than others."
(George Orwell's The Lord of the Rings)
On Wed, Oct 8, 2025 at 7:01 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Oct-09, David Rowley wrote:
On Thu, 9 Oct 2025 at 00:36, Álvaro Herrera <alvherre@kurilemu.de> wrote:
Yeah, that change makes sense to me. With an eye towards not forcing
the translators to understand the message context or forced to translate
the word "parallel", I would suggest to take the option name out of the
sentence, maybe something likevalue for VACUUM option \"%s\" must be between 0 and %d
Just looking at the other error messages. They all seems to put the
option in upper case but not in quotes. Following along with those,
we'd end up with:PARALLEL option must be between 0 and %d
Would that be enough to help the translator understand not to
translate the option name?This works for me, yeah. Though I'd still do "%s option must be between
0 and %d" (ie. make the option a separate string) and then they don't
need to understand that -- and also if there's another option elsewhere
whose value needs to be "between 0 and %d", then this string can be
reused.
+1 to using "%s" instead of hardcoding "PARALLEL" in the message.
I noticed we're currently hardcoding the "BUFFER_USAGE_LIMIT" option
name in the error message:
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("BUFFER_USAGE_LIMIT option must be 0 or between %d kB
and %d kB",
MIN_BAS_VAC_RING_SIZE_KB, MAX_BAS_VAC_RING_SIZE_KB),
hintmsg ? errhint("%s", _(hintmsg)) : 0));
Should we also change this for consistency with how we handle other
VACUUM options?
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On 2025-Oct-08, Masahiko Sawada wrote:
I noticed we're currently hardcoding the "BUFFER_USAGE_LIMIT" option
name in the error message:ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("BUFFER_USAGE_LIMIT option must be 0 or between %d kB
and %d kB",
MIN_BAS_VAC_RING_SIZE_KB, MAX_BAS_VAC_RING_SIZE_KB),
hintmsg ? errhint("%s", _(hintmsg)) : 0));Should we also change this for consistency with how we handle other
VACUUM options?
I would appreciate that, and also a change there from errhint() to
errhint_internal.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos. Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
On Thu, 9 Oct 2025 at 05:57, Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Oct-08, Masahiko Sawada wrote:
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("BUFFER_USAGE_LIMIT option must be 0 or between %d kB
and %d kB",
MIN_BAS_VAC_RING_SIZE_KB, MAX_BAS_VAC_RING_SIZE_KB),
hintmsg ? errhint("%s", _(hintmsg)) : 0));Should we also change this for consistency with how we handle other
VACUUM options?I would appreciate that, and also a change there from errhint() to
errhint_internal.
Ok, I've adjusted that in the attached.
David
Attachments:
adjust_vacuum_parallel_option_handling_code_v2.patchapplication/octet-stream; name=adjust_vacuum_parallel_option_handling_code_v2.patchDownload
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 733ef40ae7c..e2f181eed7b 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -220,9 +220,10 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
{
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("BUFFER_USAGE_LIMIT option must be 0 or between %d kB and %d kB",
+ errmsg("%s option must be 0 or between %d kB and %d kB",
+ "BUFFER_USAGE_LIMIT",
MIN_BAS_VAC_RING_SIZE_KB, MAX_BAS_VAC_RING_SIZE_KB),
- hintmsg ? errhint("%s", _(hintmsg)) : 0));
+ hintmsg ? errhint_internal("%s", _(hintmsg)) : 0));
}
ring_size = result;
@@ -266,35 +267,24 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
params.truncate = get_vacoptval_from_boolean(opt);
else if (strcmp(opt->defname, "parallel") == 0)
{
- if (opt->arg == NULL)
- {
+ int nworkers = defGetInt32(opt);
+
+ if (nworkers < 0 || nworkers > MAX_PARALLEL_WORKER_LIMIT)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("parallel option requires a value between 0 and %d",
+ errmsg("%s option must be between 0 and %d",
+ "PARALLEL",
MAX_PARALLEL_WORKER_LIMIT),
parser_errposition(pstate, opt->location)));
- }
- else
- {
- int nworkers;
-
- nworkers = defGetInt32(opt);
- if (nworkers < 0 || nworkers > MAX_PARALLEL_WORKER_LIMIT)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("parallel workers for vacuum must be between 0 and %d",
- MAX_PARALLEL_WORKER_LIMIT),
- parser_errposition(pstate, opt->location)));
- /*
- * Disable parallel vacuum, if user has specified parallel
- * degree as zero.
- */
- if (nworkers == 0)
- params.nworkers = -1;
- else
- params.nworkers = nworkers;
- }
+ /*
+ * Disable parallel vacuum, if user has specified parallel degree
+ * as zero.
+ */
+ if (nworkers == 0)
+ params.nworkers = -1;
+ else
+ params.nworkers = nworkers;
}
else if (strcmp(opt->defname, "skip_database_stats") == 0)
skip_database_stats = defGetBoolean(opt);
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 85c783e2e56..d4696bc3325 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -161,16 +161,14 @@ 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 workers for vacuum must be between 0 and 1024
+ERROR: PARALLEL option must be between 0 and 1024
LINE 1: VACUUM (PARALLEL -1) pvactst;
^
VACUUM (PARALLEL 2, INDEX_CLEANUP FALSE) pvactst;
VACUUM (PARALLEL 2, FULL TRUE) pvactst; -- error, cannot use both PARALLEL and FULL
ERROR: VACUUM FULL cannot be performed in parallel
VACUUM (PARALLEL) pvactst; -- error, cannot use PARALLEL option without parallel degree
-ERROR: parallel option requires a value between 0 and 1024
-LINE 1: VACUUM (PARALLEL) pvactst;
- ^
+ERROR: parallel requires an integer value
-- Test parallel vacuum using the minimum maintenance_work_mem with and without
-- dead tuples.
SET maintenance_work_mem TO 64;
On 2025-Oct-09, David Rowley wrote:
Ok, I've adjusted that in the attached.
LGTM, thanks.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Linux transformó mi computadora, de una `máquina para hacer cosas',
en un aparato realmente entretenido, sobre el cual cada día aprendo
algo nuevo" (Jaime Salinas)
On Thu, Oct 9, 2025 at 5:02 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2025-Oct-09, David Rowley wrote:
Ok, I've adjusted that in the attached.
LGTM, thanks.
LGTM too. Thank you!
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Fri, 10 Oct 2025 at 06:35, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Thu, Oct 9, 2025 at 5:02 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
LGTM, thanks.
LGTM too. Thank you!
Thank you to you both for looking. Pushed.
David