Re: [PATCH] ternary reloption type

Hi, Nikolay!
Here is a brief review and suggestions for the patch.

```
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -98,8 +98,8 @@ typedef struct relopt_bool
typedef struct relopt_ternary
{
relopt_gen gen;
- const char *unset_alias; /* word that will be treated as unset value */
- int default_val;
+ const char *unset_alias; /* word treated as unset, or NULL */
+ pg_ternary default_val;
} relopt_ternary;

typedef struct relopt_int
```
The v2 patch added default_val but typed it as int. The field holds PG_TERNARY_TRUE/FALSE/UNSET, so it should use pg_ternary, same as ternary_val in relopt_value and the add_ternary_reloption() parameters.

```
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -915,7 +915,8 @@ add_local_bool_reloption(local_relopts *relopts, const char *name,
*/
static relopt_ternary *
init_ternary_reloption(uint32 kinds, const char *name, const char *desc,
- pg_ternary default_val, const char* unset_alias, LOCKMODE lockmode)
+ pg_ternary default_val, const char *unset_alias,
+ LOCKMODE lockmode)
{
relopt_ternary *newoption;

@@ -933,7 +934,8 @@ init_ternary_reloption(uint32 kinds, const char *name, const char *desc,
*/
void
add_ternary_reloption(uint32 kinds, const char *name, const char *desc,
- pg_ternary default_val, const char* unset_alias, LOCKMODE lockmode)
+ pg_ternary default_val, const char *unset_alias,
+ LOCKMODE lockmode)
{
relopt_ternary *newoption;

@@ -952,7 +954,7 @@ add_ternary_reloption(uint32 kinds, const char *name, const char *desc,
void
add_local_ternary_reloption(local_relopts *relopts, const char *name,
const char *desc, pg_ternary default_val,
- const char* unset_alias, int offset)
+ const char *unset_alias, int offset)
{
relopt_ternary *newoption;

--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -191,8 +191,8 @@ extern relopt_kind add_reloption_kind(void);
extern void add_bool_reloption(uint32 kinds, const char *name, const char *desc,
bool default_val, LOCKMODE lockmode);
extern void add_ternary_reloption(uint32 kinds, const char *name,
- const char *desc, pg_ternary default_val,
- const char* unset_alias, LOCKMODE lockmode);
+ const char *desc, pg_ternary default_val,
+ const char *unset_alias, LOCKMODE lockmode);
extern void add_int_reloption(uint32 kinds, const char *name, const char *desc,
int default_val, int min_val, int max_val,
LOCKMODE lockmode);
@@ -213,9 +213,9 @@ extern void add_local_bool_reloption(local_relopts *relopts, const char *name,
const char *desc, bool default_val,
int offset);
extern void add_local_ternary_reloption(local_relopts *relopts,
- const char *name, const char *desc,
- pg_ternary default_val, const char* unset_alias,
- int offset);
+ const char *name, const char *desc,
+ pg_ternary default_val,
+ const char *unset_alias, int offset);
extern void add_local_int_reloption(local_relopts *relopts, const char *name,
const char *desc, int default_val,
int min_val, int max_val, int offset);
```
Formatting only (const char * and prototype alignment), no semantic change.

