From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp35.i.mail.ru (smtp35.i.mail.ru [94.100.177.95]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0B4A3469710 for ; Thu, 19 Nov 2020 02:06:02 +0300 (MSK) References: <20201009134529.13212-1-roman.habibov@tarantool.org> <20201009134529.13212-3-roman.habibov@tarantool.org> <20201105221728.GA8188@tarantool.org> <28F38887-3AED-4D94-9019-1BBDCF176AEF@tarantool.org> <20201117202318.GA19460@tarantool.org> From: roman Message-ID: Date: Sun, 15 Nov 2020 14:51:01 +0300 MIME-Version: 1.0 In-Reply-To: <20201117202318.GA19460@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.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 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 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 .     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 . + */ +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 . +     */ +    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 statement to be +     * created has . +     */ +    bool has_autoinc; +    /* Id of field with . */ +    uint32_t autoinc_fieldno;      bool initiateTTrans;    /* Initiate Tarantool transaction */      /** If set - do not emit byte code at all, just parse.  */      bool parse_only;