Re: [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse

[roman <roman.habibov@tarantool.org>]

Hi! Thanks for the review.

On 17.11.2020 23:23, Nikita Pettik wrote:
> On 10 Nov 15:17, Roman Khabibov wrote:
>> Hi! Thanks for the review.
>>
>>> On Nov 6, 2020, at 01:17, Nikita Pettik <korablev@tarantool.org> wrote:
>>>
>>> On 09 Oct 16:45, Roman Khabibov wrote:
>>>> +++ b/src/box/sql/parse_def.h
>>>> @@ -205,26 +205,20 @@ struct create_entity_def {
>>>> -	uint32_t check_count;
>>>> -	/** Check constraint appeared in CREATE TABLE stmt. */
>>>> -	struct rlist new_check;
>>>> -	/** True, if table to be created has AUTOINCREMENT PK. */
>>>> -	bool has_autoinc;
>>>> -	/** Id of field with AUTOINCREMENT. */
>>>> -	uint32_t autoinc_fieldno;
>>> Did you consider adding create_column_def class? Imho it would
>>> fit better in parse hierarchy: it would derive from create_entity_def,
>>> has field_def, autoinc, ck, fk members.
>> Yes. We discussed this a lot with Vlad. This class is implemented in
>> the main, last patch of the patchset. Unfortunately, the check and
>> fk constraints lists had to be removed separately from this class,
>> because the check and fk descriptions can occur separately from the
>> column descriptions in CREATE TABLE, e.g. " CREATE TABLE t (a INT
>> PRIMARY KEY, CHECK (a > 0))”. To make the code reusable, it is easier
>> to emit opcode for all constants after CREATE TABLE parsing. For the
> What does 'constant' mean? IMHO it is not so 'reusable': anyway you
> have a lot of if (is_alter/is_alter_add_constr..) branches. But OK,
> since patch is ready, I believe this refactoring is not worth time
> required for that.
Sorry for misprint, I meant 'constraint'.
>> same reason, autoinc is separate.
>>
>>>> +};
>>>> +
>>>> +struct create_checks_def {
>>> Why not create_ck_constraint_def? This naming would be more consistent
>>> with existing ck_contraint_def etc. The same for create_fk_constraint_def.
>> Because these are lists of constant defs. We decided to emphasize this.
> Do not get what's that supposed to mean. This is naming which is taken in
> our source code.. If you have any strong arguments from your discussion,
> please copy-paste them in the answer.
Ok. Let's name it with _parse_ tag in order to avoid confusion. These 
structs
contain lists of of ck_constraint_parse_def/fk_constraint_parse_def objects.

+struct create_ck_constraint_parse_def {
+    /** List of ck_constraint_parse objects. */
+    struct rlist checks;
+    /** Count of ck_constraint_parse objects. */
+    uint32_t count;
+};
+
+struct create_fk_constraint_parse_def {
+    /** List of fk_constraint_parse objects. */
+    struct rlist fkeys;
+    /** Count of fk_constraint_parse objects. */
+    uint32_t count;
  };

commit f2b3145e1e5a3b2af2a76511c51e4db865099129
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Sat Aug 1 00:27:52 2020 +0300

     sql: refactor create_table_def and parse

     Move ck, fk constraint lists from struct create_table_def into new
     defs and autoincrement into struct Parse to make the code more
     reusable when implementing <ALTER TABLE ADD COLUMN>.

     Needed for #3075

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index d55c1cd..9d827d2 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -590,8 +590,9 @@ sql_create_check_contraint(struct Parse *parser)
          }
      } else {
          assert(! is_alter);
-        uint32_t ck_idx = ++parser->create_table_def.check_count;
-        name = tt_sprintf("ck_unnamed_%s_%d", space->def->name, ck_idx);
+        uint32_t ck_idx =
+ ++parser->create_ck_constraint_parse_def.count;
+        name = tt_sprintf("ck_unnamed_%s_%u", space->def->name, ck_idx);
      }
      size_t name_len = strlen(name);

