On Mon, Mar 24, 2025 at 08:57:27PM -0700, David G. Johnston wrote:
> On Mon, Mar 24, 2025 at 11:45 AM Nathan Bossart <nathandboss...@gmail.com>
> wrote:
>> * I don't think this matches the parse_bool() behavior exactly. For
>> example, parse_bool() appears to accept inputs like "t" to mean "true".
>> This is also probably not a huge deal.
>
> Fixable for sure.
I've attached an attempt at this.
>> That being said, I'm open to applying this patch if it seems like a
>> majority of folks prefer this implementation. FWIW I'm still partial to
>> the isset_offset approach for the reasons I've already discussed.
>
> I'm preferable to turning the implementation into an enum. Slight
> preference to documenting it as the boolean it presently is; pending any
> information regarding where in user-space, particularly in psql but also
> any client-side details, this internal detail might show through. It
> avoids having to tell the reader that we choose enum in order to
> accommodate optionality. But it isn't the end of the world if we do say
> that either.
The only other place I found that reveals this internal implementation
detail is parse_one_reloption. In the attached patch, I've modified it to
emit the same error message as for RELOPT_TYPE_BOOL to further uphold the
illusion that it is a Boolean reloption. I'm not sure I've patched up all
such holes, though.
TBH I find the attached to be even more of a hack than isset_offset. We
have to try to match behavior in multiple places, and we need lengthy
commentary about why you'd want to use this enum. Furthermore, it's only
useful for Boolean-style reloptions, unlike isset_offset which can be used
for any type. But perhaps more importantly, I agree with Robert that it's
not clear what problem it's trying to solve. The concrete arguments in
this thread range from avoiding extra bytes in the struct to maintaining
consistency with other options with backing GUCs, which I'd argue are more
style preferences than anything.
In any case, AFAICT the votes are somewhat evenly divided at the moment, so
I do not intend to proceed with this patch for now.
--
nathan
>From d9034c94d496ba497f65acfe3c80b57b08b4a95c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Mon, 24 Mar 2025 12:46:26 -0500
Subject: [PATCH v2 1/1] change vacuum_truncate relopt to enum
---
src/backend/access/common/reloptions.c | 89 +++++++++++++++++++-------
src/backend/commands/vacuum.c | 4 +-
src/include/access/reloptions.h | 13 ----
src/include/utils/rel.h | 15 ++++-
src/tools/pgindent/typedefs.list | 1 +
5 files changed, 82 insertions(+), 40 deletions(-)
diff --git a/src/backend/access/common/reloptions.c
b/src/backend/access/common/reloptions.c
index 645b5c00467..a011a932e7d 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -147,15 +147,6 @@ static relopt_bool boolRelOpts[] =
},
false
},
- {
- {
- "vacuum_truncate",
- "Enables vacuum to truncate empty pages at the end of
this table",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
- ShareUpdateExclusiveLock
- },
- true
- },
{
{
"deduplicate_items",
@@ -524,6 +515,43 @@ static relopt_enum_elt_def viewCheckOptValues[] =
{(const char *) NULL} /* list terminator */
};
+/*
+ * The values are from StdRdOptBool.
+ *
+ * This enum is meant to be used for Boolean reloptions for which we need to
+ * be able to determine whether the value was explicitly set for the relation.
+ * There is a third unusable StdRdOptBool value called
+ * STDRD_OPTION_BOOL_NOT_SET that should be set as the default value for such
+ * options. Then, code that uses the option can first test for the "not set"
+ * value and fall back to something else (e.g., a GUC) as needed.
+ *
+ * Note that the strings below have been carefully chosen to match the behavior
+ * of parse_bool().
+ */
+static relopt_enum_elt_def StdRdOptBoolValues[] =
+{
+ {"on", STDRD_OPTION_BOOL_ON},
+ {"of", STDRD_OPTION_BOOL_OFF},
+ {"off", STDRD_OPTION_BOOL_OFF},
+ {"t", STDRD_OPTION_BOOL_ON},
+ {"tr", STDRD_OPTION_BOOL_ON},
+ {"tru", STDRD_OPTION_BOOL_ON},
+ {"true", STDRD_OPTION_BOOL_ON},
+ {"f", STDRD_OPTION_BOOL_OFF},
+ {"fa", STDRD_OPTION_BOOL_OFF},
+ {"fal", STDRD_OPTION_BOOL_OFF},
+ {"fals", STDRD_OPTION_BOOL_OFF},
+ {"false", STDRD_OPTION_BOOL_OFF},
+ {"y", STDRD_OPTION_BOOL_ON},
+ {"ye", STDRD_OPTION_BOOL_ON},
+ {"yes", STDRD_OPTION_BOOL_ON},
+ {"n", STDRD_OPTION_BOOL_OFF},
+ {"no", STDRD_OPTION_BOOL_OFF},
+ {"1", STDRD_OPTION_BOOL_ON},
+ {"0", STDRD_OPTION_BOOL_OFF},
+ {(const char *) NULL} /* list terminator */
+};
+
static relopt_enum enumRelOpts[] =
{
{
@@ -559,6 +587,16 @@ static relopt_enum enumRelOpts[] =
VIEW_OPTION_CHECK_OPTION_NOT_SET,
gettext_noop("Valid values are \"local\" and \"cascaded\".")
},
+ {
+ {
+ "vacuum_truncate",
+ "Enables vacuum to truncate empty pages at the end of
this table",
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
+ },
+ StdRdOptBoolValues,
+ STDRD_OPTION_BOOL_NOT_SET
+ },
/* list terminator */
{{NULL}}
};
@@ -1672,12 +1710,29 @@ parse_one_reloption(relopt_value *option, char
*text_str, int text_len,
}
}
if (validate && !parsed)
+ {
+ /*
+ * If the enum is using
StdRdOptBoolValues, it is actually
+ * a Boolean reloption for all intents
and purposes, but
+ * we need to be able to use a third
value of "not set" as
+ * the default for the reloption to be
able to determine
+ * whether it is actually set for the
relation. To uphold
+ * the illusion that this is a Boolean
reloption, we emit
+ * the same error message as for
RELOPT_TYPE_BOOL above.
+ */
+ if (optenum->members ==
StdRdOptBoolValues)
+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+
errmsg("invalid value for boolean option \"%s\": %s",
+
option->gen->name, value)));
+
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid value
for enum option \"%s\": %s",
option->gen->name, value),
optenum->detailmsg ?
errdetail_internal("%s", _(optenum->detailmsg)) : 0));
+ }
/*
* If value is not among the allowed string
values, but we are
@@ -1779,17 +1834,6 @@ fillRelOptions(void *rdopts, Size basesize,
char *itempos = ((char *) rdopts) +
elems[j].offset;
char *string_val;
- /*
- * If isset_offset is provided, store whether
the reloption is
- * set there.
- */
- if (elems[j].isset_offset > 0)
- {
- char *setpos = ((char *) rdopts)
+ elems[j].isset_offset;
-
- *(bool *) setpos = options[i].isset;
- }
-
switch (options[i].gen->type)
{
case RELOPT_TYPE_BOOL:
@@ -1911,8 +1955,8 @@ default_reloptions(Datum reloptions, bool validate,
relopt_kind kind)
offsetof(StdRdOptions, parallel_workers)},
{"vacuum_index_cleanup", RELOPT_TYPE_ENUM,
offsetof(StdRdOptions, vacuum_index_cleanup)},
- {"vacuum_truncate", RELOPT_TYPE_BOOL,
- offsetof(StdRdOptions, vacuum_truncate), offsetof(StdRdOptions,
vacuum_truncate_set)},
+ {"vacuum_truncate", RELOPT_TYPE_ENUM,
+ offsetof(StdRdOptions, vacuum_truncate)},
{"vacuum_max_eager_freeze_failure_rate", RELOPT_TYPE_REAL,
offsetof(StdRdOptions, vacuum_max_eager_freeze_failure_rate)}
};
@@ -1992,7 +2036,6 @@ build_local_reloptions(local_relopts *relopts, Datum
options, bool validate)
elems[i].optname = opt->option->name;
elems[i].opttype = opt->option->type;
elems[i].offset = opt->offset;
- elems[i].isset_offset = 0; /* not supported for local
relopts yet */
i++;
}
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index f0a7b87808d..7c725ea4b76 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -2206,9 +2206,9 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams
*params,
{
StdRdOptions *opts = (StdRdOptions *) rel->rd_options;
- if (opts && opts->vacuum_truncate_set)
+ if (opts && opts->vacuum_truncate != STDRD_OPTION_BOOL_NOT_SET)
{
- if (opts->vacuum_truncate)
+ if (opts->vacuum_truncate == STDRD_OPTION_BOOL_ON)
params->truncate = VACOPTVALUE_ENABLED;
else
params->truncate = VACOPTVALUE_DISABLED;
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index dfbb4c85460..43445cdcc6c 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -152,19 +152,6 @@ typedef struct
const char *optname; /* option's name */
relopt_type opttype; /* option's datatype */
int offset; /* offset of field in
result struct */
-
- /*
- * isset_offset is an optional offset of a field in the result struct
that
- * stores whether the option is explicitly set for the relation or if it
- * just picked up the default value. In most cases, this can be
- * accomplished by giving the reloption a special out-of-range default
- * value (e.g., some integer reloptions use -2), but this isn't always
- * possible. For example, a Boolean reloption cannot be given an
- * out-of-range default, so we need another way to discover the source
of
- * its value. This offset is only used if given a value greater than
- * zero.
- */
- int isset_offset;
} relopt_parse_elt;
/* Local reloption definition */
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index d94fddd7cef..fbcc12a9cc7 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -334,6 +334,18 @@ typedef enum StdRdOptIndexCleanup
STDRD_OPTION_VACUUM_INDEX_CLEANUP_ON,
} StdRdOptIndexCleanup;
+/*
+ * This is useful for Boolean relopts when we need to be able to determine
+ * whether it's explicitly set for a relation. See comment atop
+ * StdRdOptBoolValues in reloptions.c for details.
+ */
+typedef enum StdRdOptBool
+{
+ STDRD_OPTION_BOOL_NOT_SET = 0,
+ STDRD_OPTION_BOOL_ON,
+ STDRD_OPTION_BOOL_OFF,
+} StdRdOptBool;
+
typedef struct StdRdOptions
{
int32 vl_len_; /* varlena header (do not touch
directly!) */
@@ -343,8 +355,7 @@ typedef struct StdRdOptions
bool user_catalog_table; /* use as an additional catalog
relation */
int parallel_workers; /* max number of
parallel workers */
StdRdOptIndexCleanup vacuum_index_cleanup; /* controls index
vacuuming */
- bool vacuum_truncate; /* enables vacuum to truncate a
relation */
- bool vacuum_truncate_set; /* whether vacuum_truncate is
set */
+ StdRdOptBool vacuum_truncate; /* enables vacuum to truncate a
relation */
/*
* Fraction of pages in a relation that vacuum can eagerly scan and fail
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 3fbf5a4c212..ac530fb40e7 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2818,6 +2818,7 @@ StatsData
StatsElem
StatsExtInfo
StdAnalyzeData
+StdRdOptBool
StdRdOptIndexCleanup
StdRdOptions
Step
--
2.39.5 (Apple Git-154)
