Thread: Fix command completion for CREATE TABEL ... AS
(Please note that the patches:
"[PATCHES] Allow arbitrary levels of analyze/rewriting" and
"[PATCHES] Fix issuing of multiple command completions per command"
must be applied before this on)
This patch fixes the response for a CREATE TABLE ... AS statement.
Before you would see:
test=# create table b as select * from a;
SELECT
because it is internally transformed into the equivalent of
test=# select * into c from a;
SELECT
With this patch one now sees:
test=# create table b as select * from a;
CREATE
as expected.
ChangeLog:
* src/backend/tcop/pquery.c (ProcessQuery): Allow the caller to
specify the command tag. Create a tag based on commandType if
no
tag specified by caller.
* src/include/tcop/pquery.h: Adjust declaration for the above
function.
* src/backend/commands/explain.c (ExplainOneQuery): Adjust call
to
the above function.
* src/backend/tcop/postgres.c (pg_exec_query_string): Call
ProcessQuery() with the command tag that refers to the original
command, before rewriting.
(CreateCommandTag): Handle T_CreateAsStmt nodes.
* src/include/nodes/nodes.h: Add T_CreateAsStmt for CREATE
TABLE...AS.
* src/include/nodes/parsenodes.h: Add CreateAsStmt for CREATE
TABLE...AS.
* src/backend/nodes/copyfuncs.c (copyObject): Handle
T_CreateAsStmt.
* src/backend/nodes/equalfuncs.c (equal): Ditto.
* src/backend/parser/gram.y: Parse CREATE TABLE...AS into a
CreateAsStmt.
* src/backend/parser/analyze.c (transformStmt): Rewrite a
CreateAsStmt into a SelectStmt.
--
Fernando Nasser
Red Hat Canada Ltd. E-Mail: fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario M4P 2C9Index: src/backend/commands/explain.c
===================================================================
RCS file: /cvs/cvsfiles/devo/pgsql/src/backend/commands/explain.c,v
retrieving revision 1.12.2.1
diff -c -p -r1.12.2.1 explain.c
*** explain.c 2002/02/19 16:09:19 1.12.2.1
--- explain.c 2002/02/20 01:25:53
*************** ExplainOneQuery(Query *query, bool verbo
*** 123,129 ****
plan->instrument = InstrAlloc();
gettimeofday(&starttime, NULL);
! ProcessQuery(query, plan, None);
CommandCounterIncrement();
gettimeofday(&endtime, NULL);
--- 123,129 ----
plan->instrument = InstrAlloc();
gettimeofday(&starttime, NULL);
! ProcessQuery(query, plan, None, NULL);
CommandCounterIncrement();
gettimeofday(&endtime, NULL);
Index: src/backend/nodes/copyfuncs.c
===================================================================
RCS file: /cvs/cvsfiles/devo/pgsql/src/backend/nodes/copyfuncs.c,v
retrieving revision 1.13
diff -c -p -r1.13 copyfuncs.c
*** copyfuncs.c 2002/02/04 20:59:22 1.13
--- copyfuncs.c 2002/02/20 01:25:53
*************** copyObject(void *from)
*** 2889,2894 ****
--- 2889,2898 ----
case T_CheckPointStmt:
retval = (void *) makeNode(CheckPointStmt);
break;
+ case T_CreateAsStmt:
+ retval = _copySelectStmt(from);
+ ((CreateAsStmt *)retval)->type = T_CreateAsStmt;
+ break;
case T_A_Expr:
retval = _copyAExpr(from);
Index: src/backend/nodes/equalfuncs.c
===================================================================
RCS file: /cvs/cvsfiles/devo/pgsql/src/backend/nodes/equalfuncs.c,v
retrieving revision 1.13
diff -c -p -r1.13 equalfuncs.c
*** equalfuncs.c 2002/02/04 20:59:22 1.13
--- equalfuncs.c 2002/02/20 01:25:53
*************** equal(void *a, void *b)
*** 2039,2044 ****
--- 2039,2047 ----
case T_CheckPointStmt:
retval = true;
break;
+ case T_CreateAsStmt:
+ retval = _equalSelectStmt(a, b);
+ break;
case T_A_Expr:
retval = _equalAExpr(a, b);
Index: src/backend/parser/analyze.c
===================================================================
RCS file: /cvs/cvsfiles/devo/pgsql/src/backend/parser/analyze.c,v
retrieving revision 1.14.2.1
diff -c -p -r1.14.2.1 analyze.c
*** analyze.c 2002/02/14 16:58:38 1.14.2.1
--- analyze.c 2002/02/20 01:25:54
*************** transformStmt(ParseState *pstate, Node *
*** 270,275 ****
--- 270,276 ----
break;
case T_SelectStmt:
+ case T_CreateAsStmt:
if (((SelectStmt *) parseTree)->op == SETOP_NONE)
result = transformSelectStmt(pstate,
(SelectStmt *) parseTree);
Index: src/backend/parser/gram.y
===================================================================
RCS file: /cvs/cvsfiles/devo/pgsql/src/backend/parser/gram.y,v
retrieving revision 1.14
diff -c -p -r1.14 gram.y
*** gram.y 2002/02/04 20:59:24 1.14
--- gram.y 2002/02/20 01:25:55
*************** CreateAsStmt: CREATE OptTemp TABLE rela
*** 1625,1630 ****
--- 1625,1631 ----
n->istemp = $2;
n->into = $4;
n->intoColNames = $5;
+ n->type = T_CreateAsStmt; /* Same structure, different tag */
$$ = $7;
}
;
Index: src/backend/tcop/postgres.c
===================================================================
RCS file: /cvs/cvsfiles/devo/pgsql/src/backend/tcop/postgres.c,v
retrieving revision 1.15.2.1
diff -c -p -r1.15.2.1 postgres.c
*** postgres.c 2002/02/19 16:09:19 1.15.2.1
--- postgres.c 2002/02/20 01:25:55
*************** pg_exec_query_string(char *query_string,
*** 824,830 ****
{
if (DebugLvl > 1)
elog(DEBUG, "ProcessQuery");
! ProcessQuery(querytree, plan, dest);
}
if (Show_executor_stats)
--- 824,830 ----
{
if (DebugLvl > 1)
elog(DEBUG, "ProcessQuery");
! ProcessQuery(querytree, plan, dest, commandTag);
}
if (Show_executor_stats)
*************** CreateCommandTag(Node *parsetree)
*** 2155,2160 ****
--- 2155,2161 ----
break;
case T_CreateStmt:
+ case T_CreateAsStmt:
tag = "CREATE";
break;
Index: src/backend/tcop/pquery.c
===================================================================
RCS file: /cvs/cvsfiles/devo/pgsql/src/backend/tcop/pquery.c,v
retrieving revision 1.12.2.1
diff -c -p -r1.12.2.1 pquery.c
*** pquery.c 2002/02/19 16:09:19 1.12.2.1
--- pquery.c 2002/02/20 01:25:55
*************** PreparePortal(char *portalName)
*** 167,176 ****
void
ProcessQuery(Query *parsetree,
Plan *plan,
! CommandDest dest)
{
int operation = parsetree->commandType;
- char *tag;
bool isRetrieveIntoPortal;
bool isRetrieveIntoRelation;
char *intoName = NULL;
--- 167,176 ----
void
ProcessQuery(Query *parsetree,
Plan *plan,
! CommandDest dest,
! char * tag)
{
int operation = parsetree->commandType;
bool isRetrieveIntoPortal;
bool isRetrieveIntoRelation;
char *intoName = NULL;
*************** ProcessQuery(Query *parsetree,
*** 180,186 ****
EState *state;
TupleDesc attinfo;
! tag = CreateOperationTag(operation);
/*
* initialize portal/into relation status
--- 180,188 ----
EState *state;
TupleDesc attinfo;
! if (tag == NULL)
! tag = CreateOperationTag(operation);
!
/*
* initialize portal/into relation status
Index: src/include/nodes/nodes.h
===================================================================
RCS file: /cvs/cvsfiles/devo/pgsql/src/include/nodes/nodes.h,v
retrieving revision 1.13
diff -c -p -r1.13 nodes.h
*** nodes.h 2002/02/04 20:59:39 1.13
--- nodes.h 2002/02/20 01:25:55
*************** typedef enum NodeTag
*** 194,199 ****
--- 194,200 ----
T_DropGroupStmt,
T_ReindexStmt,
T_CheckPointStmt,
+ T_CreateAsStmt,
T_A_Expr = 700,
T_Attr,
Index: src/include/nodes/parsenodes.h
===================================================================
RCS file: /cvs/cvsfiles/devo/pgsql/src/include/nodes/parsenodes.h,v
retrieving revision 1.13
diff -c -p -r1.13 parsenodes.h
*** parsenodes.h 2002/02/04 20:59:39 1.13
--- parsenodes.h 2002/02/20 01:25:55
*************** typedef struct SelectStmt
*** 910,915 ****
--- 910,925 ----
} SelectStmt;
/* ----------------------
+ * Create Table ... As Statement
+ *
+ * A CREATE TABLE ... AS statement is a SelectStmt with the
+ * 'into' field set and the proper tag.
+ * ----------------------
+ */
+
+ typedef struct SelectStmt CreateAsStmt;
+
+ /* ----------------------
* Set Operation node for post-analysis query trees
*
* After parse analysis, a SELECT with set operations is represented by a
Index: src/include/tcop/pquery.h
===================================================================
RCS file: /cvs/cvsfiles/devo/pgsql/src/include/tcop/pquery.h,v
retrieving revision 1.11
diff -c -p -r1.11 pquery.h
*** pquery.h 2002/02/04 20:59:40 1.11
--- pquery.h 2002/02/20 01:25:55
***************
*** 18,24 ****
#include "utils/portal.h"
! extern void ProcessQuery(Query *parsetree, Plan *plan, CommandDest dest);
extern EState *CreateExecutorState(void);
--- 18,25 ----
#include "utils/portal.h"
! extern void ProcessQuery(Query *parsetree, Plan *plan, CommandDest dest,
! char *tag);
extern EState *CreateExecutorState(void);
Fernando Nasser <fnasser@redhat.com> writes:
> This patch fixes the response for a CREATE TABLE ... AS statement.
Do we really need the overhead of adding a new parse node type just
to change the command tag returned for a CREATE ... AS?
If your definition of "correct command tag" is "first word of what the
user typed", we could get that more directly and reliably with a little
bit of lexer hacking; we should not base it on the parse tree at all.
But I'm not convinced that this is necessary or appropriate.
On the other hand, maybe you've got some other goal in mind that makes
it worthwhile to have distinct parse representations for SELECT ... INTO
and CREATE ... AS. But please explain what the motivation is for
changing this.
regards, tom lane
Tom Lane wrote: > > Fernando Nasser <fnasser@redhat.com> writes: > > This patch fixes the response for a CREATE TABLE ... AS statement. > > Do we really need the overhead of adding a new parse node type just > to change the command tag returned for a CREATE ... AS? > There is not much overhead -- CreateAsStmt it is just an alias for SelectStmt. > If your definition of "correct command tag" is "first word of what the > user typed", we could get that more directly and reliably with a little > bit of lexer hacking; we should not base it on the parse tree at all. But the only thing that comes from yparse in the parsetree is a tag (like T_CreateAsStmt). I can't get hold of that token. > But I'm not convinced that this is necessary or appropriate. > > On the other hand, maybe you've got some other goal in mind that makes > it worthwhile to have distinct parse representations for SELECT ... INTO > and CREATE ... AS. But please explain what the motivation is for > changing this. > Well, getting a SELECT response for a CREATE statement is a little bit disconcerting (except for people that know that the old syntax used to be SELECT ... INTO). Doesn't look very professional. As I implemented it with a minimal overhead it costs close to nothing to get it right. So why not do it? -- Fernando Nasser Red Hat Canada Ltd. E-Mail: fnasser@redhat.com 2323 Yonge Street, Suite #300 Toronto, Ontario M4P 2C9
Fernando Nasser <fnasser@redhat.com> writes:
> Tom Lane wrote:
>> Do we really need the overhead of adding a new parse node type just
>> to change the command tag returned for a CREATE ... AS?
> There is not much overhead -- CreateAsStmt it is just an alias for
> SelectStmt.
Yes, but for every node type there need to be copy, compare, output,
and input routines; and something like SelectStmt is certain to need
changes from time to time, which will also have to get done on
CreateAsStmt (or cause subtle bugs if someone forgets). It's that
long-term maintenance cost that I'm objecting to.
Moreover, this is far from the only case where we translate multiple
user-entered constructs into the same parse-level representation.
A relevant example is that COMMIT and END produce identical parse
trees, as do ABORT and ROLLBACK. Will you insist on changing all
such cases?
> Well, getting a SELECT response for a CREATE statement is a little bit
> disconcerting (except for people that know that the old syntax used to
> be SELECT ... INTO). Doesn't look very professional.
Again, what's your definition of "looking professional"? Do you want to
define it as repeating the first word of what the user typed, regardless
of what our internal representation is? If so, I can make that happen,
in a direct way that doesn't depend on assuming any global properties
of the parse representation. A quick little hack to make the lexer save
the first lexed token of each statement will do nicely.
However, an equally defensible definition of "looking professional"
is "emitting the most standard name for the construct" --- this is
what we currently do with COMMIT/END/ABORT/ROLLBACK. I think it could
be argued with reasonable vigor that the right answer for your concern
is that SELECT...INTO and CREATE...AS should both produce
command-completion tags of CREATE.
regards, tom lane
Tom Lane wrote: > > Fernando Nasser <fnasser@redhat.com> writes: > > Tom Lane wrote: > >> Do we really need the overhead of adding a new parse node type just > >> to change the command tag returned for a CREATE ... AS? > > > There is not much overhead -- CreateAsStmt it is just an alias for > > SelectStmt. > > Yes, but for every node type there need to be copy, compare, output, > and input routines; and something like SelectStmt is certain to need > changes from time to time, which will also have to get done on > CreateAsStmt (or cause subtle bugs if someone forgets). It's that > long-term maintenance cost that I'm objecting to. > No, it is an alias. The same copy/equal are used. Changes are automatically carried over. No maintenance required :-) > Moreover, this is far from the only case where we translate multiple > user-entered constructs into the same parse-level representation. > A relevant example is that COMMIT and END produce identical parse > trees, as do ABORT and ROLLBACK. Will you insist on changing all > such cases? > No, those should print the SQL standard name. The user must be aware that he/she is using a non-standard name (extension) that is being converted. And the words mean the same thing, whereas CREATE and SELECT are very different beasts. > > Well, getting a SELECT response for a CREATE statement is a little bit > > disconcerting (except for people that know that the old syntax used to > > be SELECT ... INTO). Doesn't look very professional. > > Again, what's your definition of "looking professional"? Do you want to > define it as repeating the first word of what the user typed, regardless > of what our internal representation is? If so, I can make that happen, > in a direct way that doesn't depend on assuming any global properties > of the parse representation. A quick little hack to make the lexer save > the first lexed token of each statement will do nicely. > The lexer does not know about statements. What exactly do you mean? And the parser does not return anything but the parsetree. There we could add one more parameter to yparse to return a command name. I don't know how portable this meddling with yparse parameters is though. > However, an equally defensible definition of "looking professional" > is "emitting the most standard name for the construct" --- this is > what we currently do with COMMIT/END/ABORT/ROLLBACK. I think it could > be argued with reasonable vigor that the right answer for your concern > is that SELECT...INTO and CREATE...AS should both produce > command-completion tags of CREATE. > I agree (see above) that extensions should return the standard completion. I have no problem with that. I will be pleased to make SELECT...INTO generate a CreateAsStmt and it will also print CREATE. -- Fernando Nasser Red Hat Canada Ltd. E-Mail: fnasser@redhat.com 2323 Yonge Street, Suite #300 Toronto, Ontario M4P 2C9
Fernando Nasser <fnasser@redhat.com> writes:
> I have no problem with that. I will be pleased to make SELECT...INTO
> generate a CreateAsStmt and it will also print CREATE.
Well, if you want to do that then there should be actual revisions to
the parsetree representation (viz, get rid of INTO), so as to make this
stuff cleaner instead of dirtier. INTO is really quite messy right now,
since we have to cope with pulling what ought to be a top-level
construct out of a nested SelectStmt. I would like to see CreateAs
be a separate statement construct having three fields: target table
name, optional column name list, and source SelectStmt (pointer to
a SELECT parsetree). Then we could remove the INTO field from
SelectStmt and make the parsetree and parse analysis work cleaner.
However, right now this transformation is (effectively) done in
analyze.c, and I'm not sure whether it can be done as cleanly in
gram.y. But do take a look at it ...
The alternative is to stick with the existing parsetree representation
and just tweak the command-tag-extraction code to look for an into
clause and print CREATE if so.
regards, tom lane
Tom Lane wrote: > > Fernando Nasser <fnasser@redhat.com> writes: > > I have no problem with that. I will be pleased to make SELECT...INTO > > generate a CreateAsStmt and it will also print CREATE. > > Well, if you want to do that then there should be actual revisions to > the parsetree representation (viz, get rid of INTO), so as to make this > stuff cleaner instead of dirtier. INTO is really quite messy right now, > since we have to cope with pulling what ought to be a top-level > construct out of a nested SelectStmt. I would like to see CreateAs > be a separate statement construct having three fields: target table > name, optional column name list, and source SelectStmt (pointer to > a SELECT parsetree). Then we could remove the INTO field from > SelectStmt and make the parsetree and parse analysis work cleaner. > > However, right now this transformation is (effectively) done in > analyze.c, and I'm not sure whether it can be done as cleanly in > gram.y. But do take a look at it ... > OK, I will take a look to see how difficult it would be to get rid of 'into' by having a full fledge CreateAsStmt. > The alternative is to stick with the existing parsetree representation > and just tweak the command-tag-extraction code to look for an into > clause and print CREATE if so. > Yes, that is a simple solution. If the above is to difficult we can do that. Thanks for the suggestion. I will rework the patch to one form or the other. -- Fernando Nasser Red Hat Canada Ltd. E-Mail: fnasser@redhat.com 2323 Yonge Street, Suite #300 Toronto, Ontario M4P 2C9