public inbox for [email protected]help / color / mirror / Atom feed
Re: Disabling vacuum truncate for autovacuum 5+ messages / 2 participants [nested] [flat]
* Re: Disabling vacuum truncate for autovacuum @ 2025-03-20 18:13 Nathan Bossart <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Nathan Bossart @ 2025-03-20 18:13 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Fujii Masao <[email protected]>; Robert Treat <[email protected]>; Laurenz Albe <[email protected]>; Gurjeet Singh <[email protected]>; Andres Freund <[email protected]>; Will Storey <[email protected]>; Robert Haas <[email protected]>; Postgres Hackers <[email protected]> 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 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Disabling vacuum truncate for autovacuum @ 2025-03-20 21:18 David G. Johnston <[email protected]> parent: Nathan Bossart <[email protected]> 1 sibling, 1 reply; 5+ messages in thread From: David G. Johnston @ 2025-03-20 21:18 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Fujii Masao <[email protected]>; Robert Treat <[email protected]>; Laurenz Albe <[email protected]>; Gurjeet Singh <[email protected]>; Andres Freund <[email protected]>; Will Storey <[email protected]>; Robert Haas <[email protected]>; Postgres Hackers <[email protected]> On Thu, Mar 20, 2025 at 11:13 AM Nathan Bossart <[email protected]> wrote: > 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. > > 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. ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Disabling vacuum truncate for autovacuum @ 2025-03-20 21:31 Nathan Bossart <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Nathan Bossart @ 2025-03-20 21:31 UTC (permalink / raw) To: David G. Johnston <[email protected]>; +Cc: Fujii Masao <[email protected]>; Robert Treat <[email protected]>; Laurenz Albe <[email protected]>; Gurjeet Singh <[email protected]>; Andres Freund <[email protected]>; Will Storey <[email protected]>; Robert Haas <[email protected]>; Postgres Hackers <[email protected]> On Thu, Mar 20, 2025 at 02:18:33PM -0700, David G. Johnston wrote: > 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)? 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 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Disabling vacuum truncate for autovacuum @ 2025-03-21 15:50 David G. Johnston <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: David G. Johnston @ 2025-03-21 15:50 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Fujii Masao <[email protected]>; Robert Treat <[email protected]>; Laurenz Albe <[email protected]>; Gurjeet Singh <[email protected]>; Andres Freund <[email protected]>; Will Storey <[email protected]>; Robert Haas <[email protected]>; Postgres Hackers <[email protected]> On Thu, Mar 20, 2025 at 2:31 PM Nathan Bossart <[email protected]> wrote: > On Thu, Mar 20, 2025 at 02:18:33PM -0700, David G. Johnston wrote: > > 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)? > > 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. > > 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. ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Disabling vacuum truncate for autovacuum @ 2025-03-21 15:54 David G. Johnston <[email protected]> parent: Nathan Bossart <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: David G. Johnston @ 2025-03-21 15:54 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Fujii Masao <[email protected]>; Robert Treat <[email protected]>; Laurenz Albe <[email protected]>; Gurjeet Singh <[email protected]>; Andres Freund <[email protected]>; Will Storey <[email protected]>; Robert Haas <[email protected]>; Postgres Hackers <[email protected]> On Thu, Mar 20, 2025 at 11:13 AM Nathan Bossart <[email protected]> wrote: > > 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; > > > 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. ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2025-03-21 15:54 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-03-20 18:13 Re: Disabling vacuum truncate for autovacuum Nathan Bossart <[email protected]> 2025-03-20 21:18 ` David G. Johnston <[email protected]> 2025-03-20 21:31 ` Nathan Bossart <[email protected]> 2025-03-21 15:50 ` David G. Johnston <[email protected]> 2025-03-21 15:54 ` David G. Johnston <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox