On Tue, Mar 9, 2021 at 10:09 AM David G. Johnston
<david.g.johns...@gmail.com> wrote:
> Frankly, I am hoping for a bit more constructive feedback and even
> collaboration from a committer, specifically Tom, on this one given the
> outstanding user complaints received on the topic, our disagreement regarding
> fixing it (which motivates the patch to better document and add tests), and
> professional courtesy given to a fellow consistent community contributor.
>
> So, no, making it just go away because one of the dozens of committers can’t
> make time to try and make it work doesn’t sit well with me. If a committer
> wants to actively reject the patch with an explanation then so be it.
I have reviewed this patch and my opinion is that we should reject it,
because in my opinion, the documentation changes are not improvements,
and there is no really clear need for the regression test additions.
According to my view of the situation, there are three kinds of
changes in this patch. The first set of hunks make minor wording
adjustments. Typical is this:
<command>CREATE DOMAIN</command> creates a new domain. A domain is
- essentially a data type with optional constraints (restrictions on
+ a data type with optional constraints (restrictions on
the allowed set of values).
So, the only change here is deleting the word "essentially." Now, it's
possible that if someone different had written the original text, they
might have chosen to leave that word out. Personally, I would have
chosen to include it, but it's a judgement call. However, I find it
extremely difficult to imagine that anybody will be confused because
of the presence of that word there.
- The domain name must be unique among the types and domains existing
- in its schema.
+ The domain name must be unique among all types (not just domains)
+ existing in its schema.
Similarly here. It is arguable which way the text reads better, but
the stated purpose of the patch is to make the behavior more clear,
and I cannot imagine someone reading the existing text and getting
confused, and then reading the patched text and being not confused.
- Do not throw an error if the domain does not exist. A notice is issued
- in this case.
+ This parameter instructs <productname>PostgreSQL</productname> to search
+ for the first instance of any type with the provided name.
+ If no type is found a notice is issued and the command ends.
+ If a type is found then one of two things will happen:
+ if the type is a domain it is dropped, otherwise the command fails.
This is the second kind of hunk that is present in this patch. There
are a whole bunch that are very similar, adjusting the documentation
for various object types. I appreciate that it does confuse users
sometimes that a DROP IF NOT EXISTS command can still fail for some
other reason, so maybe we should try to clarify that in some way, but
I find this explanation to be too complex and technical to be helpful.
If we feel it's necessary to be more clear here, I'd suggest keeping
the existing text and adding a sentence like: "Note that the command
can still fail for other reasons; for example, it is an error if a
type with the provided name exists but is not a domain."
I feel that it's unnecessary to try to clarify what the behavior of
the command is relative to search_path, but if it were agreed that we
need to do so, I still would not like this way of doing it. First, you
never actually use the term "search_path". I expect a lot of people
would be confused by the reference to searching "for the first
instance" because they won't know what is being searched. Second, this
entire bit of text is inside the description of "IF NOT EXISTS" but
the behavior in question is mostly stuff that applies whether or not
"IF NOT EXISTS" is specified. Moreover, it's not only not specific to
IF NOT EXISTS, it's not even specific to this particular command. The
behavior of looking through the search_path for the first instance of
some object type is one that we use practically everywhere in all
kinds of situations. I think we are going to go crazy if we try to
re-explain that in every place where it might be relevant.
The final portion of the patch adds new regression tests. I'm not
going to say that this is completely without merit, because it can be
useful to know if some patch changes the behavior, but I guess I don't
really see a whole lot of value in it, either. It's easy to end up
with a ton of tests that take up a little bit of time every time
someone runs 'make check' but only ever find behavior changes that the
developer was intentionally trying to make. I think it's possible that
these changes would end up falling into that category. I wouldn't
complaining about them getting committed, or about committing them
myself, if there were a chorus of people convinced that they were
worth having, but there isn't, and I don't find them valuable enough
myself to justify a commit.
--
Robert Haas
EDB: http://www.enterprisedb.com
