vacuum_truncate configuration parameter and isset_offset

В письме от понедельник, 24 марта 2025 г. 15:27:35 MSK пользователь Nikolay
Shaplov написал:

PS. I've just looked at code for vacuum_index_cleanup it has very same
logic, just replace "auto" with anything you like, and uses enum.
Grep for StdRdOptIndexCleanupValues for more info

I thought about it a bit more...

ALTER TABLE test SET (vacuum_truncate);
ALTER TABLE test SET (vacuum_truncate=false);
ALTER TABLE test RESET (vacuum_truncate);

Nobody will ever guess that these are three different cases.

since for enum solution
ALTER TABLE test SET (vacuum_truncate=on);
ALTER TABLE test SET (vacuum_truncate=off);
ALTER TABLE test SET (vacuum_truncate=unset);
ALTER TABLE test RESET (vacuum_truncate);

it is quite clear that first three are different, and 4th is most probably is
equivalent for 3rd.

So, my suggestion is to change it as soon as possible.

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

On Mon, Mar 24, 2025 at 03:27:35PM +0300, Nikolay Shaplov wrote:

I would suggest to use enum here to achieve same goal, or add some "trilean"
data type that can be "true/false/unset"

I did first try making the vacuum_truncate reloption an enum, but I didn't
proceed with that approach for a couple of reasons:

* vacuum_truncate is already a Boolean reloption, so switching it to an
enum would require enumerating all possible values accepted by
parse_bool(), which has the following comment:

/*
* Try to interpret value as boolean value. Valid values are: true,
* false, yes, no, on, off, 1, 0; as well as unique prefixes thereof.
* If the string parses okay, return true, else false.
* If okay and result is not NULL, return the value in *result.
*/

I also considered adding some sort of pluggable parsing functionality or
something like StdRdOptIndexCleanupValues (i.e., a subset of the most
popular values accepted by parse_bool()), but those seemed a little too
complicated for the problem at hand.

* I wasn't sure whether it would be better to expose this third "unset"
value or to make it hidden and for internal use only. The former would
mean we have two ways to declare a reloption "unset." You could either
not set it, or you could explicitly set it to "unset." The latter would
require additional code somewhere, or we could just leave it
undocumented. Both of these approaches seemed like weird user
experiences to me.

Overall, the biggest reason I didn't proceed with the enum is because it
felt like it was making it the user's problem. Rather than just teaching
our code how to determine if a reloption was explicitly set, we'd be
introducing unnecessary complexity to the user, or at least we'd be trying
to closely match the existing functionality in an attempt to hide this
complexity from them.

--
nathan

On Mon, Mar 24, 2025 at 10:43 AM Nathan Bossart
<nathandbossart@gmail.com> wrote:

Overall, the biggest reason I didn't proceed with the enum is because it
felt like it was making it the user's problem. Rather than just teaching
our code how to determine if a reloption was explicitly set, we'd be
introducing unnecessary complexity to the user, or at least we'd be trying
to closely match the existing functionality in an attempt to hide this
complexity from them.

+1. Giving the user the ability to set the option to a value called
"unset" doesn't seem right to me.

--
Robert Haas
EDB: http://www.enterprisedb.com

В письме от понедельник, 24 марта 2025 г. 17:43:38 MSK пользователь Nathan
Bossart написал:

* vacuum_truncate is already a Boolean reloption,

Yes, I guess schema update would be best solution here.

so switching it to an
enum would require enumerating all possible values accepted by
parse_bool(), which has the following comment:

/*
* Try to interpret value as boolean value. Valid values are: true,
* false, yes, no, on, off, 1, 0; as well as unique prefixes thereof.
* If the string parses okay, return true, else false.
* If okay and result is not NULL, return the value in *result.
*/

We already do it for some option. May be we need more generic solution, but
boolean should remain boolean, with only two possible values.

* I wasn't sure whether it would be better to expose this third "unset"
value or to make it hidden and for internal use only.

If you hide something from user, you will confuse him. If the solution is
more explicit, then it is more easy to use it, and understand it. If something
is implying (like deleted option means third value), then there would be
confusion. Please do not do that.

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

В письме от понедельник, 24 марта 2025 г. 17:48:25 MSK пользователь Robert
Haas написал:

On Mon, Mar 24, 2025 at 10:43 AM Nathan Bossart

<nathandbossart@gmail.com> wrote:

