Re: BUG #16492: DROP VIEW IF EXISTS error

On Mon, Jun 15, 2020 at 8:06 AM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:

> Huh? The lack of concrete examples makes it difficult to take seriously
>> your defense of the current behavior.
>>
>
> CREATE SCHEMA s1;
> CREATE SCHEMA s2;
>
> CREATE VIEW s1.rel AS SELECT 1;
> CREATE TABLE s2.rel (a int, b int)
>
> SET SEARCH_PATH TO s1, s2;
>
> DROP TABLE IF EXISTS rel;
> CREATE TABLE rel(a int, b int);
>
> This is a synthetic example. But this case shows so the behaviour is same,
> and search_path should not be calculated. This script fails in both cases
> (both variants of DROP TABLE IF EXISTS) with same error messages like
> example when search_path will not be used.
>

So the presence of a view of the name rel in the search_path s1 namespace
is matched against the drop table non-schema specific name rel and in both
cases the code stops looking and returns saying that it was unable to find
a table named rel?

**Given that it happens in both scenarios that wouldn't prevent making this
change. The create table still cannot execute successfully whether the if
exists actually errors out or does nothing.**

I cannot test the alternative so cannot confirm your "with same error
message" statement...but presently the drop table fails in your example
which is what I find to be more important. If the drop fails today the
create should fail today. That way if the drop stops failing the create
will continue to do so and the outcome of the script is the same. If the
drop doesn't fail the create might fail or it might not. I've already
stated that a change here presumes not caring about keeping the location
and message of the resulting failure the same, just whether a failure
occurs or not.

Your conclusion that "search_path will not be used" is wrong since that is
not possible - the only way to find anything in the system is via the
search_path. This is easily demonstrated since adding "DROP VIEW s1.rel;"
after the "SET search_path" results in the script working (kinda, see
below).

If you are saying that only the first search_path containing the named
relation in the namespace is considered that seems like debatable behavior
- and in any case is likewise neither undocumented nor intuitive. I would
indeed find that to be desirable as a safety feature and if you are saying
that the fixed if exists behavior would already work that way that is one
oh-by-the-way that I hadn't considered that "just works" - though it is
independent of the if exists behavior. So much so that I'd also say that
what you just showed is a problem in its own right - though arguably one of
the users own making for inconsistent usage of schema prefixes.

SET search_path to s1, s2
DROP VIEW IF EXISTS s1.rel; -- drops s1.rel so the if exists bug doesn't
come into play
DROP TABLE IF EXISTS rel; -- drops s2.rel, again dropping happens the same
in either case
CREATE TABLE rel; -- creates s1.rel

If the expectation is that the user is dropping and creating tables that
are supposed to have been logically the same (in the same namespace) the
fact that we dropped the table in s2 but created the new one is s1 is
undesirable.

Again, regardless of the desirability of the above it behaves identically
no matter how IF EXISTS behaves so it's not a reason to avoid fixing the
bug.

For the example above I would have expected the fixed IF EXISTS to be:
SET search_path to s1, s2;
DROP TABLE IF EXISTS rel; -- drops s2.rel
CREATE TABLE rel; -- fails to create table s1.rel do to namespace collision
but it still dropped s2.rel.

If that was/is the outcome then I'd say that indeed this is a concern
because now the namespace checks in s1 is preventing a valid drop from
happening in s2. But then we are back to the fact that the absence of a
namespace collision in s1 results in exactly that behavior. Though if all
of this is being done in a transaction then the dropping of s2.rel will
just be rolled back when s1.rel fails to be created. And apparently this
isn't the actual behavior, which itself is actually less broken in the face
of this change (i.e., I have no problem with an "ambiguous relation name"
error being raised here).

I'm now more solidly behind the idea of only doing this in v14 but this
doesn't really change my feelings on the fact that the existing behavior is
a bug. It prevents using a valid set of drop commands in series with the
only potentially problematic cases being those where the user is careless
with namespaces and doesn't do related things in a transaction. Improving
the logic and introducing the concept of "ambiguous relation name" would
increase the complexity of the fix but would be more user-friendly and
hopefully acceptable by the hacker community.

Maybe a good first step is to agree on what a documentation and possible
code comment would look like describing the current behavior since we are
going to need that for the current releases regardless.

David J.