Re: vacuum_truncate configuration parameter and isset_offset

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Nikolay Shaplov <dhyan(at)nataraj(dot)su>
Cc: "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, Nathan Bossart <nathandbossart(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-26 14:53:15
Message-ID: CA+TgmoZkegV_Scm2s+2vX1JLb_4dvevXjFL0n=2v0QN1VX8M9Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Mar 26, 2025 at 10:40 AM Nikolay Shaplov <dhyan(at)nataraj(dot)su> wrote:
> Then please, listen to a reason. If we add isset_offset, it will make code
> inconsistent, because it follow same purpose as unreachable default_value in
> other non boolean options. Having two conflicting ways to do same thing is a
> bad design.

As Nathan also says, I'm not sure why we're obliged to do everything
one way or the other.

If you look at postgresql.conf, there are some settings where a
designated value is used to disable something -- for instance
temp_file_limit=-1 disables any limit on the size of temp files, and
backend_flush_after=0 also disables the corresponding behavior -- but
there are other settings where that doesn't make sense and we add a
separate switch e.g. ssl=off disables SSL, separate from any of the
other SSL-affecting GUCs, and archive_mode=off disables archiving
separately from the archive_command setting. Your argument seems to
lead to the conclusion that it is bad design, but I don't see it that
way. I think it would be bad design to add a separate
temp_file_limit_enabled=on/off setting when we could just use
temp_file_limit=-1 to mean that, and I think it would be equally a bad
idea to enable ssl based on some non-obvious criterion like whether
ssl_cert_file is set to the empty string. The cases are different, and
it's OK to treat them differently, IMHO.

> We can get rid of unreachable default_value in every option and use
> isset_offset everywhere to detect unset options, but this will give us lot's of
> extra flags in StdRdOptions,

I agree, that doesn't seem like a good idea.

> and more over we still have case when we have
> useful default_value. So code will have to either ignore default_value in
> option definition when author imply that he will use isset_offset flag later, or
> ignore isset_offset flag when processing option with useful default value. These
> two cases, I emphasize, are not directly stated in the code, they are implied
> somewhere in author's head, and you will never guess which one is in this
> case, until you finish reading, or one need to write a lot of comments.

I'm surprised that you think it will be hard to clarify this in the comments.

--
Robert Haas
EDB: http://www.enterprisedb.com

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Vladlen Popolitov 2025-03-26 14:53:35 Current master hangs under the debugger after Parallel Seq Scan (Linux, MacOS)
Previous Message Daniel Gustafsson 2025-03-26 14:47:21 Re: Windows: openssl & gssapi dislike each other