Avoid schema changes with IF NOT EXISTS #14143
Conversation
| statement error | ||
| DROP VIEW IF EXISTS integers | ||
| ---- | ||
| Existing object integers is of type Table, trying to replace with type View |
There was a problem hiding this comment.
"trying to replace" feels out of place in a DROP statement
There was a problem hiding this comment.
Agreed. This was the existing message in DuckSchemaEntry::DropEntry (not changed by this PR), I just added a test for it (to show that what we do now for create matches with what was already happening on drop). Happy to change the message in DuckSchemaEntry::DropEntry too if you're ok with this being in the scope of this PR. What should the message look like in your opinion?
There was a problem hiding this comment.
Thanks! Yea the changes make sense to me, good catch
Probably something like:
Existing object "integers" is of type Table, unable to drop View for the DROP
Unable to create View "integers", an object of type Table by that name already exists for the CREATE
I hope we have enough context to distinguish between the create and the drop
There was a problem hiding this comment.
Or actually, we probably want to look at Postgres for the error messages
postgres=# drop view if exists integers;
ERROR: "integers" is not a view
HINT: Use DROP TABLE to remove a table.
postgres=# create view integers as select 42;
CREATE VIEW
postgres=# create table if not exists integers(a integer);
NOTICE: relation "integers" already exists, skipping
CREATE TABLE
Hmm... the earlier messages I send are close enough to this, we don't really do the ERROR, NOTICE, HINT stuff anywhere else
There was a problem hiding this comment.
Based on your feedback above I pushed 8e8a019.
An interesting realization from your Postgres comparison is that create table if not exists does not throw an exception if the existing object is not a table. Instead it ignores the comment. I've taken that same approach now.
For DROP ... IF EXISTS I've amended the error message to be slightly more correct. Doing a full alignment with Postgres error messages is out of scope for this PR though (I gave it a start and found plenty of inconsistencies as these errors can be thrown from multiple places).
| @@ -14,6 +14,15 @@ CREATE TABLE IF NOT EXISTS integers2 AS SELECT 42 | |||
| statement ok | |||
| CREATE TABLE IF NOT EXISTS integers2 AS SELECT 42 | |||
|
|
|||
| # similar to Postgres, treat as NOOP if the entry exists but the type is different | |||
| statement ok | |||
| CREATE VIEW IF NOT EXISTS integers2 AS SELECT 42 | |||
There was a problem hiding this comment.
@Mytherin how do you feel about this?
Postgres has this behavior (minus a warning/notice message):
postgres=# create table if not exists integers(a integer);
NOTICE: relation "integers" already exists, skipping
CREATE TABLE
I'd say this should fail because a VIEW by this name doesn't exist, which sounds like what the user expects when this passes to me
There was a problem hiding this comment.
Also note that this (not throwing an exception) is the current behavior of DuckDB as well, there are no actual behavioral changes by this PR except for fixing the issue where the version is no longer incremented on these NOOP commands.
|
Thanks! |
Avoid schema changes with IF NOT EXISTS (duckdb/duckdb#14143) add method to check whether julia connection is open (duckdb/duckdb#14195)
Avoid schema changes with IF NOT EXISTS (duckdb/duckdb#14143) add method to check whether julia connection is open (duckdb/duckdb#14195) Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
We've noticed that repeated calls of
CREATE TABLE IF NOT EXISTSfor the same table were causing catalog versions to go up (causing rebinds for other queries). As ORMs are frequently using this pattern to ensure that a table / schema exists before inserting data (i.e. packaging the CREATE TABLE IF NOT EXISTS into the same TX as the INSERT statement), this led to rebinds happening all the time.