Subject: Re: Disabling vacuum truncate for autovacuum

From: Nathan Bossart <[email-masked]>
Date: Thu, 20 Mar 2025 13:13:02 -0500

On Thu, Mar 20, 2025 at 09:59:45AM -0700, David G. Johnston wrote:
> I get the need for this kind of logic, since we used a boolean for the
> table option, but as a self-proclaimed hack it seems worth more comments
> than provided here.  Especially pertaining to whether this is indeed
> generic or vacuum_truncate specific.  It's unclear since both isset and
> vacuum_truncate_set have been introduced.

I'm happy to expand the comments, but...

> If it is now a general property
> applying to any setting then vacuum_truncate_set shouldn't be needed - we
> should just get the isset value of the existing vacuum_truncate reloption
> by name.

the reason I added this field is because I couldn't find any existing way
to get this information where it's needed.  So, I followed the existing
pattern of adding an offset to something we can access.  This can be used
for any reloption, but currently vacuum_truncate is the only one that does.

How does something like this look for the comment?

	/*
	 * isset_offset is an optional offset of a field in the result struct
	 * that stores whether the value is explicitly set for the relation or
	 * has just picked up the default value.  In most cases, this can be
	 * deduced by giving the reloption a special default value (e.g., -2 is
	 * a common one for integer reloptions), but this isn't always
	 * possible.  One notable example is Boolean reloptions, where it's
	 * difficult to discern the source of the value.  This offset is only
	 * used if given a value greater than zero.
	 */
	int			isset_offset;

-- 
nathan

---

From: "David G. Johnston" <[email-masked]>
Date: Thu, 20 Mar 2025 14:18:33 -0700

> for any reloption, but currently vacuum_truncate is the only one that does.
I'll come back to the comment if it's needed.  I was concerned about
dump/restore and apparently pg_dump has been perfectly capable of
determining whether the current catalog state for a reloption, even a
boolean, is unset, true, or false.  Namely, the following outcomes:

create table vtt (x int not null, y int not null);
CREATE TABLE public.vtt (
    x integer NOT NULL,
    y integer NOT NULL
);

alter table vtt set (vacuum_truncate = true);
CREATE TABLE public.vtt (
    x integer NOT NULL,
    y integer NOT NULL
)
WITH (vacuum_truncate='true');

alter table vtt reset (vacuum_truncate);

CREATE TABLE public.vtt (
    x integer NOT NULL,
    y integer NOT NULL
);

So my concern about dump/restore seems to be alleviated but then, why can
we not just do whatever pg_dump is doing to decide whether the current
value for vacuum_truncate is its default (and thus would not be dumped) or
not (and would be dumped)?

David J.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 20 Mar 2025 16:31:33 -0500

> not (and would be dumped)?

pg_dump looks at the pg_class.reloptions array directly.  In the vacuum
code, we look at the pre-parsed rd_options (see RelationParseRelOptions()
in relcache.c), which will have already resolved vacuum_truncate to its
default value if it was not explicitly set.  We could probably look at
pg_class.reloptions directly in the vacuum code if we _really_ wanted to,
but I felt that putting this information into rd_options was much cleaner.

-- 
nathan

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 21 Mar 2025 08:50:31 -0700

> but I felt that putting this information into rd_options was much cleaner.
I understand this now and suggest the following comment changes based upon
that understanding.

 diff --git a/src/backend/access/common/reloptions.c
b/src/backend/access/common/reloptions.c
index 645b5c0046..c795df6100 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1912,7 +1912,9 @@ default_reloptions(Datum reloptions, bool validate,
relopt_kind kind)
                {"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)},
+               offsetof(StdRdOptions, vacuum_truncate),
+               /* vacuum_truncate uses the isset_offset member instead of
a sentinel value */
+               offsetof(StdRdOptions, vacuum_truncate_set)},
                {"vacuum_max_eager_freeze_failure_rate", RELOPT_TYPE_REAL,
                offsetof(StdRdOptions,
vacuum_max_eager_freeze_failure_rate)}
        };
diff --git a/src/include/access/reloptions.h
b/src/include/access/reloptions.h
index 146aed47c2..fddeee0d54 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -152,7 +152,17 @@ typedef struct
        const char *optname;            /* option's name */
        relopt_type opttype;            /* option's datatype */
        int                     offset;                 /* offset of field
in result struct */
-       int                     isset_offset;   /* if > 0, offset of "is
set" field */
+       /*
+        * The reloptions system knows whether a reloption has been set by
the
+        * DBA or not.  Historically, this information was lost when parsing
+        * the reloptions so we coped by using sentinel values.
+        * This doesn't work for boolean reloptions.  For those, we
+        * need a place for the reloption parser to store its knowledge of
+        * whether the reloption was set.  Set this field to an offset
+        * in the StdRdOptions struct.  The pointed-to location needs to
+        * be a bool.  A value of 0 here is interpreted as "no need to
store".
+        */
+       int                     isset_offset;
 } relopt_parse_elt;

 /* Local reloption definition */
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index d94fddd7ce..461648aac6 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -344,7 +344,7 @@ typedef struct StdRdOptions
        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 */
+       bool            vacuum_truncate_set;    /* reserve space for isset
status of vacuum_truncate */

        /*
         * Fraction of pages in a relation that vacuum can eagerly scan and
fail

David J.

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 21 Mar 2025 08:54:55 -0700

>          * used if given a value greater than zero.
I didn't actually come back to this before writing my comment.

I'm glad they both say basically the same thing.

I'm still partial to mine but yours probably fits the overall style of the
codebase better.

David J.