Обсуждение: [PATCH] trenary reloption type

Hi, All!

There is ongoing tendency in PostgreSQL to replace boolean reloptions that has 
"on" and "off" state, with other option types that has "on", "off" and "use 
defaults" states.

Some of these options are implemented as enum options. They are
"vacuum_index_cleanup" and gist's "buffering"

and one recently converted option "vacuum_truncate" that uses extra 
"vacuum_truncate_set" flag to indicate it's unset state.

This patch introduce trenary reloptions type, that replaces both 
implementation with separate data-type that behaves in the same way as bool 
type does, but has one extra state that indicate that option have not been set 
to "on" or "off" state. 

This third state, I call it "unset" state, can either  be only reached by not 
setting "true" or "false" value to an option, as it is done in vacuum_truncate 
option. Or option designer can assign this third state a custom name, so user 
can explicitly  set option to the "third" slate. As it is done in 
`vacuum_index_cleanup` and gist's `buffering` option, using "auto" keyword.

This patch changes implementation of `vacuum_truncate`, `vacuum_index_cleanup` 
and gist's `buffering` to trinary options. This make code more neat and 
consistent. I'd suggest to commit it to the master branch.

Possible flaws and drawbacks:

1. I've added trenary enum type to c.h. It might be a bit too global, but I 
did not find any better place for it, since it is needed globally and it is 
kind of similar to boolean. If you know better place, please speak.

2. `vacuum_index_cleanup` and gist's `buffering` will now accepts all possible 
"true" and "false" aliases as in boolean type, the way they did not do it 
before. Like "1" or "FAL".  I see no great harm in it, but it is still 
behaviour change.

3. Error messages for `vacuum_index_cleanup` and gist's `buffering` are now 
less informative. It is not right, but I do not see right now a way to improve 
that. May be it is a price to pay for code consistency. If you have any idea 
how to fix it, please speak.

As for the rest, other behavior should not be changed.

I've added as many tests as I can. local_reloption support is also 
implemented.

I've split patch into four part so it can be read and reviewed step by step:
1. Tests that ensures old behaviour
2. Trenary option for `vacuum_truncate` reloption
3. Add "unset_alias" feature to implement "auto" alias for 
`vacuum_index_cleanup` and gist's `buffering`
4. More tests

But I guess they should be commit as a single commit.

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

My English is bad :-(

It is either trinary, or ternary, but not what I've written in previous 
message.

Thanks to Timur for pointing to this issue.

Here goes a new version of the patch with proper naming for an new option 
type. 



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

Hello Nikolay!

Found a typo in reloptions.h, treaed -> treated.

Can ternary enum be added in a separate header file, say,
src/include/ternary.h instead of adding it to c.h? I'm just not sure if
c.h is it the right place for relation-options-specific code.
Of course, I can be wrong.

--
Regards,
Timur Magomedov

В письме от пятница, 12 сентября 2025 г. 16:46:19 MSK пользователь Timur
Magomedov написал:
> Hello Nikolay!
>
> Found a typo in reloptions.h, treaed -> treated.
Oups. Fixed that in the attached version.

> Can ternary enum be added in a separate header file, say,
> src/include/ternary.h instead of adding it to c.h? I'm just not sure if
> c.h is it the right place for relation-options-specific code.
> Of course, I can be wrong.

I am not sure either. But my guess is that spamming into c.h is lesser crime
then adding another useless header file.

Moreover, ternary value is not relation-options-specific, it is actually
relation specific, if you think about it thoroughly. Relation code uses it, and
there is no way to avoid that.

Are there any other notions about the code?

I tried to make thongs more neat and more consistent here. Did I succeed?

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

I took a quick look at 0001+0002 and I think it's quite reasonable.
Here it is again with some minor fixups.  (I'm omitting the further
patches for now, we can rebase them later.)

I'm CCing Nathan as committer of the vacuum_truncate_set stuff which
Nikolay so strongly disliked.  Any objections to going with this
approach?

Thanks,


(Please note that Gmail is sabotaging my kurilemu.de domain, so there's
significant delay in my emails to the list from that address.  I guess
I'm lucky that Nikolay decided to CC my old address in this thread.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The problem with the future is that it keeps turning into the present"
(Hobbes)

On Fri, Jan 16, 2026 at 04:14:52PM +0100, Álvaro Herrera wrote:
> I'm CCing Nathan as committer of the vacuum_truncate_set stuff which
> Nikolay so strongly disliked.  Any objections to going with this
> approach?

Looks generally reasonable.

> This could also be used for other options such as `vacuum_index_cleanup`
> and `buffering`, but lets get the scaffolding in first.

Part of me wonders if we should just modify the Boolean relopt
implementation instead of using ternary only when needed.

> +                parsed = parse_bool(value, &b);
> +                option->values.ternary_val = b ? TERNARY_TRUE : TERNARY_FALSE;
> +                if (validate && !parsed)
> +                    ereport(ERROR,
> +                            (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +                             errmsg("invalid value for ternary option \"%s\": %s",
> +                                    option->gen->name, value)));

Shouldn't this say "invalid value for boolean option"?  IIUC the intent is
for ternary to be exactly like bool, except it defaults to an "unset" value
that can't be chosen by the user.  In that sense, I think "ternary" is kind
of a misnomer, but I wouldn't count this as an objection.

-- 
nathan

I took a quick look at 0001+0002 and I think it's quite reasonable.
Here it is again with some minor fixups.

Good. I like ternary -> pg_ternary change. That is reasonable. And postgres.h is better place for it then c.h.

  (I'm omitting the further
patches for now, we can rebase them later.)

I've rebased the rest of patches, and the whole patchset is in the attachment.

I'm CCing Nathan as committer of the vacuum_truncate_set stuff which
Nikolay so strongly disliked.  Any objections to going with this
approach?

Ok, will quote him and reply below in the this letter.

> This could also be used for other options such as `vacuum_index_cleanup`
> and `buffering`, but lets get the scaffolding in first.

Part of me wonders if we should just modify the Boolean relopt
implementation instead of using ternary only when needed.

Ternary option, with it's third optional option is quite complex.

I'd rather not bother postgres developer with this complexity if they just want to add pure boolean option.

>+				parsed = parse_bool(value, &b);
>+				option->values.ternary_val = b ? TERNARY_TRUE : TERNARY_FALSE;
>+				if (validate && !parsed)
>+					ereport(ERROR,
>+							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>+							 errmsg("invalid value for ternary option \"%s\": %s",
>+									option->gen->name, value)));

Shouldn't this say "invalid value for boolean option"?  IIUC the intent is
for ternary to be exactly like bool, except it defaults to an "unset" value
that can't be chosen by the user.  In that sense, I think "ternary" is kind
of a misnomer, but I wouldn't count this as an objection.

If you look at buffering option for example, you will see it does not behave as pure boolean option.
"auto" value can be explicitly set by user. 

Error reporting is not strong part of my patch, I would agree with that.

What if we give two different error reports in two different cases:

Give boolean-like error report when option does not have explicit alias for third option, and more verbose
error message, listing all available options in the case when third option have explicit alias?

like 

"invalid value for boolean option \"%s\": %s",

and

"invalid value for option \"%s\": %s",
"valid values are 'on', 'off' and '%s'"


we may actually say nothing about option being ternary here

On 2026-Jan-21, Nikolay Shaplov wrote:

> On 16.01.2026 18:14, Álvaro Herrera wrote:
> > I took a quick look at 0001+0002 and I think it's quite reasonable.
> > Here it is again with some minor fixups.
> 
> Good. I like ternary -> pg_ternary change. That is reasonable. And
> postgres.h is better place for it then c.h.

I further changed TERNARY_TRUE and so on to have a PG_ prefix also; it's
not impossible that there's userland code somewhere outside Postgres
that uses those symbol names, so let's avoid a collision.

> I've rebased the rest of patches, and the whole patchset is in the
> attachment.

I'm not a fan of how these commit messages are structured.  You explain
the point of the whole patch series in the commit message for 0001, but
that one only adds some tests for the existing behavior.  If I were to
commit that, it would make no sense in the overall Postgres commit
history.  So I have squashed 0001 with 0002, and also with the fraction
of 0004 that includes tests for the feature in 0002.  I think it's
strange to submit those tests in 0004, when the other half of the tests
are for the feature in 0003.  I recommend to consider how you structure
your patch splits so that they would make sense in the Postgres commit
history assuming they are committed on separate days, and that there are
multiple other commits in between.

I do agree with Nathan that there seems to be little point in the
message saying this new type is different from Boolean.  The user can
still say only "on" or "off".  Even with your "alias" proposal, there
will only be a mechanism to let the system choose between those two
values, but it will still be one or the other.  There's no provision to
have the system behave as if the user set the value to half.

Another thing I did is remove default_val for ternaries.  As far as I
can see, it makes no sense.  If your reloption defaults to either on or
off, then it's just a Boolean, right?  It can no longer be unset,
because if you unset it, then it becomes the default.

Anyway, I have pushed the first part, after rewriting the commit
message.  You can resubmit the rest after rebasing on the current tree.
I have also marked the commitfest item as committed.  Please create a
new one for the next part.

Thanks,

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"El sentido de las cosas no viene de las cosas, sino de
las inteligencias que las aplican a sus problemas diarios
en busca del progreso." (Ernesto Hernández-Novich)

I've rebased the rest of patches, and the whole patchset is in the
attachment.

I'm not a fan of how these commit messages are structured.  You explain
the point of the whole patch series in the commit message for 0001, but
that one only adds some tests for the existing behavior.  If I were to
commit that, it would make no sense in the overall Postgres commit
history.  So I have squashed 0001 with 0002, and also with the fraction
of 0004 that includes tests for the feature in 0002.  

I guess that comes from my frustration with big reloptions patch. I've been  looking
for a way to make iy more easy to read and understand. My thought was that if
I split them into logical "layers" showing the  development of the patch's idea, it
would be more easy to understand them as a whole. And they were intended
to be squashed before commit, may be I should state that more clearly.

I think it's
strange to submit those tests in 0004, when the other half of the tests
are for the feature in 0003.  

Yeah, here you are right, I should add tests both to 0003 and 0004. I did not think
they can be committed separately

I recommend to consider how you structure
your patch splits so that they would make sense in the Postgres commit
history assuming they are committed on separate days, and that there are
multiple other commits in between.

Second part of ternary options patch is consist of one piece, so now it is not a problem,
and as for big reloptions patch, I think we can discuss it later. I doubt it is readable
when it is provided in single piece. 

I do agree with Nathan that there seems to be little point in the
message saying this new type is different from Boolean.  The user can
still say only "on" or "off".  Even with your "alias" proposal, there
will only be a mechanism to let the system choose between those two
values, but it will still be one or the other.  There's no provision to
have the system behave as if the user set the value to half.

For ternary options with explicit "third" value, I added another error message, it says nothing
about option type, it just lists possible values. This can be good solution.

Another thing I did is remove default_val for ternaries.  As far as I
can see, it makes no sense.  If your reloption defaults to either on or
off, then it's just a Boolean, right?  It can no longer be unset,
because if you unset it, then it becomes the default.

For the part you've committed that is correct. But with explicit "third" option, you can't tell
for sure which value is default. Like:
prefer_XXXX_optimization: yes/no/never
It can be implemented as ternary option, but default value here can be "yes".
I would not try to predict what behavior option developer will need, and try to provide all
possibilities.
That's why I put default value back. But I will not be much upset if you still decide to remove it.
Or I can remove it myself if you insist. We can add it later when someone runs into this "never" case.

Anyway, I have pushed the first part, after rewriting the commit
message.  

Thanks! Now the world is better place, from my point of view.

You can resubmit the rest after rebasing on the current tree.
I have also marked the commitfest item as committed.  Please create a
new one for the next part.

I've reworked second part of the patch, It is in the attachment, and I am going to create new commitfest record for it. Thank you for your work.