| From: | Nikolay Shaplov <dhyan(at)nataraj(dot)su> |
|---|---|
| To: | Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Chris Travers <chris(dot)travers(at)gmail(dot)com>, Timur Magomedov <t(dot)magomedov(at)postgrespro(dot)ru>, Nathan Bossart <nathandbossart(at)gmail(dot)com> |
| Subject: | Re: [PATCH] ternary reloption type |
| Date: | 2026-01-21 18:44:29 |
| Message-ID: | 3c4118a2-ec6b-4ea9-89af-233e1a2892ce@nataraj.su |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On 16.01.2026 18:14, Álvaro Herrera wrote:
> I took a quick look at 0001+0002 and I think it's quite reasonable.
> Here it is again with some minor fixups.
Good. I like ternary -> pg_ternary change. That is reasonable. And
postgres.h is better place for it then c.h.
> (I'm omitting the further
> patches for now, we can rebase them later.)
I've rebased the rest of patches, and the whole patchset is in the
attachment.
> I'm CCing Nathan as committer of the vacuum_truncate_set stuff which
> Nikolay so strongly disliked. Any objections to going with this
> approach?
Ok, will quote him and reply below in the this letter.
> > This could also be used for other options such as `vacuum_index_cleanup`
> > and `buffering`, but lets get the scaffolding in first.
> Part of me wonders if we should just modify the Boolean relopt
> implementation instead of using ternary only when needed.
Ternary option, with it's third optional option is quite complex.
I'd rather not bother postgres developer with this complexity if they
just want to add pure boolean option.
> >+ parsed = parse_bool(value, &b);
> >+ option->values.ternary_val = b ? TERNARY_TRUE : TERNARY_FALSE;
> >+ if (validate && !parsed)
> >+ ereport(ERROR,
> >+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> >+ errmsg("invalid value for ternary option \"%s\": %s",
> >+ option->gen->name, value)));
> Shouldn't this say "invalid value for boolean option"? IIUC the intent is
> for ternary to be exactly like bool, except it defaults to an "unset" value
> that can't be chosen by the user. In that sense, I think "ternary" is kind
> of a misnomer, but I wouldn't count this as an objection.
If you look at buffering option for example, you will see it does not
behave as pure boolean option.
"auto" value can be explicitly set by user.
Error reporting is not strong part of my patch, I would agree with that.
What if we give two different error reports in two different cases:
Give boolean-like error report when option does not have explicit alias
for third option, and more verbose
error message, listing all available options in the case when third
option have explicit alias?
like
"invalid value for boolean option \"%s\": %s",
and
"invalid value for option \"%s\": %s",
"valid values are 'on', 'off' and '%s'"
we may actually say nothing about option being ternary here
| Attachment | Content-Type | Size |
|---|---|---|
| v3a-0004-Extra-tests.patch | text/x-patch | 12.3 KB |
| v3a-0003-Add-alias-to-be-used-as-unset-state.patch | text/x-patch | 12.4 KB |
| v3a-0002-Introduce-ternary-reloptions.patch | text/x-patch | 12.2 KB |
| v3a-0001-Add-ternary-reloption-type.patch | text/x-patch | 4.8 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Matheus Alcantara | 2026-01-21 18:47:51 | Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row |
| Previous Message | Tom Lane | 2026-01-21 18:27:04 | Re: Remove no-op pull_var_clause flag |