Re: DROP relation IF EXISTS Docs and Tests - Bug Fix - Mailing list pgsql-hackers

On Tue, Aug 10, 2021 at 5:53 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>> 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
between10+ SQL commands isn't worth the test suite time.  I'd say that probably half of the tests are demonstrating
non-intuitivebehavior from my perspective.  The bug test noted above plus the one the demonstration that a table in the
non-firstschema 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
VIEWIF EXISTS test_rel_exists. 

It's not the function of the regression tests to demonstrate behavior.
Nobody says "I wonder how the system works, I guess I'll go look at
the regression tests," or at least very few people. The job of the
regression test is to let us know when things get broken by future
changes to the source code, or in other words, to find regressions.

> What about the inherent confusion that having both DROP DOMAIN when DROP TYPE will also drop domains?  The doc change
forthat 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.
>
> drop_type.sgml
> +   This includes domains, though they can be removed specifically
> +   by using the <xref linkend="sql-dropdomain"/> command.

Maybe we could add a sentence to the end of the DROP DOMAIN synopsis,
like: Since a domain is a kind of type, they can alternatively be
dropped using DROP TYPE.

And for DROP TYPE, we could say something like: Since a domain is a
kind of type, it is possible to drop a domain using this command
rather than DROP DOMAIN.

I think that's similar to your proposal, but I prefer it because I
think the duplicate-but-restrict language makes it sound kind of dumb
that DROP DOMAIN exists at all, and I don't think it's dumb that we
have DROP commands matching the CREATE commands that we also have.

> Adding sql-droptype to "See Also" on all the domain related command pages as well.

I probably wouldn't do this, but the notes above could include cross-links.

> After looking at this again I will say I do agree that the procedural nature of the doc changes for the main issue
wereprobably overkill and a "oh-by-the-way" note as to the fact that we ERROR on a namespace conflict would address
thatconcern in a user-facing way adequately.  Looking back and what I went through to put the test script together I
don'tregret doing the work and feel that someone like myself would benefit from its existence as a whole.  It's more
usefulthan a README that would communicate the same information. 

Yeah. I tend to feel like this is the kind of thing where it's not
likely to be 100% obvious to users how it all works no matter what we
put in the documentation, and that some of the behavior of a system
has to be learned just by trying out the system. Now, that doesn't
mean that we should be immune to ideas about how to improve it, just
that someone can always have a question that isn't answered there.

--
Robert Haas
EDB: http://www.enterprisedb.com