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 1A3A629D87 for ; Tue, 26 Mar 2019 14:06:34 -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 cA5t4orkUnpJ for ; Tue, 26 Mar 2019 14:06:34 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 8A4C829D81 for ; Tue, 26 Mar 2019 14:06:32 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing 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> <611D6548-9882-4FB0-9D2A-828E2CAFF83D@tarantool.org> From: Vladislav Shpilevoy Message-ID: <573b3a69-3b68-c40d-e0b5-2d36a45964fb@tarantool.org> Date: Tue, 26 Mar 2019 21:06:29 +0300 MIME-Version: 1.0 In-Reply-To: <611D6548-9882-4FB0-9D2A-828E2CAFF83D@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "n.pettik" , tarantool-patches@freelists.org Thanks for the fixes! This commit LGTM. Lets proceed to the next patches, and start with a rebase, which is going to be hard. On 26/03/2019 21:01, n.pettik wrote: > >> See 4 comments below. >> >>> 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) ::= IF NOT EXISTS. {A = 1;} >>> create_table_args ::= LP columnlist RP(E). { >>> sqlEndTable(pParse,&E,0); >>> + create_table_def_destroy(&pParse->create_table_def); >>> } >>> create_table_args ::= AS select(S). { >>> sqlEndTable(pParse,0,S); >> >> 1. Why do not you destroy table def here? > > Because CREATE TABLE AS SELECT is completely broken. > I’ve 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’t 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 > > 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 = pParse->db; /* The database connection */ > > - if (pEnd == NULL && pSelect == NULL) { > - return; > - } > assert(!db->mallocFailed); > struct space *new_space = pParse->new_space; > if (new_space == 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) ::= CREATE(A). {disableLookaside(pParse);} > ifnotexists(A) ::= . {A = 0;} > ifnotexists(A) ::= IF NOT EXISTS. {A = 1;} > > -create_table_args ::= LP columnlist RP(E). { > - sqlEndTable(pParse,&E,0); > -} > -create_table_args ::= AS select(S). { > - sqlEndTable(pParse,0,S); > - sql_select_delete(pParse->db, S); > +create_table_args ::= LP columnlist RP. { > + sqlEndTable(pParse); > } > + > +/* > + * CREATE TABLE AS SELECT is broken. To be re-implemented > + * in gh-3223. > + * > + * create_table_args ::= AS select(S). { > + * sqlEndTable(pParse); > + * sql_select_delete(pParse->db, S); > + * } > + */ > columnlist ::= columnlist COMMA tconsdef. > columnlist ::= columnlist COMMA columnname carglist. > columnlist ::= 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); > > -void sqlEndTable(Parse *, Token *, Select *); > +void > +sqlEndTable(struct Parse *parse); > > /** > * Create cursor which will be positioned to the space/index. > >> >>> sql_select_delete(pParse->db, S); >>> } >>> -columnlist ::= columnlist COMMA tconsdef. >>> +columnlist ::= columnlist COMMA tcons. >>> columnlist ::= columnlist COMMA columnname carglist. >>> columnlist ::= columnname carglist. >>> -columnlist ::= tconsdef. >>> +columnlist ::= tcons. >>> columnname(A) ::= 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’t 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) ::= IF NOT EXISTS. {A = 1;} > > create_table_args ::= LP columnlist RP. { > sqlEndTable(pParse); > - create_table_def_destroy(&pParse->create_table_def); > } > > /* > 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 == 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 = parser->db; > sqlDbFree(db, parser->aLabel); > sql_expr_list_delete(db, parser->pConstExpr); > + create_table_def_destroy(&parser->create_table_def); > if (db != NULL) { > assert(db->lookaside.bDisable >= > parser->disableLookaside); > > >> >> 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 >