[tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Mar 26 21:06:29 MSK 2019
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
>
More information about the Tarantool-patches
mailing list