@@ -652,8 +653,8 @@ sql_create_check_contraint(struct Parse *parser)
          sqlVdbeCountChanges(v);
          sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
      } else {
- rlist_add_entry(&parser->create_table_def.new_check, ck_parse,
-                link);
+ rlist_add_entry(&parser->create_ck_constraint_parse_def.checks,
+                ck_parse, link);
      }
  }

@@ -930,7 +931,7 @@ emitNewSysSequenceRecord(Parse *pParse, int 
reg_seq_id, const char *seq_name)
  static int
  emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int 
reg_seq_id)
  {
-    uint32_t fieldno = pParse->create_table_def.autoinc_fieldno;
+    uint32_t fieldno = pParse->autoinc_fieldno;

      Vdbe *v = sqlGetVdbe(pParse);
      int first_col = pParse->nMem + 1;
@@ -1147,6 +1148,92 @@ resolve_link(struct Parse *parse_context, const 
struct space_def *def,
      return -1;
  }

+/**
+ * Emit code to create sequences, indexes, check and foreign key
+ * constraints appeared in <CREATE TABLE>.
+ */
+static void
+vdbe_emit_create_constraints(struct Parse *parse, int reg_space_id)
+{
+    assert(reg_space_id != 0);
+    struct space *space = parse->create_table_def.new_space;
+    assert(space != NULL);
+    uint32_t i = 0;
+    for (; i < space->index_count; ++i) {
+        struct index *idx = space->index[i];
+        vdbe_emit_create_index(parse, space->def, idx->def,
+                       reg_space_id, idx->def->iid);
+    }
+
+    /*
+     * Check to see if we need to create an _sequence table
+     * for keeping track of autoincrement keys.
+     */
+    if (parse->has_autoinc) {
+        /* Do an insertion into _sequence. */
+        int reg_seq_id = ++parse->nMem;
+        struct Vdbe *v = sqlGetVdbe(parse);
+        assert(v != NULL);
+        sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id);
+        int reg_seq_rec = emitNewSysSequenceRecord(parse, reg_seq_id,
+                               space->def->name);
+        sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_rec);
+        /* Do an insertion into _space_sequence. */
+        int reg_space_seq_record =
+            emitNewSysSpaceSequenceRecord(parse, reg_space_id,
+                              reg_seq_id);
+        sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
+                  reg_space_seq_record);
+    }
+
+    /* Code creation of FK constraints, if any. */
+    struct fk_constraint_parse *fk_parse;
+    rlist_foreach_entry(fk_parse,
+ &parse->create_fk_constraint_parse_def.fkeys,
+                link) {
+        struct fk_constraint_def *fk_def = fk_parse->fk_def;
+        if (fk_parse->selfref_cols != NULL) {
+            struct ExprList *cols = fk_parse->selfref_cols;
+            for (uint32_t i = 0; i < fk_def->field_count; ++i) {
+                if (resolve_link(parse, space->def,
+                         cols->a[i].zName,
+  &fk_def->links[i].parent_field,
+                         fk_def->name) != 0)
+                    return;
+            }
+            fk_def->parent_id = reg_space_id;
+        } else if (fk_parse->is_self_referenced) {
+            struct key_def *pk_key_def =
+ sql_space_primary_key(space)->def->key_def;
+            if (pk_key_def->part_count != fk_def->field_count) {
+                diag_set(ClientError, ER_CREATE_FK_CONSTRAINT,
+                     fk_def->name, "number of columns in "\
+                     "foreign key does not match the "\
+                     "number of columns in the primary "\
+                     "index of referenced table");
+                parse->is_aborted = true;
+                return;
+            }
+            for (uint32_t i = 0; i < fk_def->field_count; ++i) {
+                fk_def->links[i].parent_field =
+                    pk_key_def->parts[i].fieldno;
+            }
+            fk_def->parent_id = reg_space_id;
+        }
+        fk_def->child_id = reg_space_id;
+        vdbe_emit_fk_constraint_create(parse, fk_def, space->def->name);
+    }
+
+    /* Code creation of CK constraints, if any. */
+    struct ck_constraint_parse *ck_parse;
+    rlist_foreach_entry(ck_parse,
+ &parse->create_ck_constraint_parse_def.checks,
+                link) {
+        vdbe_emit_ck_constraint_create(parse, ck_parse->ck_def,
+                           reg_space_id, space->def->name);
+    }
+}
+
  /*
   * This routine is called to report the final ")" that terminates
   * a CREATE TABLE statement.
@@ -1213,73 +1300,7 @@ sqlEndTable(struct Parse *pParse)

      int reg_space_id = getNewSpaceId(pParse);
      vdbe_emit_space_create(pParse, reg_space_id, name_reg, new_space);
-    for (uint32_t i = 0; i < new_space->index_count; ++i) {
-        struct index *idx = new_space->index[i];
-        vdbe_emit_create_index(pParse, new_space->def, idx->def,
-                       reg_space_id, idx->def->iid);
-    }
-
-    /*
-     * Check to see if we need to create an _sequence table
-     * for keeping track of autoincrement keys.
-     */
-    if (pParse->create_table_def.has_autoinc) {
-        assert(reg_space_id != 0);
-        /* Do an insertion into _sequence. */
-        int reg_seq_id = ++pParse->nMem;
-        sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id);
-        int reg_seq_record =
-            emitNewSysSequenceRecord(pParse, reg_seq_id,
-                         new_space->def->name);
-        sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record);
-        /* Do an insertion into _space_sequence. */
-        int reg_space_seq_record =
-            emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
-                              reg_seq_id);
-        sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
-                  reg_space_seq_record);
-    }
-    /* Code creation of FK constraints, if any. */
-    struct fk_constraint_parse *fk_parse;
-    rlist_foreach_entry(fk_parse, &pParse->create_table_def.new_fkey,
-                link) {
-        struct fk_constraint_def *fk_def = fk_parse->fk_def;
-        if (fk_parse->selfref_cols != NULL) {
-            struct ExprList *cols = fk_parse->selfref_cols;
-            for (uint32_t i = 0; i < fk_def->field_count; ++i) {
-                if (resolve_link(pParse, new_space->def,
-                         cols->a[i].zName,
-  &fk_def->links[i].parent_field,
-                         fk_def->name) != 0)
-                    return;
-            }
-            fk_def->parent_id = reg_space_id;
-        } else if (fk_parse->is_self_referenced) {
-            struct index *pk = sql_space_primary_key(new_space);
-            if (pk->def->key_def->part_count != fk_def->field_count) {
-                diag_set(ClientError, ER_CREATE_FK_CONSTRAINT,
-                     fk_def->name, "number of columns in "\
-                     "foreign key does not match the "\
-                     "number of columns in the primary "\
-                     "index of referenced table");
-                pParse->is_aborted = true;
-                return;
-            }
-            for (uint32_t i = 0; i < fk_def->field_count; ++i) {
-                fk_def->links[i].parent_field =
- pk->def->key_def->parts[i].fieldno;
-            }
-            fk_def->parent_id = reg_space_id;
-        }
-        fk_def->child_id = reg_space_id;
-        vdbe_emit_fk_constraint_create(pParse, fk_def, space_name_copy);
-    }
-    struct ck_constraint_parse *ck_parse;
-    rlist_foreach_entry(ck_parse, &pParse->create_table_def.new_check,
-                link) {
-        vdbe_emit_ck_constraint_create(pParse, ck_parse->ck_def,
-                           reg_space_id, space_name_copy);
-    }
+    vdbe_emit_create_constraints(pParse, reg_space_id);
  }

  void
