[tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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
>