On Tue, 2006-02-14 at 14:32 -0500, Kris Jurka wrote:
> This patch adds most of the options available for regular CREATE TABLE
> syntax to the CREATE TABLE x AS SELECT ... and AS EXECUTE ...
> Specifically this allows specification of on commit behavior for temp
> tables and tablespaces for regular tables to these two statements.

The implementation is pretty ugly -- it clutters ExecuteStmt and Query
with fields that really do not belong there. Per previous discussion, I
think it would be better to refactor the CREATE TABLE AS implementation
to be essentially a CREATE TABLE followed by a INSERT ... SELECT.

(That's not necessarily a reason to reject the patch, but the patch does
increase the benefit of performing that refactoring.)

A few cosmetic comments:

typedef struct ExecuteStmt
{
NodeTag type;
char *name;
RangeVar *into;
ContainsOids intocontainsoids;
bool intohasoids;
OnCommitAction intooncommit;
char *intotablespacename;
List *params;
} ExecuteStmt;

I think we ought to use either camel-case or underscore characters to
separate words.

parser/analyze.c, circa 1822:

if (stmt->intoTableSpaceName)
qry->intoTableSpaceName = pstrdup(stmt->intoTableSpaceName);
else
qry->intoTableSpaceName = NULL;

You can omit the "else", as makeNode() zeroes all the fields of the new
node. (You could argue that leaving the assignment is more readable, but
I personally don't think so: this behavior of makeNode() is used in a
several places in the backend.)

-Neil



---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Neil Conway <neilc@samurai.com> writes:
> The implementation is pretty ugly -- it clutters ExecuteStmt and Query
> with fields that really do not belong there. Per previous discussion, I
> think it would be better to refactor the CREATE TABLE AS implementation
> to be essentially a CREATE TABLE followed by a INSERT ... SELECT.

I kinda wonder why bother at all. I don't see any good reason why
people shouldn't issue two statements.


>> if (stmt->intoTableSpaceName)
>> qry->intoTableSpaceName = pstrdup(stmt->intoTableSpaceName);
>> else
>> qry->intoTableSpaceName = NULL;

> You can omit the "else", as makeNode() zeroes all the fields of the new
> node.

For that matter, why not just

qry->intoTableSpaceName = stmt->intoTableSpaceName;

There's no need for the string-copy operation here, is there?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

On Tue, 14 Feb 2006, Tom Lane wrote:

> I kinda wonder why bother at all. I don't see any good reason why
> people shouldn't issue two statements.
>

Well if you don't know what the resulting columns are going to be that
could be difficult. There are a number of reasons why this patch is an
improvement.

1) People have requested this feature:
http://archives.postgresql.org/pgsql...1/msg00163.php
http://archives.postgresql.org/pgsql...3/msg00186.php

2) The SQL spec requires ON COMMIT for CREATE TEMP TABLE AS SELECT.

3) The unification of EXECUTE and SELECT options actually simplifies the
grammar by removing the WithOidsAs production hack.

Kris Jurka

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

On Tue, 2006-02-14 at 15:38 -0500, Tom Lane wrote:
> Neil Conway <neilc@samurai.com> writes:
> > The implementation is pretty ugly -- it clutters ExecuteStmt and Query
> > with fields that really do not belong there. Per previous discussion, I
> > think it would be better to refactor the CREATE TABLE AS implementation
> > to be essentially a CREATE TABLE followed by a INSERT ... SELECT.
>
> I kinda wonder why bother at all. I don't see any good reason why
> people shouldn't issue two statements.

The good reason is that CREATE plus INSERT SELECT is extremely bug prone
and very annoying to have to code SQL that way. CTAS is sent from on
high, IMHO, even without the performance optimisation.

Best Regards, Simon Riggs


---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match