Re: DROP relation IF EXISTS Docs and Tests - Bug Fix

On Tue, Aug 10, 2021 at 12:04 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Mar 9, 2021 at 10:09 AM David G. Johnston
> <david(dot)g(dot)johnston(at)gmail(dot)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.

drop_type.sgml
+ This includes domains, though they can be removed specifically
+ 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.