```
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1723,10 +1725,10 @@ parse_one_reloption(relopt_value *option, char *text_str, int text_len,
if (validate && !parsed)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("invalid value for option \"%s\": %s",
+ errmsg("invalid value for option \"%s\": %s",
option->gen->name, value),
- errdetail("Valid values are \"on\", \"off\", and \"%s\".",
- opt->unset_alias)));
+ errdetail("Valid values are \"on\", \"off\", \"true\", \"false\", \"yes\", \"no\", \"1\", \"0\", and \"%s\".",
+ opt->unset_alias)));
}
break;
case RELOPT_TYPE_INT:

--- a/src/test/modules/dummy_index_am/expected/reloptions.out
+++ b/src/test/modules/dummy_index_am/expected/reloptions.out
@@ -123,7 +123,7 @@ ERROR: invalid value for boolean option "option_ternary_1": do_not_know_yet
ALTER INDEX dummy_test_idx SET (option_ternary_2 = 'do_not_know_yet'); -- ok
ALTER INDEX dummy_test_idx SET (option_ternary_2 = 'illegal_value'); -- error
ERROR: invalid value for option "option_ternary_2": illegal_value
-DETAIL: Valid values are "on", "off", and "do_not_know_yet".
+DETAIL: Valid values are "on", "off", "true", "false", "yes", "no", "1", "0", and "do_not_know_yet".
SELECT unnest(reloptions) FROM pg_class WHERE relname = 'dummy_test_idx';
unnest
----------------------------------
```
Ternary values are parsed with parse_bool() and then unset_alias, so a rejection should tell the user the full set of valid spellings. The previous enum-based vacuum_index_cleanup error effectively did that; we restore the same level of detail for the ternary path.

```
@@ -1901,7 +1903,8 @@ fillRelOptions(void *rdopts, Size basesize,
break;
case RELOPT_TYPE_TERNARY:
*(pg_ternary *) itempos = options[i].isset ?
- options[i].ternary_val : PG_TERNARY_UNSET;
+ options[i].ternary_val :
+ ((relopt_ternary *) options[i].gen)->default_val;
break;
case RELOPT_TYPE_INT:
*(int *) itempos = options[i].isset ?
```
parseRelOptions() returns every option of a kind, with isset=false when it is not in pg_class.reloptions. For bool/int/enum we already apply default_val in that case; ternary was still hardcoding PG_TERNARY_UNSET, so default_val on relopt_ternary was unused.
Built-in ternaries (vacuum_truncate, vacuum_index_cleanup, buffering) all have default_val == PG_TERNARY_UNSET, so behavior for them is unchanged. This matters for AMs that call add_ternary_reloption() with another default (dummy_index_am option_ternary_2).

```
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -913,7 +913,8 @@ gistoptions(Datum reloptions, bool validate)
{
static const relopt_parse_elt tab[] = {
{"fillfactor", RELOPT_TYPE_INT, offsetof(GiSTOptions, fillfactor)},
- {"buffering", RELOPT_TYPE_ENUM, offsetof(GiSTOptions, buffering_mode)}
+ {"buffering", RELOPT_TYPE_TERNARY,
+ offsetof(GiSTOptions, buffering_mode)}
};
```
buffering is registered globally as a ternary reloption; the local relopt_parse_elt table still said ENUM after v2. fillRelOptions() uses options[i].gen->type from the global registry, so this was not a runtime bug, but the table was wrong and confusing for anyone reading the GiST options path.

```
--- a/src/test/regress/expected/reloptions.out
+++ b/src/test/regress/expected/reloptions.out
@@ -134,6 +134,23 @@ SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass;
{vacuum_index_cleanup=auto}
(1 row)

+-- boolean synonyms are accepted
+DROP TABLE reloptions_test;
+CREATE TABLE reloptions_test(i INT) WITH (vacuum_index_cleanup=true);
+SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass;
+ reloptions
+-----------------------------
+ {vacuum_index_cleanup=true}
+(1 row)
+
+DROP TABLE reloptions_test;
+CREATE TABLE reloptions_test(i INT) WITH (vacuum_index_cleanup=false);
+SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass;
+ reloptions
+------------------------------
+ {vacuum_index_cleanup=false}
+(1 row)
+

--- a/src/test/regress/sql/reloptions.sql
+++ b/src/test/regress/sql/reloptions.sql
@@ -80,6 +80,14 @@ DROP TABLE reloptions_test;
CREATE TABLE reloptions_test(i INT) WITH (vacuum_index_cleanup=auto);
SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass;

+-- boolean synonyms are accepted
+DROP TABLE reloptions_test;
+CREATE TABLE reloptions_test(i INT) WITH (vacuum_index_cleanup=true);
+SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass;
+DROP TABLE reloptions_test;
+CREATE TABLE reloptions_test(i INT) WITH (vacuum_index_cleanup=false);
+SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass;
+
-- Test vacuum_truncate option
DROP TABLE reloptions_test;
```
After moving vacuum_index_cleanup from enum to ternary, we still accept true/false via parse_bool(). These tests lock that in and show the value is stored in pg_class.reloptions as given (not normalized to on/off).

