Re: DROP relation IF EXISTS Docs and Tests - Bug Fix - Mailing list pgsql-hackers
| From | David G. Johnston |
|---|---|
| Subject | Re: DROP relation IF EXISTS Docs and Tests - Bug Fix |
| Date | |
| Msg-id | CAKFQuwb=buOPOoCL+i4Qen9aOmbRO+pOBZ9pUStPP58YwO_-dQ@mail.gmail.com Whole thread Raw |
| In response to | Re: DROP relation IF EXISTS Docs and Tests - Bug Fix (Robert Haas <robertmhaas@gmail.com>) |
| Responses |
Re: DROP relation IF EXISTS Docs and Tests - Bug Fix
(Robert Haas <robertmhaas@gmail.com>)
|
| List | pgsql-hackers |
On Tue, Aug 10, 2021 at 12:04 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Mar 9, 2021 at 10:09 AM David G. Johnston
<david.g.johnston@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,
Thank you for the feedback.
So, the only change here is deleting the word "essentially."
I do tend to find this wishy-washy language to be more annoying than the community at large.
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 would at least like to see this added in response to the various bug reports that found the shared namespace among types, and the fact that it causes an error, to be a surprise.
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.
I'd say the Bug # 16492 tests warrant keeping independent of the opinion that demonstrating the complicated interplay between 10+ SQL commands isn't worth the test suite time. I'd say that probably half of the tests are demonstrating non-intuitive behavior from my perspective. The bug test noted above plus the one the demonstration that a table in the non-first schema in a search_path will not prevent a create <type> command from succeeding but will cause a DROP <type non-table> IF EXISTS to error out. Does it need to test all 5 types, probably not, but it should at least test DROP VIEW IF EXISTS test_rel_exists.
What about the inherent confusion that having both DROP DOMAIN when DROP TYPE will also drop domains? The doc change for that doesn't really fit into your buckets. It would include:
drop_domain.sgml
+ This duplicates the functionality provided by
+ <xref linkend="sql-droptype"/> but restricts
+ the type's type to domain.
+ <xref linkend="sql-droptype"/> but restricts
+ the type's type to domain.
drop_type.sgml
+ This includes domains, though they can be removed specifically
+ by using the <xref linkend="sql-dropdomain"/> command.
+ by using the <xref linkend="sql-dropdomain"/> command.
Adding sql-droptype to "See Also" on all the domain related command pages as well.
After looking at this again I will say I do agree that the procedural nature of the doc changes for the main issue were probably overkill and a "oh-by-the-way" note as to the fact that we ERROR on a namespace conflict would address that concern in a user-facing way adequately. Looking back and what I went through to put the test script together I don't regret doing the work and feel that someone like myself would benefit from its existence as a whole. It's more useful than a README that would communicate the same information.
David J.
pgsql-hackers by date: