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

[n.pettik]

> 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