From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 8C28329D80 for ; Tue, 26 Mar 2019 14:01:41 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id mmPmLTIxBGmq for ; Tue, 26 Mar 2019 14:01:41 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1341529D72 for ; Tue, 26 Mar 2019 14:01:40 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing From: "n.pettik" In-Reply-To: Date: Tue, 26 Mar 2019 21:01:38 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <611D6548-9882-4FB0-9D2A-828E2CAFF83D@tarantool.org> References: <0fcc585532a1f1200a7dfd4a8e911ecf9f2c94aa.1548265148.git.korablev@tarantool.org> <45655E85-4DBB-4AC4-8161-5B783C66C688@tarantool.org> <6e4d0aae-5dbe-f8d6-3bfe-7723ea2f3d9d@tarantool.org> <78138FFB-162A-4703-9A8F-CB88FD0570EE@tarantool.org> <4841a3f7-5b21-10ea-4f58-160a5639b7d9@tarantool.org> <89C4A9F1-CB69-4F7B-95FF-9FEF751F1BB6@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy > See 4 comments below. >=20 >> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y >> index 996f55d37..aa16cb874 100644 >> --- a/src/box/sql/parse.y >> +++ b/src/box/sql/parse.y >> @@ -178,15 +179,16 @@ ifnotexists(A) ::=3D IF NOT EXISTS. {A =3D 1;} >> create_table_args ::=3D LP columnlist RP(E). { >> sqlEndTable(pParse,&E,0); >> + create_table_def_destroy(&pParse->create_table_def); >> } >> create_table_args ::=3D AS select(S). { >> sqlEndTable(pParse,0,S); >=20 > 1. Why do not you destroy table def here? Because CREATE TABLE AS SELECT is completely broken. I=E2=80=99ve removed remains of this feature from parser and = sqlEndTable(). To be re-implemented in = https://github.com/tarantool/tarantool/issues/3223 On the other hand, even if it wasn=E2=80=99t broken, there would be no = need to call table_def_destroy(): AS SELECT implies that there is no FK constraints related to table being created (destroy cleans only FKs). sql: remove remains of CREATE TABLE AS SELECT stmt =20 This statement is completely broken and to be re-implemented in scope of #3223 issue. Current patch removes remains of this feature. diff --git a/src/box/sql/build.c b/src/box/sql/build.c index f82bcd7bc..e5af8c3bd 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1132,23 +1132,12 @@ resolve_link(struct Parse *parse_context, const = struct space_def *def, * * During this routine byte code for creation of new Tarantool * space and all necessary Tarantool indexes is emitted. - * - * If the pSelect argument is not NULL, it means that this routine - * was called to create a space generated from a - * "CREATE TABLE ... AS SELECT ..." statement. The column names of - * the new space will match the result set of the SELECT. */ void -sqlEndTable(Parse * pParse, /* Parse context */ - Token * pEnd, /* The ')' before options in the CREATE = TABLE */ - Select * pSelect /* Select from a "CREATE ... AS = SELECT" */ - ) +sqlEndTable(struct Parse *pParse) { sql *db =3D pParse->db; /* The database connection */ =20 - if (pEnd =3D=3D NULL && pSelect =3D=3D NULL) { - return; - } assert(!db->mallocFailed); struct space *new_space =3D pParse->new_space; if (new_space =3D=3D NULL) diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 996f55d37..c3a0e6245 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -176,13 +176,19 @@ createkw(A) ::=3D CREATE(A). = {disableLookaside(pParse);} ifnotexists(A) ::=3D . {A =3D 0;} ifnotexists(A) ::=3D IF NOT EXISTS. {A =3D 1;} =20 -create_table_args ::=3D LP columnlist RP(E). { - sqlEndTable(pParse,&E,0); -} -create_table_args ::=3D AS select(S). { - sqlEndTable(pParse,0,S); - sql_select_delete(pParse->db, S); +create_table_args ::=3D LP columnlist RP. { + sqlEndTable(pParse); } + +/* + * CREATE TABLE AS SELECT is broken. To be re-implemented + * in gh-3223. + * + * create_table_args ::=3D AS select(S). { + * sqlEndTable(pParse); + * sql_select_delete(pParse->db, S); + * } + */ columnlist ::=3D columnlist COMMA tconsdef. columnlist ::=3D columnlist COMMA columnname carglist. columnlist ::=3D columnname carglist. diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index cd66e5383..fb2062b37 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -3333,7 +3333,8 @@ void sqlAddCollateType(Parse *, Token *); struct coll * sql_column_collation(struct space_def *def, uint32_t column, uint32_t = *coll_id); =20 -void sqlEndTable(Parse *, Token *, Select *); +void +sqlEndTable(struct Parse *parse); =20 /** * Create cursor which will be positioned to the space/index. >=20 >> sql_select_delete(pParse->db, S); >> } >> -columnlist ::=3D columnlist COMMA tconsdef. >> +columnlist ::=3D columnlist COMMA tcons. >> columnlist ::=3D columnlist COMMA columnname carglist. >> columnlist ::=3D columnname carglist. >> -columnlist ::=3D tconsdef. >> +columnlist ::=3D tcons. >> columnname(A) ::=3D nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);} >> // An IDENTIFIER can be a generic identifier, or one of several > 2. As I understand, now table def leaks if parser stops after = sqlStartTable > but before sqlEndTable. Unfortunately, you are right. I thought that parsing process can=E2=80=99t= be stopped between these stages. Diff: diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index fa71b2a42..818e9f461 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -179,7 +179,6 @@ ifnotexists(A) ::=3D IF NOT EXISTS. {A =3D 1;} =20 create_table_args ::=3D LP columnlist RP. { sqlEndTable(pParse); - create_table_def_destroy(&pParse->create_table_def); } =20 /* diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h index 98234aa8c..a1af2bacd 100644 --- a/src/box/sql/parse_def.h +++ b/src/box/sql/parse_def.h @@ -454,6 +454,8 @@ create_view_def_init(struct create_view_def = *view_def, struct Token *name, static inline void create_table_def_destroy(struct create_table_def *table_def) { + if (table_def->new_space =3D=3D NULL) + return; struct fk_constraint_parse *fk; rlist_foreach_entry(fk, &table_def->new_fkey, link) sql_expr_list_delete(sql_get(), fk->selfref_cols); diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index e449c1fcb..034fd1492 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -308,6 +308,7 @@ sql_parser_destroy(Parse *parser) sql *db =3D parser->db; sqlDbFree(db, parser->aLabel); sql_expr_list_delete(db, parser->pConstExpr); + create_table_def_destroy(&parser->create_table_def); if (db !=3D NULL) { assert(db->lookaside.bDisable >=3D parser->disableLookaside); >=20 > 3. sql_create_index comment still refers to parse->new_space. diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 9a1fe5be0..8a05fa9f3 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -3369,8 +3369,8 @@ void sqlIdListDelete(sql *, IdList *); * indexed. Both will be NULL for a primary key or an index that * is created to satisfy a UNIQUE constraint. If tbl_name and * name are NULL, use parse->new_space as the table to be indexed. - * parse->new_space is a space that is currently being - * constructed by a CREATE TABLE statement. + * parse->create_tale_def->new_space is a space that is currently + * being constructed by a CREATE TABLE statement. * * @param parse All information about this parse. */ > 4. Please, write a comment above struct create_table_def = create_table_def in > struct Parse why it is not in the union. diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 9a1fe5be0..c4becd5f6 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2693,6 +2693,12 @@ struct Parse { struct drop_trigger_def drop_trigger_def; struct drop_view_def drop_view_def; }; + /** + * Table def is not part of union since information + * being held must survive till the end of parsing of + * whole CREATE TABLE statement (to pass it to + * sqlEndTable() function). + */ struct create_table_def create_table_def; /** * List of all records that were inserted in system spaces