```
--- a/src/test/regress/expected/gist.out
+++ b/src/test/regress/expected/gist.out
@@ -9,11 +9,12 @@ create index gist_pointidx on gist_point_tbl using gist(p);
create index gist_pointidx2 on gist_point_tbl using gist(p) with (buffering = on, fillfactor=50);
create index gist_pointidx3 on gist_point_tbl using gist(p) with (buffering = off);
create index gist_pointidx4 on gist_point_tbl using gist(p) with (buffering = auto);
-drop index gist_pointidx2, gist_pointidx3, gist_pointidx4;
+create index gist_pointidx4b on gist_point_tbl using gist(p) with (buffering = true);
+drop index gist_pointidx2, gist_pointidx3, gist_pointidx4, gist_pointidx4b;
-- Make sure bad values are refused
create index gist_pointidx5 on gist_point_tbl using gist(p) with (buffering = invalid_value);
ERROR: invalid value for option "buffering": invalid_value
-DETAIL: Valid values are "on", "off", and "auto".
+DETAIL: Valid values are "on", "off", "true", "false", "yes", "no", "1", "0", and "auto".
create index gist_pointidx5 on gist_point_tbl using gist(p) with (fillfactor=9);
ERROR: value 9 out of bounds for option "fillfactor"
DETAIL: Valid values are between "10" and "100".

--- a/src/test/regress/sql/gist.sql
+++ b/src/test/regress/sql/gist.sql
@@ -11,7 +11,8 @@ create index gist_pointidx on gist_point_tbl using gist(p);
create index gist_pointidx2 on gist_point_tbl using gist(p) with (buffering = on, fillfactor=50);
create index gist_pointidx3 on gist_point_tbl using gist(p) with (buffering = off);
create index gist_pointidx4 on gist_point_tbl using gist(p) with (buffering = auto);
-drop index gist_pointidx2, gist_pointidx3, gist_pointidx4;
+create index gist_pointidx4b on gist_point_tbl using gist(p) with (buffering = true);
+drop index gist_pointidx2, gist_pointidx3, gist_pointidx4, gist_pointidx4b;
```
Same idea for GiST buffering: true should work as a synonym for on. Expected output updated for the new index and the extended DETAIL line on invalid input.

------
Regards,
Andrey Rachitskiy


> 18 мая 2026 г., в 19:11, Nikolay Shaplov <dhyan(at)nataraj(dot)su> написал(а):
>
> --
> Nikolay Shaplov aka Nataraj
> Fuzzing Engineer at Postgres Professional
> Matrix IM: @dhyan:nataraj.su
>
> Отправитель: Nikolay Shaplov <dhyan(at)nataraj(dot)su>
> Тема: Ответ: [PATCH] ternary reloption type
> Дата: 10 мая 2026 г. в 16:09:28 GMT+5
> Кому: Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, pgsql-hackers(at)lists(dot)postgresql(dot)org
> Копия: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Chris Travers <chris(dot)travers(at)gmail(dot)com>, Timur Magomedov <t(dot)magomedov(at)postgrespro(dot)ru>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, Nikolay Shaplov <dhyan(at)nataraj(dot)su>
>
>
> В письме от четверг, 5 февраля 2026 г. 16:08:59 Москва, стандартное время
> пользователь Nikolay Shaplov написал:
>
> Here hoes a rebased version of second part of ternary patch that changes
> `vacuum_index_cleanup` and GiST's `buffering` reloptions to ternary type
>
> --
> Nikolay Shaplov aka Nataraj
> Fuzzing Engineer at Postgres Professional
> Matrix IM: @dhyan:nataraj.su

>