| From: | Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
|---|---|
| To: | Nikolay Shaplov <dhyan(at)nataraj(dot)su> |
| 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 19:23:50 |
| Message-ID: | 202601211907.po3lliqjtvsy@alvherre.pgsql |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On 2026-Jan-21, Nikolay Shaplov wrote:
> 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 further changed TERNARY_TRUE and so on to have a PG_ prefix also; it's
not impossible that there's userland code somewhere outside Postgres
that uses those symbol names, so let's avoid a collision.
> I've rebased the rest of patches, and the whole patchset is in the
> attachment.
I'm not a fan of how these commit messages are structured. You explain
the point of the whole patch series in the commit message for 0001, but
that one only adds some tests for the existing behavior. If I were to
commit that, it would make no sense in the overall Postgres commit
history. So I have squashed 0001 with 0002, and also with the fraction
of 0004 that includes tests for the feature in 0002. I think it's
strange to submit those tests in 0004, when the other half of the tests
are for the feature in 0003. I recommend to consider how you structure
your patch splits so that they would make sense in the Postgres commit
history assuming they are committed on separate days, and that there are
multiple other commits in between.
I do agree with Nathan that there seems to be little point in the
message saying this new type is different from Boolean. The user can
still say only "on" or "off". Even with your "alias" proposal, there
will only be a mechanism to let the system choose between those two
values, but it will still be one or the other. There's no provision to
have the system behave as if the user set the value to half.
Another thing I did is remove default_val for ternaries. As far as I
can see, it makes no sense. If your reloption defaults to either on or
off, then it's just a Boolean, right? It can no longer be unset,
because if you unset it, then it becomes the default.
Anyway, I have pushed the first part, after rewriting the commit
message. You can resubmit the rest after rebasing on the current tree.
I have also marked the commitfest item as committed. Please create a
new one for the next part.
Thanks,
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"El sentido de las cosas no viene de las cosas, sino de
las inteligencias que las aplican a sus problemas diarios
en busca del progreso." (Ernesto Hernández-Novich)
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Robert Haas | 2026-01-21 19:26:02 | Re: pg_plan_advice |
| Previous Message | Andres Freund | 2026-01-21 18:57:58 | Re: Having problems generating a code coverage report |