Обсуждение: 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

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

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 unt
> OK, 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: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.

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

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 this
>
> CREATE 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