Обсуждение: 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

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 like
>
>   value 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 like
> >
> >   value 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 like
> > >
> > >   value 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

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