Re: vacuum_truncate configuration parameter and isset_offset

From: Nathan Bossart <nathandbossart(at)gmail(dot)com>
To: "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>
Cc: Nikolay Shaplov <dhyan(at)nataraj(dot)su>, Robert Haas <robertmhaas(at)gmail(dot)com>, Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Robert Treat <rob(at)xzilla(dot)net>, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>, Albe <laurenz(dot)albe(at)cybertec(dot)at>, Gurjeet Singh <gurjeet(at)singh(dot)im>, Will Storey <will(at)summercat(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: vacuum_truncate configuration parameter and isset_offset
Date: 2025-03-25 14:57:46
Message-ID: Z-LEauKA1SCbF493@nathan
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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 <nathandbossart(at)gmail(dot)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

Attachment Content-Type Size
v2-0001-change-vacuum_truncate-relopt-to-enum.patch text/plain 8.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Sami Imseih 2025-03-25 14:59:34 Re: query_id: jumble names of temp tables for better pg_stat_statement UX
Previous Message Corey Huinker 2025-03-25 14:53:22 Re: Statistics Import and Export