Overall, the biggest reason I didn't proceed with the enum is because it
felt like it was making it the user's problem. Rather than just teaching
our code how to determine if a reloption was explicitly set, we'd be
introducing unnecessary complexity to the user, or at least we'd be trying
to closely match the existing functionality in an attempt to hide this
complexity from them.

+1. Giving the user the ability to set the option to a value called
"unset" doesn't seem right to me.

1. Enum allows you to make default value "unreachable" for setting. But this
is not the most important thing.

2. This option has three possible values: on/off/system_default. We can find
better name for system_default, but it would not change it's meaning.
I would prefer to let user set these three values explicitly.

Nobody would guess that

ALTER TABLE test SET (vacuum_truncate=false);
means "off"

and
ALTER TABLE test RESET (vacuum_truncate);
means "system_default"

This will lead to a lot of confusion.

So I strongly against this implying third value for boolean. This is nasty
thing. Boolean should have two values.

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

On Mon, Mar 24, 2025 at 11:12 AM Nikolay Shaplov <dhyan@nataraj.su> wrote:

Nobody would guess that

ALTER TABLE test SET (vacuum_truncate=false);
means "off"

and
ALTER TABLE test RESET (vacuum_truncate);
means "system_default"

This will lead to a lot of confusion.

I agree that this confuses people, but I don't think it's more
confusing here than for other vacuum reloptions. I have seen people
try to unset vacuum reloptions by using SET to configure them to the
default value rather than by using RESET to remove them. But then
later they change the system default and that table is still nailed to
the old default. I always find myself slightly bemused by this,
because it doesn't seem that hard to me to figure out how it actually
works, but it's definitely a real issue. However, I don't see why the
issue is any more acute for this parameter than any other, and it
certainly does not seem like a good idea to make this parameter work
differently from the other ones.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Mar 24, 2025 at 8:26 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Mar 24, 2025 at 11:12 AM Nikolay Shaplov <dhyan@nataraj.su> wrote:

Nobody would guess that

ALTER TABLE test SET (vacuum_truncate=false);
means "off"

and
ALTER TABLE test RESET (vacuum_truncate);
means "system_default"

This will lead to a lot of confusion.

I agree that this confuses people, but I don't think it's more
confusing here than for other vacuum reloptions. I have seen people
try to unset vacuum reloptions by using SET to configure them to the
default value rather than by using RESET to remove them. But then
later they change the system default and that table is still nailed to
the old default. I always find myself slightly bemused by this,
because it doesn't seem that hard to me to figure out how it actually
works, but it's definitely a real issue. However, I don't see why the
issue is any more acute for this parameter than any other, and it
certainly does not seem like a good idea to make this parameter work
differently from the other ones.

+1

David J.

В письме от понедельник, 24 марта 2025 г. 18:25:44 MSK пользователь Robert
Haas написал:

and
ALTER TABLE test RESET (vacuum_truncate);
means "system_default"

This will lead to a lot of confusion.

I agree that this confuses people,

Though a bit more, I think that as a user I would prefer to be able to
explicitly set "system_defaults" value. If I have not set any, most probably I
do not care, or have not thought about it. But if I set option to
"system_defaults" then I do it intentionally and have some good reasons for
it.

but I don't think it's more
confusing here than for other vacuum reloptions. I have seen people
try to unset vacuum reloptions by using SET to configure them to the
default value rather than by using RESET to remove them. But then
later they change the system default and that table is still nailed to
the old default. I always find myself slightly bemused by this,
because it doesn't seem that hard to me to figure out how it actually
works, but it's definitely a real issue. However, I don't see why the
issue is any more acute for this parameter than any other, and it
certainly does not seem like a good idea to make this parameter work
differently from the other ones.

May be I can agree with you. But this does not explain why we should switch
boolean from two possible values into three... It can't have three values for
any practical reason. This should not be boolean anymore in this case.

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

On Mon, Mar 24, 2025 at 8:45 AM Nikolay Shaplov <dhyan@nataraj.su> wrote:

but I don't think it's more
confusing here than for other vacuum reloptions. I have seen people
try to unset vacuum reloptions by using SET to configure them to the
default value rather than by using RESET to remove them. But then
later they change the system default and that table is still nailed to
the old default. I always find myself slightly bemused by this,
because it doesn't seem that hard to me to figure out how it actually
works, but it's definitely a real issue. However, I don't see why the
issue is any more acute for this parameter than any other, and it
certainly does not seem like a good idea to make this parameter work
differently from the other ones.

