Thread
-
Re: [PATCH] ternary reloption type
Andrey Rachitskiy <pl0h0yp1@gmail.com> — 2026-05-18T19:30:24Z
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@nataraj.su> написал(а): > > -- > Nikolay Shaplov aka Nataraj > Fuzzing Engineer at Postgres Professional > Matrix IM: @dhyan:nataraj.su > > Отправитель: Nikolay Shaplov <dhyan@nataraj.su> > Тема: Ответ: [PATCH] ternary reloption type > Дата: 10 мая 2026 г. в 16:09:28 GMT+5 > Кому: Álvaro Herrera <alvherre@alvh.no-ip.org>, pgsql-hackers@lists.postgresql.org > Копия: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Chris Travers <chris.travers@gmail.com>, Timur Magomedov <t.magomedov@postgrespro.ru>, Nathan Bossart <nathandbossart@gmail.com>, Nikolay Shaplov <dhyan@nataraj.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  >