On Mon, Dec 14, 2020 at 1:54 PM Bharath Rupireddy <bharath.
rupireddyforpostg...@gmail.com> wrote:
> On Mon, Dec 14, 2020 at 11:52 AM Michael Paquier <mich...@paquier.xyz>
wrote:
> > On Mon, Dec 14, 2020 at 03:15:12PM +0900, Michael Paquier wrote:
> > > Please note that this case fails with your patch, but the presence of
> > > IF NOT EXISTS should ensure that we don't fail and issue a NOTICE
> > > instead, no?
>
> Thanks for the use case. The provided use case (or for that matter any
> use case with explain analyze ctas if-not-exists) fails if the
> relation already exists. It happens on the master branch, please have
> a look at tests [1]. You are right in saying that whether it is
> explain/explain analyze ctas if there is if-not-exists we should issue
> notice instead of error as with plain ctas.
>
> Do we want to fix this behaviour for explain/explain analyze ctat with
> if-not-exists cases? Thoughts?
>
> If yes, we could change the code in ExplainOneUtility() such that we
> check relation existence before rewrite/planning, issue notice and
> return. Then. the user sees a notice and an empty plan as we are
> returning from ExplainOneUtility(). Is it okay to show a warning and
> an empty plan to the user? Thoughts?
>
> >> Taking this case specifically (OK, I am playing with
> > > the rules a bit to insert data into the relation itself, still), this
> > > query may finish by adding tuples to the table whose creation should
> > > have been bypassed but the query got executed and inserted tuples.
>
> IIUC, with the use case provided, the tuples will not be inserted as
> the query later fails (and the txn gets aborted) if the relation
> exists.
>
> > > That's one example of behavior that may be confusing. There may be
> > > others, but it seems to me that it may be simpler to execute or even
> > > plan the query at all if the relation already exists.
> >
> > Er.. Sorry. I meant here to *not* execute or even *not* plan the
> > query at all if the relation already exists.
>
> +1 to not plan and execute the query at all if the relation which
> ctas/cmv is trying to create already exists.
Posting a v2 patch after modifying the new function CheckRelExistenceInCTAS()
a bit as suggested earlier.
The behavior of the ctas/cmv, in case the relation already exists is as
shown in [1]. The things that have been changed with the patch are: 1) In
any case we do not rewrite or plan the select part if the relation already
exists 2) For explain ctas/cmv (without analyze), now the relation
existence is checked early and the error is thrown as highlighted in [1].
With patch, there is no behavioral change(from that of master branch) in
explain analyze ctas/cmv with if-not-exists i.e. error is thrown not the
notice.
Thoughts?
[1]
With patch:
postgres=# create table foo as select 1;
ERROR: relation "foo" already exists
postgres=# create table if not exists foo as select 1;
NOTICE: relation "foo" already exists, skipping
CREATE TABLE AS
postgres=# explain analyze create table foo as select 1;
ERROR: relation "foo" already exists
postgres=# explain analyze create table if not exists foo as select 1;
ERROR: relation "foo" already exists
postgres=# explain create table foo as select 1;
ERROR: relation "foo" already exists
postgres=# explain create table if not exists foo as select 1;
ERROR: relation "foo" already exists
On master/without patch:
postgres=# create table foo as select 1;
ERROR: relation "foo" already exists
postgres=# create table if not exists foo as select 1;
NOTICE: relation "foo" already exists, skipping
CREATE TABLE AS
postgres=# explain analyze create table foo as select 1;
ERROR: relation "foo" already exists
postgres=# explain analyze create table if not exists foo as select 1;
ERROR: relation "foo" already exists
postgres=# explain create table foo as select 1;
QUERY PLAN
------------------------------------------
Result (cost=0.00..0.01 rows=1 width=4)
(1 row)
postgres=# explain create table if not exists foo as select 1;
QUERY PLAN
------------------------------------------
Result (cost=0.00..0.01 rows=1 width=4)
(1 row)
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From ae963a1e945bad1932221990d72b09deac42dc20 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Thu, 17 Dec 2020 14:13:27 +0530
Subject: [PATCH v2] Fail Fast In CTAS/CMV If Relation Already Exists
Currently, for CTAS or CREATE MATERIALIZED VIEW(CMV) without
IF-NOT-EXISTS clause, the existence of the relation (either
table or materialized view) gets checked during execution and an
error is thrown there. All the unnecessary rewrite and planning
for the SELECT part of the query have happened just to fail
later. However, if IF-NOT-EXISTS clause is present, then a notice
is issued and returned immediately without rewrite and planning
further. This seems somewhat inconsistent.
This patch propose to check the relation existence early in
ExecCreateTableAs() as well as in ExplainOneUtility() and
throw an error in case it exists already to avoid unnecessary
rewrite, planning and execution of the SELECT part.
---
src/backend/commands/createas.c | 69 ++++++++++++++++++++++++++-------
src/backend/commands/explain.c | 12 ++++++
src/include/commands/createas.h | 2 +
3 files changed, 69 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 6bf6c5a310..2f4d865494 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -238,22 +238,21 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
List *rewritten;
PlannedStmt *plan;
QueryDesc *queryDesc;
+ bool rel_exists;
- if (stmt->if_not_exists)
- {
- Oid nspid;
-
- nspid = RangeVarGetCreationNamespace(stmt->into->rel);
+ /*
+ * Check if the relation that is going to get created exists already. If
+ * yes and there is no if-not-exists clause, then emit error early so that
+ * we can avoid planning for the SELECT part unnecessarily which happens
+ * otherwise only to fail later.
+ *
+ * If there is if-not-exists clause, we issue a NOTICE and return from here
+ * with invalid object address.
+ */
+ rel_exists = CheckRelExistenceInCTAS(stmt, false);
- if (get_relname_relid(stmt->into->rel->relname, nspid))
- {
- ereport(NOTICE,
- (errcode(ERRCODE_DUPLICATE_TABLE),
- errmsg("relation \"%s\" already exists, skipping",
- stmt->into->rel->relname)));
- return InvalidObjectAddress;
- }
- }
+ if (stmt->if_not_exists && rel_exists)
+ return InvalidObjectAddress;
/*
* Create the tuple receiver object and insert info it will need
@@ -400,6 +399,48 @@ GetIntoRelEFlags(IntoClause *intoClause)
return flags;
}
+/*
+ * CheckRelExistenceInCTAS --- check whether a specified relation in CTAS
+ * already exists.
+ *
+ * We do this before any planning occurs for the SELECT part to avoid planning
+ * cost. If the relation exists: 1) issue NOTICE and return true for plain i.e.
+ * non-explain queries with if-not-exists clause. 2) emit an ERROR for both
+ * plain queries without if-not-exists clause and explain queries irrespective
+ * of if-not-exists clause. Return false if the relation does not exist.
+ */
+bool CheckRelExistenceInCTAS(CreateTableAsStmt *ctas, bool is_explain)
+{
+ Oid nspid;
+
+ nspid = RangeVarGetCreationNamespace(ctas->into->rel);
+
+ if (get_relname_relid(ctas->into->rel->relname, nspid))
+ {
+ /*
+ * Issue notice in case if-not-exists clause is present, otherwise an
+ * error is emitted. However for explains we throw error irrespective
+ * of if-not-exists clause, as we do not want to show a notice and an
+ * empty plan in the output which can happen if a notice is issued.
+ */
+ if (!is_explain && ctas->if_not_exists)
+ ereport(NOTICE,
+ (errcode(ERRCODE_DUPLICATE_TABLE),
+ errmsg("relation \"%s\" already exists, skipping",
+ ctas->into->rel->relname)));
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_TABLE),
+ errmsg("relation \"%s\" already exists",
+ ctas->into->rel->relname)));
+
+ return true;
+ }
+
+ /* Relation does not exist. */
+ return false;
+}
+
/*
* CreateIntoRelDestReceiver -- create a suitable DestReceiver object
*
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 43f9b01e83..9238a4a3a6 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -435,6 +435,18 @@ ExplainOneUtility(Node *utilityStmt, IntoClause *into, ExplainState *es,
CreateTableAsStmt *ctas = (CreateTableAsStmt *) utilityStmt;
List *rewritten;
+ /*
+ * Check if the relation that is going to get created exists already.
+ * If yes, then emit error early so that we can avoid planning for
+ * the SELECT part unnecessarily which happens otherwise only to fail
+ * later.
+ *
+ * We intentionally ignore the return value of CheckRelExistenceInCTAS
+ * as it returns for explains only when the relation does not exist
+ * otherwise it throws an error and does not come here.
+ */
+ (void) CheckRelExistenceInCTAS(ctas, true);
+
rewritten = QueryRewrite(castNode(Query, copyObject(ctas->query)));
Assert(list_length(rewritten) == 1);
ExplainOneQuery(linitial_node(Query, rewritten),
diff --git a/src/include/commands/createas.h b/src/include/commands/createas.h
index 7629230254..33d4d9701f 100644
--- a/src/include/commands/createas.h
+++ b/src/include/commands/createas.h
@@ -29,4 +29,6 @@ extern int GetIntoRelEFlags(IntoClause *intoClause);
extern DestReceiver *CreateIntoRelDestReceiver(IntoClause *intoClause);
+extern bool CheckRelExistenceInCTAS(CreateTableAsStmt *ctas, bool is_explain);
+
#endif /* CREATEAS_H */
--
2.25.1