May be I can agree with you. But this does not explain why we should
switch
boolean from two possible values into three... It can't have three values
for
any practical reason. This should not be boolean anymore in this case.

The boolean is two-valued. The state of the reloption is also binary - set
or unset (i.e., it is listed in pg_class.reloptions, or not). In the unset
state the effective value of a reloption comes from the corresponding GUC
(or, lacking that, some hard-coded default). If set, the value comes from
the associated boolean. RESET places a reloption into the unset state.

David J.

Hello

I don't understand why this shouldn't work exactly like
vacuum_index_cleanup (cf. vacuum_rel lines 2170ff). That would require
no new mechanism.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Java is clearly an example of money oriented programming" (A. Stepanov)

В письме от понедельник, 24 марта 2025 г. 19:00:51 MSK пользователь Álvaro
Herrera написал:

I don't understand why this shouldn't work exactly like
vacuum_index_cleanup (cf. vacuum_rel lines 2170ff). That would require
no new mechanism.

In my opinion it should.

"no new mechanism" is good. "new mechanism" is bad. :-)

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

On Mon, Mar 24, 2025 at 9:00 AM Álvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

Hello

I don't understand why this shouldn't work exactly like
vacuum_index_cleanup (cf. vacuum_rel lines 2170ff). That would require
no new mechanism.

That reloption is already an enum and there is no GUC to defer to when the
value is unset. It doesn't seem like an equivalent scenario. AUTO is a
perfectly useful value as opposed to an undocumented sentinel for
unset/missing.

David J.

On Mon, Mar 24, 2025 at 9:08 AM David G. Johnston <
david.g.johnston@gmail.com> wrote:

On Mon, Mar 24, 2025 at 9:00 AM Álvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

Hello

I don't understand why this shouldn't work exactly like
vacuum_index_cleanup (cf. vacuum_rel lines 2170ff). That would require
no new mechanism.

That reloption is already an enum and there is no GUC to defer to when the
value is unset. It doesn't seem like an equivalent scenario. AUTO is a
perfectly useful value as opposed to an undocumented sentinel for
unset/missing.

Sorry, the "already an enum" comment is wrong - I see the commit now where
we basically re-implemented boolean value processing logic and added an
"auto" option.

Basically we'd do this to make a boolean-compatible enum adding an
undocumented value "null" as a valid and default set value and then
interpret "null" as meaning "go use the vacuum_truncate GUC".

It's too late to argue against sentinel values so I suppose this would have
to be acceptable.

David J.

On Mon, Mar 24, 2025 at 5:42 AM Nikolay Shaplov <dhyan@nataraj.su> wrote:

В письме от понедельник, 24 марта 2025 г. 15:27:35 MSK пользователь
Nikolay
Shaplov написал:

PS. I've just looked at code for vacuum_index_cleanup it has very same
logic, just replace "auto" with anything you like, and uses enum.
Grep for StdRdOptIndexCleanupValues for more info

I thought about it a bit more...

ALTER TABLE test SET (vacuum_truncate);
ALTER TABLE test SET (vacuum_truncate=false);
ALTER TABLE test RESET (vacuum_truncate);

Nobody will ever guess that these are three different cases.

since for enum solution
ALTER TABLE test SET (vacuum_truncate=on);
ALTER TABLE test SET (vacuum_truncate=off);
ALTER TABLE test SET (vacuum_truncate=unset);
ALTER TABLE test RESET (vacuum_truncate);

Actually, we'd want to prohibit vacuum_truncate=unset (or whatever internal
enum option value we choose) as input since none of the other reloptions
allow the user to specify the sentinel value but instead require the use of
reset to achieve it.

i.e., the reset value for parallel_workers is -1:

{
"parallel_workers",
"Number of parallel processes that can be used per executor node for this
relation.",
RELOPT_KIND_HEAP,
ShareUpdateExclusiveLock
},
-1, 0, 1024

but:

alter table vct set (parallel_workers=-1);
ERROR: value -1 out of bounds for option "parallel_workers"
DETAIL: Valid values are between "0" and "1024".

So, given the precedent of vacuum_index_cleanup and the above, we should
turn this into an enum that accepts all existing boolean literal inputs and
also has a undocumented "unset" default value that the user is not allowed
to explicitly set but instead only gets used to resolve an unset reloption
at runtime.

