Add missing CREATE TABLE IF NOT EXISTS table_name AS EXECUTE query;
Hi,
The first example below works while the second one is a syntax error
despite that the documentation
(https://www.postgresql.org/docs/11/sql-createtableas.html) makes it
seem like both should be valid. I do not see any reason to not support
CTAS with both IF NOT EXISTS and EXECUTE at the same time so i am
guessing that this was an oversight.
I have attached a patch which fixes this. What do you think? Should I
add a new test case for this or is the change simple enough to not
require any new test?
# CREATE TABLE IF NOT EXISTS a AS SELECT 1;
SELECT 1
# PREPARE q AS SELECT 1;
PREPARE
Time: 0.209 ms
foo=# CREATE TABLE IF NOT EXISTS a AS EXECUTE q;
ERROR: syntax error at or near "EXECUTE"
LINE 1: CREATE TABLE IF NOT EXISTS a AS EXECUTE q;
^
Andreas
Attachments:
ctas-exec-ifne-v1.sqlapplication/sql; name=ctas-exec-ifne-v1.sqlDownload
From eff8446b04ec6640045a4d92efec4ca09e0ff5b4 Mon Sep 17 00:00:00 2001
From: Andreas Karlsson <andreas@proxel.se>
Date: Wed, 6 Feb 2019 03:17:57 +0100
Subject: [PATCH] Add missing IF NOT EXIST support for CTAS with prepared
statments
---
src/backend/parser/gram.y | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index c1faf4152c..524c5b762a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10696,11 +10696,29 @@ ExecuteStmt: EXECUTE name execute_param_clause
ctas->into = $4;
ctas->relkind = OBJECT_TABLE;
ctas->is_select_into = false;
+ ctas->if_not_exists = false;
/* cram additional flags into the IntoClause */
$4->rel->relpersistence = $2;
$4->skipData = !($9);
$$ = (Node *) ctas;
}
+ | CREATE OptTemp TABLE IF_P NOT EXISTS create_as_target AS
+ EXECUTE name execute_param_clause opt_with_data
+ {
+ CreateTableAsStmt *ctas = makeNode(CreateTableAsStmt);
+ ExecuteStmt *n = makeNode(ExecuteStmt);
+ n->name = $10;
+ n->params = $11;
+ ctas->query = (Node *) n;
+ ctas->into = $7;
+ ctas->relkind = OBJECT_TABLE;
+ ctas->is_select_into = false;
+ ctas->if_not_exists = true;
+ /* cram additional flags into the IntoClause */
+ $7->rel->relpersistence = $2;
+ $7->skipData = !($12);
+ $$ = (Node *) ctas;
+ }
;
execute_param_clause: '(' expr_list ')' { $$ = $2; }
--
2.11.0
On Wed, Feb 06, 2019 at 03:20:41AM +0100, Andreas Karlsson wrote:
The first example below works while the second one is a syntax error despite
that the documentation
(https://www.postgresql.org/docs/11/sql-createtableas.html) makes it seem
like both should be valid. I do not see any reason to not support CTAS with
both IF NOT EXISTS and EXECUTE at the same time so I am guessing that this
was an oversight.
Agreed, good catch.
I have attached a patch which fixes this. What do you think? Should I add a
new test case for this or is the change simple enough to not require any new
test?
A test case in select_into.sql would be nice. Should we back-patch
that? It seems to me that this qualifies as a bug fix, and I don't
see a reason to not support it knowing that we have the infrastructure
for that.
--
Michael
On 2/6/19 12:49 PM, Michael Paquier wrote:
A test case in select_into.sql would be nice. Should we back-patch
that? It seems to me that this qualifies as a bug fix, and I don't
see a reason to not support it knowing that we have the infrastructure
for that.
I added a test in create_table.sql where the test for the other form of
CTAS IF NOT EXISTS is.
I have no idea if something like this should be back patched. I will add
it to the commit fest either way so it is not lost.
Andreas
Attachments:
ctas-exec-ifne-v2.sqlapplication/sql; name=ctas-exec-ifne-v2.sqlDownload
From 9c0333dab1d504056ba50188712189f4ddbd8aaa Mon Sep 17 00:00:00 2001
From: Andreas Karlsson <andreas@proxel.se>
Date: Wed, 6 Feb 2019 03:17:57 +0100
Subject: [PATCH] Add missing IF NOT EXIST support for CTAS with prepared
statments
---
src/backend/parser/gram.y | 18 ++++++++++++++++++
src/test/regress/expected/create_table.out | 8 ++++++++
src/test/regress/sql/create_table.sql | 7 +++++++
3 files changed, 33 insertions(+)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index c1faf4152c..524c5b762a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10696,11 +10696,29 @@ ExecuteStmt: EXECUTE name execute_param_clause
ctas->into = $4;
ctas->relkind = OBJECT_TABLE;
ctas->is_select_into = false;
+ ctas->if_not_exists = false;
/* cram additional flags into the IntoClause */
$4->rel->relpersistence = $2;
$4->skipData = !($9);
$$ = (Node *) ctas;
}
+ | CREATE OptTemp TABLE IF_P NOT EXISTS create_as_target AS
+ EXECUTE name execute_param_clause opt_with_data
+ {
+ CreateTableAsStmt *ctas = makeNode(CreateTableAsStmt);
+ ExecuteStmt *n = makeNode(ExecuteStmt);
+ n->name = $10;
+ n->params = $11;
+ ctas->query = (Node *) n;
+ ctas->into = $7;
+ ctas->relkind = OBJECT_TABLE;
+ ctas->is_select_into = false;
+ ctas->if_not_exists = true;
+ /* cram additional flags into the IntoClause */
+ $7->rel->relpersistence = $2;
+ $7->skipData = !($12);
+ $$ = (Node *) ctas;
+ }
;
execute_param_clause: '(' expr_list ')' { $$ = $2; }
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 100fa53ab0..553d3c372d 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -261,6 +261,14 @@ ERROR: relation "as_select1" already exists
CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
NOTICE: relation "as_select1" already exists, skipping
DROP TABLE as_select1;
+PREPARE select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
+CREATE TABLE as_select1 AS EXECUTE select1;
+CREATE TABLE as_select1 AS EXECUTE select1;
+ERROR: relation "as_select1" already exists
+CREATE TABLE IF NOT EXISTS as_select1 AS EXECUTE select1;
+NOTICE: relation "as_select1" already exists, skipping
+DROP TABLE as_select1;
+DEALLOCATE select1;
-- create an extra wide table to test for issues related to that
-- (temporarily hide query, to avoid the long CREATE TABLE stmt)
\set ECHO none
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 22a3d90101..0e796a3020 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -277,6 +277,13 @@ CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
DROP TABLE as_select1;
+PREPARE select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
+CREATE TABLE as_select1 AS EXECUTE select1;
+CREATE TABLE as_select1 AS EXECUTE select1;
+CREATE TABLE IF NOT EXISTS as_select1 AS EXECUTE select1;
+DROP TABLE as_select1;
+DEALLOCATE select1;
+
-- create an extra wide table to test for issues related to that
-- (temporarily hide query, to avoid the long CREATE TABLE stmt)
\set ECHO none
--
2.11.0
On Wed, Feb 06, 2019 at 04:23:44PM +0100, Andreas Karlsson wrote:
I added a test in create_table.sql where the test for the other form of CTAS
IF NOT EXISTS is.
Okay, we can live with that. Instead of a scan on pg_class, I would
have switched to a more simple thing like that for the PREPARE query:
PREPARE select1 AS SELECT 1 AS a;
Then it would be good to add a scan on the created relation after
running all the CREATE TABLE queries to make sure that it remains with
only one tuple.
I have no idea if something like this should be back patched. I will add it
to the commit fest either way so it is not lost.
I'd like to get that addressed before working on the other item
reshuffling the CTAS relation creation. Let's wait for a couple of
days and see if folks have objections, and then revisit it at the
beginning of next week. CTAS IF NOT EXISTS is supported down to
9.5 and is documented as such, and my proposal is to back-patch
accordingly.
--
Michael
On Thu, Feb 07, 2019 at 10:31:35AM +0900, Michael Paquier wrote:
I'd like to get that addressed before working on the other item
reshuffling the CTAS relation creation. Let's wait for a couple of
days and see if folks have objections, and then revisit it at the
beginning of next week. CTAS IF NOT EXISTS is supported down to
9.5 and is documented as such, and my proposal is to back-patch
accordingly.
Let's wait a bit more than the beginning of this week. I forgot about
this week's minor release, and it is too late to do something about
this report now, so we will have to wait until the release had
happened.
--
Michael
On Mon, Feb 11, 2019 at 09:53:59PM +0900, Michael Paquier wrote:
Let's wait a bit more than the beginning of this week. I forgot about
this week's minor release, and it is too late to do something about
this report now, so we will have to wait until the release had
happened.
OK, committed down to 9.5.
Another thing I have noticed is the following, which is kind of funky
(just rinse and repeat once):
=# EXPLAIN ANALYZE CREATE TABLE IF NOT EXISTS ac AS SELECT 1;
ERROR: 42P07: relation "ac" already exists
LOCATION: heap_create_with_catalog, heap.c:1111
The issue here is that we check for IF NOT EXISTS at the high level of
ExecCreateTableAs, however EXPLAIN takes the lower path of
create_ctas_internal() which enforces if_not_exists to false when
building the CreateStmt object to create the relation. This brings
out a more interesting issue: how should an EXPLAIN behave in this
case? It has nothing to output as the relation already exists.
--
Michael
On 15/02/2019 09.14, Michael Paquier wrote:
On Mon, Feb 11, 2019 at 09:53:59PM +0900, Michael Paquier wrote:
Let's wait a bit more than the beginning of this week. I forgot about
this week's minor release, and it is too late to do something about
this report now, so we will have to wait untOK, committed down to 9.5.
Thanks!
Another thing I have noticed is the following, which is kind of funky
(just rinse and repeat once):
=# EXPLAIN ANALYZE CREATE TABLE IF NOT EXISTS ac AS SELECT 1;
ERROR: 42P07: relation "ac" already exists
LOCATION: heap_create_with_catalog, heap.c:1111The issue here is that we check for IF NOT EXISTS at the high level of
ExecCreateTableAs, however EXPLAIN takes the lower path of
create_ctas_internal() which enforces if_not_exists to false when
building the CreateStmt object to create the relation. This brings
out a more interesting issue: how should an EXPLAIN behave in this
case? It has nothing to output as the relation already exists.
Yeah, noticed the same thing myself while refactoring the CTAS code, but
I guess the output could be like the current output for "EXPLAIN ANALYZE
<CTAS> WITH NO DATA;", i.e. a top plan with "(never executed)" (see
below for an example).
# EXPLAIN ANALYZE CREATE TABLE ac AS SELECT 1 WITH NO DATA;
QUERY PLAN
-----------------------------------------------------------
Result (cost=0.00..0.01 rows=1 width=4) (never executed)
Planning Time: 0.125 ms
Execution Time: 17.046 ms
(3 rows)
The main thing which bothers me right now about my refactoring is how
different the code paths for CTAS and EXPLAIN ANALYZE CTAS are, which
leads to weirdness like this. I wonder if we cannot make them share more
code e.g. by having ExplainOneUtility() call into some function in
createas.c.
Maybe I should give it a shot and then start a new thread for the
refactoring once I have looked more into this.
Andreas
On Sun, Feb 17, 2019 at 03:31:05PM +0100, Andreas Karlsson wrote:
Yeah, noticed the same thing myself while refactoring the CTAS code, but I
guess the output could be like the current output for "EXPLAIN ANALYZE
<CTAS> WITH NO DATA;", i.e. a top plan with "(never executed)" (see below
for an example).
Yes, that matches my ideas on the matter (when excluding some
partitions at execution time for pruning something similar is used),
except that I would have used "(never executed as relation exists)" or
similar so as it is possible to make the difference between a relation
not created and no data inserted if using a CTAS IF NOT EXISTS with NO
DATA.
The main thing which bothers me right now about my refactoring is how
different the code paths for CTAS and EXPLAIN ANALYZE CTAS are, which leads
to weirdness like this. I wonder if we cannot make them share more code e.g.
by having ExplainOneUtility() call into some function in createas.c.
I think that we are on the same page here. The code path creating the
CTAS relation includes the if_not_exists check which should be used by
EXPLAIN, EXECUTE and normal CTAS, and return an ObjectAddress maybe
invalid if the relation has not been created. I have not looked in
details, but it seems that we would need to pass down if_not_exists to
the IntoClause.
Maybe I should give it a shot and then start a new thread for the
refactoring once I have looked more into this.
Thanks! That's not something which would get back-patched, and the
new APIs can be designed more carefully.
--
Michael
Hi,
Will it be helpful if the comments section of ExecuteStmt in gram.y is
updated to include the IF NOT EXISTS clause.Something like this
CREATE TABLE <name> [IF NOT EXISTS] AS EXECUTE <plan_name> [(params, ...)]
Regards,
Ram.
On Sun, Mar 10, 2019 at 06:56:57PM +0530, Ramanarayana wrote:
Will it be helpful if the comments section of ExecuteStmt in gram.y is
updated to include the IF NOT EXISTS clause.Something like thisCREATE TABLE <name> [IF NOT EXISTS] AS EXECUTE <plan_name> [(params, ...)]
Not sure it helps much. The other clauses don't include that much
details, and the grammar supported can be guessed easily from the
code.
--
Michael