@@ -1893,7 +1914,9 @@ sql_create_foreign_key(struct Parse *parse_context)
              goto tnt_error;
          }
          memset(fk_parse, 0, sizeof(*fk_parse));
-        rlist_add_entry(&table_def->new_fkey, fk_parse, link);
+        struct rlist *fkeys =
+ &parse_context->create_fk_constraint_parse_def.fkeys;
+        rlist_add_entry(fkeys, fk_parse, link);
      }
      struct Token *parent = create_fk_def->parent_name;
      assert(parent != NULL);
@@ -1910,8 +1933,10 @@ sql_create_foreign_key(struct Parse *parse_context)
      struct space *parent_space = space_by_name(parent_name);
      if (parent_space == NULL) {
          if (is_self_referenced) {
+            struct create_fk_constraint_parse_def *parse_def =
+ &parse_context->create_fk_constraint_parse_def;
              struct fk_constraint_parse *fk =
- rlist_first_entry(&table_def->new_fkey,
+ rlist_first_entry(&parse_def->fkeys,
                            struct fk_constraint_parse,
                            link);
              fk->selfref_cols = parent_cols;
@@ -1923,10 +1948,11 @@ sql_create_foreign_key(struct Parse *parse_context)
      }
      if (!is_alter) {
          if (create_def->name.n == 0) {
-            constraint_name =
-                sqlMPrintf(db, "fk_unnamed_%s_%d",
-                       space->def->name,
-                       ++table_def->fkey_count);
+            struct create_fk_constraint_parse_def *parse_def =
+ &parse_context->create_fk_constraint_parse_def;
+            uint32_t idx = ++parse_def->count;
+            constraint_name = sqlMPrintf(db, "fk_unnamed_%s_%u",
+                             space->def->name, idx);
          } else {
              constraint_name =
                  sql_name_from_token(db, &create_def->name);
@@ -2041,9 +2067,11 @@ sql_create_foreign_key(struct Parse *parse_context)
       * maintain list of all FK constraints inside parser.
       */
      if (!is_alter) {
+        struct rlist *fkeys =
+ &parse_context->create_fk_constraint_parse_def.fkeys;
          struct fk_constraint_parse *fk_parse =
-            rlist_first_entry(&table_def->new_fkey,
-                      struct fk_constraint_parse, link);
+            rlist_first_entry(fkeys, struct fk_constraint_parse,
+                      link);
          fk_parse->fk_def = fk_def;
      } else {
          vdbe_emit_fk_constraint_create(parse_context, fk_def,
@@ -2065,12 +2093,12 @@ tnt_error:
  void
  fk_constraint_change_defer_mode(struct Parse *parse_context, bool 
is_deferred)
  {
-    if (parse_context->db->init.busy ||
- rlist_empty(&parse_context->create_table_def.new_fkey))
+    struct rlist *fkeys =
+ &parse_context->create_fk_constraint_parse_def.fkeys;
+    if (parse_context->db->init.busy || rlist_empty(fkeys))
          return;
- rlist_first_entry(&parse_context->create_table_def.new_fkey,
-              struct fk_constraint_parse, link)->fk_def->is_deferred =
-        is_deferred;
+    rlist_first_entry(fkeys, struct fk_constraint_parse,
+              link)->fk_def->is_deferred = is_deferred;
  }

  /**
@@ -3306,15 +3334,15 @@ vdbe_emit_halt_with_presence_test(struct Parse 
*parser, int space_id,
  int
  sql_add_autoincrement(struct Parse *parse_context, uint32_t fieldno)
  {
-    if (parse_context->create_table_def.has_autoinc) {
+    if (parse_context->has_autoinc) {
          diag_set(ClientError, ER_SQL_SYNTAX_WITH_POS,
               parse_context->line_count, parse_context->line_pos,
               "table must feature at most one AUTOINCREMENT field");
          parse_context->is_aborted = true;
          return -1;
      }
-    parse_context->create_table_def.has_autoinc = true;
-    parse_context->create_table_def.autoinc_fieldno = fieldno;
+    parse_context->has_autoinc = true;
+    parse_context->autoinc_fieldno = fieldno;
      return 0;
  }

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 9958755..9e870c8 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -181,6 +181,8 @@ cmd ::= ROLLBACK TO savepoint_opt nm(X). {
  cmd ::= create_table create_table_args with_opts create_table_end.
  create_table ::= createkw TABLE ifnotexists(E) nm(Y). {
    create_table_def_init(&pParse->create_table_def, &Y, E);
+ 
create_ck_constraint_parse_def_init(&pParse->create_ck_constraint_parse_def);
+ 
create_fk_constraint_parse_def_init(&pParse->create_fk_constraint_parse_def);
    pParse->create_table_def.new_space = sqlStartTable(pParse, &Y);
    pParse->initiateTTrans = true;
  }
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index cb0ecd2..7ad7496 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -205,26 +205,20 @@ struct create_entity_def {
  struct create_table_def {
      struct create_entity_def base;
      struct space *new_space;
-    /**
-     * Number of FK constraints declared within
-     * CREATE TABLE statement.
-     */
-    uint32_t fkey_count;
-    /**
-     * Foreign key constraint appeared in CREATE TABLE stmt.
-     */
-    struct rlist new_fkey;
-    /**
-     * Number of CK constraints declared within
-     * CREATE TABLE statement.
-     */
-    uint32_t check_count;
-    /** Check constraint appeared in CREATE TABLE stmt. */
-    struct rlist new_check;
-    /** True, if table to be created has AUTOINCREMENT PK. */
-    bool has_autoinc;
-    /** Id of field with AUTOINCREMENT. */
-    uint32_t autoinc_fieldno;
+};
+
+struct create_ck_constraint_parse_def {
+    /** List of ck_constraint_parse_def objects. */
+    struct rlist checks;
+    /** Count of ck_constraint_parse_def objects. */
+    uint32_t count;
+};
+
+struct create_fk_constraint_parse_def {
+    /** List of fk_constraint_parse_def objects. */
+    struct rlist fkeys;
+    /** Count of fk_constraint_parse_def objects. */
+    uint32_t count;
  };

  struct create_view_def {
@@ -482,9 +476,20 @@ create_table_def_init(struct create_table_def 
*table_def, struct Token *name,
  {
      create_entity_def_init(&table_def->base, ENTITY_TYPE_TABLE, NULL, 
name,
                     if_not_exists);
-    rlist_create(&table_def->new_fkey);
-    rlist_create(&table_def->new_check);
-    table_def->autoinc_fieldno = 0;
+}
+
+static inline void
+create_ck_constraint_parse_def_init(struct 
create_ck_constraint_parse_def *def)
+{
+    rlist_create(&def->checks);
+    def->count = 0;
+}
+
+static inline void
+create_fk_constraint_parse_def_init(struct 
create_fk_constraint_parse_def *def)
+{
+    rlist_create(&def->fkeys);
+    def->count = 0;
  }

  static inline void
@@ -500,12 +505,12 @@ 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)
+create_fk_constraint_parse_def_destroy(struct 
create_fk_constraint_parse_def *d)
  {
-    if (table_def->new_space == NULL)
+    if (d->count == 0)
          return;
      struct fk_constraint_parse *fk;
-    rlist_foreach_entry(fk, &table_def->new_fkey, link)
+    rlist_foreach_entry(fk, &d->fkeys, 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 a5a2588..d859a02 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -200,6 +200,7 @@ sql_parser_create(struct Parse *parser, struct sql 
*db, uint32_t sql_flags)
      parser->sql_flags = sql_flags;
      parser->line_count = 1;
      parser->line_pos = 1;
+    parser->has_autoinc = false;
      region_create(&parser->region, &cord()->slabc);
  }

@@ -211,7 +212,9 @@ 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);
+    struct create_fk_constraint_parse_def *create_fk_constraint_parse_def =
+        &parser->create_fk_constraint_parse_def;
+ create_fk_constraint_parse_def_destroy(create_fk_constraint_parse_def);
      if (db != NULL) {
          assert(db->lookaside.bDisable >=
                 parser->disableLookaside);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index dedad09..6c79a86 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2257,6 +2257,18 @@ struct Parse {
       * sqlEndTable() function).
       */
      struct create_table_def create_table_def;
+    /*
+     * FK and CK constraints appeared in a <CREATE TABLE>.
+     */
+    struct create_fk_constraint_parse_def create_fk_constraint_parse_def;
+    struct create_ck_constraint_parse_def create_ck_constraint_parse_def;
+    /*
+     * True, if column within a <CREATE TABLE> statement to be
+     * created has <AUTOINCREMENT>.
+     */
+    bool has_autoinc;
+    /* Id of field with <AUTOINCREMENT>. */
+    uint32_t autoinc_fieldno;
      bool initiateTTrans;    /* Initiate Tarantool transaction */
      /** If set - do not emit byte code at all, just parse.  */
      bool parse_only;