David J.

On Mon, Mar 24, 2025 at 05:00:51PM +0100, �lvaro Herrera wrote:

I don't understand why this shouldn't work exactly like
vacuum_index_cleanup (cf. vacuum_rel lines 2170ff). That would require
no new mechanism.

I explained my reasons for not proceeding with that approach upthread [0]/messages/by-id/Z-Fvmnf57z6zM8Ky@nathan.

I don't think there are any existing reloptions where there are two ways to
say "use the GUC." For ones with corresponding GUCs, unsetting the storage
parameter is the mechanism for doing so. vacuum_index_cleanup's AUTO
option would be the closest thing, but there's no backing GUC, so the
meaning of AUTO doesn't change unless someone changes the code.

TBH I'm not understanding the pushback for adding a way to determine
whether the storage parameter is actually set. It's very simple, and it
seems like it could be useful elsewhere. But more importantly, it allows
us to more closely match the behavior of the existing reloptions with GUCs,
and it prevents type mismatches (e.g., the reloption is an enum but the GUC
is a Boolean).

[0]: /messages/by-id/Z-Fvmnf57z6zM8Ky@nathan

--
nathan

В письме от понедельник, 24 марта 2025 г. 19:41:27 MSK пользователь Nathan
Bossart написал:

But more importantly, it allows
us to more closely match the behavior of the existing reloptions with GUCs,
and it prevents type mismatches (e.g., the reloption is an enum but the GUC
is a Boolean).

Please do not forget, that reloptions is not the only application of options
mechanism. We have attribute options, opclass options and may have more.
You've just changed the whole engine, for what is seems to be an exceptional
case, that can be solved via existing means. Changing of an engine should not
be done carelessly. You've just make changes for any boolean option of any
postgres object that can ever have options for years and years on. This
change should be done after really good consideration. I strongly suggest to
revert it back, solve the problem via existing means if it needs immediate
solving, and then carefully design boolean-nullable option, or whatever you
want to have, keeping in mind that this changes the whole engine for all
options postgres will ever have. A general design of a new option fature
without anything like "Here I feel like to have this, and there I feel like to
have that". A option type that will be useful for everyone. If it is carefully
considered, I will support it.

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

On Mon, Mar 24, 2025 at 9:41 AM Nathan Bossart <nathandbossart@gmail.com>
wrote:

TBH I'm not understanding the pushback for adding a way to determine
whether the storage parameter is actually set. It's very simple, and it
seems like it could be useful elsewhere.

IMO this is superior to using sentinel values for the same purpose, but
those already exist and having both is reasonably unappealing.

But more importantly, it allows

us to more closely match the behavior of the existing reloptions with GUCs,
and it prevents type mismatches (e.g., the reloption is an enum but the GUC
is a Boolean).

Good point; we could solve this in documentation by simply keeping boolean
if no non-boolean enum values are publicly accessible. But this is part of
why having "set/unset" not use sentinel values is better.

David J.

On Mon, Mar 24, 2025 at 09:40:24AM -0700, David G. Johnston wrote:

So, given the precedent of vacuum_index_cleanup and the above, we should
turn this into an enum that accepts all existing boolean literal inputs and
also has a undocumented "unset" default value that the user is not allowed
to explicitly set but instead only gets used to resolve an unset reloption
at runtime.

This would involve adding a field to relopt_enum_elt_def to declare a value
as "unsettable," right? That seems feasible, but IMHO it's comparable to
adding a field to reopt_parse_elt.

--
nathan

В письме от понедельник, 24 марта 2025 г. 19:58:38 MSK пользователь Nathan
Bossart написал:

On Mon, Mar 24, 2025 at 09:40:24AM -0700, David G. Johnston wrote:

So, given the precedent of vacuum_index_cleanup and the above, we should
turn this into an enum that accepts all existing boolean literal inputs
and
also has a undocumented "unset" default value that the user is not allowed
to explicitly set but instead only gets used to resolve an unset reloption
at runtime.

This would involve adding a field to relopt_enum_elt_def to declare a value
as "unsettable," right? That seems feasible, but IMHO it's comparable to
adding a field to reopt_parse_elt.

If you look at view's check_option option, you will see, how unsettable enum
default can be implemented using existing code.

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su