On 21.01.2026 22:23, Álvaro Herrera wrote:
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 guess that comes from my frustration with big reloptions patch. I've been looking
for a way to make iy more easy to read and understand. My thought was that if
I split them into logical "layers" showing the development of the patch's idea, it
would be more easy to understand them as a whole. And they were intended
to be squashed before commit, may be I should state that more clearly.
I think it's
strange to submit those tests in 0004, when the other half of the tests
are for the feature in 0003.
Yeah, here you are right, I should add tests both to 0003 and 0004. I did not think
they can be committed separately
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.
Second part of ternary options patch is consist of one piece, so now it is not a problem,
and as for big reloptions patch, I think we can discuss it later. I doubt it is readable
when it is provided in single piece.
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.
For ternary options with explicit "third" value, I added another error message, it says nothing
about option type, it just lists possible values. This can be good solution.
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.
For the part you've committed that is correct. But with explicit "third" option, you can't tell
for sure which value is default. Like:
prefer_XXXX_optimization: yes/no/never
It can be implemented as ternary option, but default value here can be "yes".
I would not try to predict what behavior option developer will need, and try to provide all
possibilities.
That's why I put default value back. But I will not be much upset if you still decide to remove it.
Or I can remove it myself if you insist. We can add it later when someone runs into this "never" case.
Anyway, I have pushed the first part, after rewriting the commit
message.
Thanks! Now the world is better place, from my point of view.
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.
I've reworked second part of the patch, It is in the attachment, and I am going to create new commitfest record for it. Thank you for your work.
