From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 98C8144643A for ; Sun, 4 Oct 2020 00:37:35 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\)) From: Roman Khabibov In-Reply-To: <36bb1beb-a59a-57d9-a1ef-c0ef5af49d35@tarantool.org> Date: Sun, 4 Oct 2020 00:37:34 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20200911215115.6622-1-roman.habibov@tarantool.org> <20200911215115.6622-3-roman.habibov@tarantool.org> <36bb1beb-a59a-57d9-a1ef-c0ef5af49d35@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 2/6] sql: refactor create_table_def and parse List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. Sorry, didn=E2=80=99t notice this letter because of several answers below. > On Sep 17, 2020, at 17:43, Vladislav Shpilevoy = wrote: >=20 > Hi! Thanks for the patch! >=20 > On 11.09.2020 23:51, Roman Khabibov wrote: >> Move ck, fk constraint lists and autoincrement info from >> struct create_table_def to struct Parse >=20 > 1. You moved them into new defs, not into struct Parse. You need > to revisit some commit messages in this branch after review fixes > are done. Fixed. >> to make the code more >> reusable when implementing . >>=20 >> Needed for #3075 >> --- >> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h >> index cb0ecd2fc..c44e3dbbe 100644 >> --- a/src/box/sql/parse_def.h >> +++ b/src/box/sql/parse_def.h >> @@ -205,26 +205,16 @@ 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_checks_def { >> + struct rlist checks; >> + uint32_t count; >> +}; >> + >> +struct create_fkeys_def { >> + struct rlist fkeys; >> + uint32_t count; >=20 > *. All struct memebers should have a comment. At least it is worth > documenting what are the members of checks and fkeys lists. Done. >> }; >>=20 >> struct create_view_def { >> @@ -500,12 +501,12 @@ create_view_def_init(struct create_view_def = *view_def, struct Token *name, >> } >>=20 >> static inline void >> -create_table_def_destroy(struct create_table_def *table_def) >> +fkeys_def_destroy(struct create_fkeys_def *fkeys_def) >=20 > 2. Methods of a struct should have a prefix with the struct name. > It should be create_fkeys_def_destroy(). @@ -500,12 +505,12 @@ create_view_def_init(struct create_view_def = *view_def, struct Token *name, } =20 static inline void -create_table_def_destroy(struct create_table_def *table_def) +create_fkeys_def_destroy(struct create_fkeys_def *fkeys_def) { - if (table_def->new_space =3D=3D NULL) + if (fkeys_def->count =3D=3D 0) return; struct fk_constraint_parse *fk; - rlist_foreach_entry(fk, &table_def->new_fkey, link) + rlist_foreach_entry(fk, &fkeys_def->fkeys, link) sql_expr_list_delete(sql_get(), fk->selfref_cols); } >> { >> - if (table_def->new_space =3D=3D NULL) >> + if (fkeys_def->count =3D=3D 0) >> return; >> struct fk_constraint_parse *fk; >> - rlist_foreach_entry(fk, &table_def->new_fkey, link) >> + rlist_foreach_entry(fk, &fkeys_def->fkeys, link) >> sql_expr_list_delete(sql_get(), fk->selfref_cols); >> } >>=20 >> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h >> index 5913d7614..7057c20b6 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_fkeys_def fkeys_def; >> + struct create_checks_def checks_def; >=20 > 3. fkeys_def -> create_fkeys_def. > checks_def -> create_checks_def. Done. >> + /* >> + * 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; >>=20 commit 2334d5439719f251daa6d3332f76a0f780d4739a Author: Roman Khabibov Date: Sat Aug 1 00:27:52 2020 +0300 sql: refactor create_table_def and parse =20 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 . =20 Needed for #3075 diff --git a/src/box/sql/build.c b/src/box/sql/build.c index d55c1cd71..b9f7e4638 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -590,7 +590,7 @@ sql_create_check_contraint(struct Parse *parser) } } else { assert(! is_alter); - uint32_t ck_idx =3D = ++parser->create_table_def.check_count; + uint32_t ck_idx =3D ++parser->create_checks_def.count; name =3D tt_sprintf("ck_unnamed_%s_%d", = space->def->name, ck_idx); } size_t name_len =3D strlen(name); @@ -652,7 +652,7 @@ 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, + rlist_add_entry(&parser->create_checks_def.checks, = ck_parse, link); } } @@ -930,7 +930,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 =3D pParse->create_table_def.autoinc_fieldno; + uint32_t fieldno =3D pParse->autoinc_fieldno; =20 Vdbe *v =3D sqlGetVdbe(pParse); int first_col =3D pParse->nMem + 1; @@ -1147,6 +1147,88 @@ resolve_link(struct Parse *parse_context, const = struct space_def *def, return -1; } =20 +/** + * Emit code to create sequences, indexes, check and foreign key + * constraints appeared in . + */ +static void +sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id) +{ + assert(reg_space_id !=3D 0); + struct space *space =3D parse->create_table_def.new_space; + assert(space !=3D NULL); + uint32_t i =3D 0; + for (; i < space->index_count; ++i) { + struct index *idx =3D 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 =3D ++parse->nMem; + struct Vdbe *v =3D sqlGetVdbe(parse); + assert(v !=3D NULL); + sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id); + int reg_seq_rec =3D 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 =3D + 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_fkeys_def.fkeys, = link) { + struct fk_constraint_def *fk_def =3D fk_parse->fk_def; + if (fk_parse->selfref_cols !=3D NULL) { + struct ExprList *cols =3D = fk_parse->selfref_cols; + for (uint32_t i =3D 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) !=3D 0) + return; + } + fk_def->parent_id =3D reg_space_id; + } else if (fk_parse->is_self_referenced) { + struct key_def *pk_key_def =3D + = sql_space_primary_key(space)->def->key_def; + if (pk_key_def->part_count !=3D = 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 =3D true; + return; + } + for (uint32_t i =3D 0; i < fk_def->field_count; = ++i) { + fk_def->links[i].parent_field =3D + pk_key_def->parts[i].fieldno; + } + fk_def->parent_id =3D reg_space_id; + } + fk_def->child_id =3D 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_checks_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 +1295,7 @@ sqlEndTable(struct Parse *pParse) =20 int reg_space_id =3D getNewSpaceId(pParse); vdbe_emit_space_create(pParse, reg_space_id, name_reg, = new_space); - for (uint32_t i =3D 0; i < new_space->index_count; ++i) { - struct index *idx =3D 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 !=3D 0); - /* Do an insertion into _sequence. */ - int reg_seq_id =3D ++pParse->nMem; - sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id); - int reg_seq_record =3D - 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 =3D - 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 =3D fk_parse->fk_def; - if (fk_parse->selfref_cols !=3D NULL) { - struct ExprList *cols =3D = fk_parse->selfref_cols; - for (uint32_t i =3D 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) !=3D 0) - return; - } - fk_def->parent_id =3D reg_space_id; - } else if (fk_parse->is_self_referenced) { - struct index *pk =3D = sql_space_primary_key(new_space); - if (pk->def->key_def->part_count !=3D = 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 =3D true; - return; - } - for (uint32_t i =3D 0; i < fk_def->field_count; = ++i) { - fk_def->links[i].parent_field =3D - = pk->def->key_def->parts[i].fieldno; - } - fk_def->parent_id =3D reg_space_id; - } - fk_def->child_id =3D 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); - } + sql_vdbe_create_constraints(pParse, reg_space_id); } =20 void @@ -1893,7 +1909,8 @@ 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); + rlist_add_entry(&parse_context->create_fkeys_def.fkeys, + fk_parse, link); } struct Token *parent =3D create_fk_def->parent_name; assert(parent !=3D NULL); @@ -1910,8 +1927,10 @@ sql_create_foreign_key(struct Parse = *parse_context) struct space *parent_space =3D space_by_name(parent_name); if (parent_space =3D=3D NULL) { if (is_self_referenced) { + struct rlist *fkeys =3D + &parse_context->create_fkeys_def.fkeys; struct fk_constraint_parse *fk =3D - rlist_first_entry(&table_def->new_fkey, + rlist_first_entry(fkeys, struct = fk_constraint_parse, link); fk->selfref_cols =3D parent_cols; @@ -1923,10 +1942,10 @@ sql_create_foreign_key(struct Parse = *parse_context) } if (!is_alter) { if (create_def->name.n =3D=3D 0) { + uint32_t i =3D = ++parse_context->create_fkeys_def.count; constraint_name =3D sqlMPrintf(db, "fk_unnamed_%s_%d", - space->def->name, - ++table_def->fkey_count); + space->def->name, i); } else { constraint_name =3D sql_name_from_token(db, = &create_def->name); @@ -2041,9 +2060,10 @@ sql_create_foreign_key(struct Parse = *parse_context) * maintain list of all FK constraints inside parser. */ if (!is_alter) { + struct rlist *fkeys =3D = &parse_context->create_fkeys_def.fkeys; struct fk_constraint_parse *fk_parse =3D - 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 =3D fk_def; } else { vdbe_emit_fk_constraint_create(parse_context, fk_def, @@ -2065,12 +2085,11 @@ 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 =3D &parse_context->create_fkeys_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 =3D - is_deferred; + rlist_first_entry(fkeys, struct fk_constraint_parse, + link)->fk_def->is_deferred =3D is_deferred; } =20 /** @@ -3306,15 +3325,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 =3D true; return -1; } - parse_context->create_table_def.has_autoinc =3D true; - parse_context->create_table_def.autoinc_fieldno =3D fieldno; + parse_context->has_autoinc =3D true; + parse_context->autoinc_fieldno =3D fieldno; return 0; } =20 diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 995875566..da4e2a3ae 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -181,6 +181,8 @@ cmd ::=3D ROLLBACK TO savepoint_opt nm(X). { cmd ::=3D create_table create_table_args with_opts create_table_end. create_table ::=3D createkw TABLE ifnotexists(E) nm(Y). { create_table_def_init(&pParse->create_table_def, &Y, E); + create_checks_def_init(&pParse->create_checks_def); + create_fkeys_def_init(&pParse->create_fkeys_def); pParse->create_table_def.new_space =3D sqlStartTable(pParse, &Y); pParse->initiateTTrans =3D true; } diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h index cb0ecd2fc..504d190b2 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_checks_def { + /** List of check constraint objects. */ + struct rlist checks; + /** Count of check constraint objects. */ + uint32_t count; +}; + +struct create_fkeys_def { + /** List of foreign key constraint objects. */ + struct rlist fkeys; + /** Count of foreign key constraint objects. */ + uint32_t count; }; =20 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 =3D 0; +} + +static inline void +create_checks_def_init(struct create_checks_def *checks_def) +{ + rlist_create(&checks_def->checks); + checks_def->count =3D 0; +} + +static inline void +create_fkeys_def_init(struct create_fkeys_def *fkeys_def) +{ + rlist_create(&fkeys_def->fkeys); + fkeys_def->count =3D 0; } =20 static inline void @@ -500,12 +505,12 @@ create_view_def_init(struct create_view_def = *view_def, struct Token *name, } =20 static inline void -create_table_def_destroy(struct create_table_def *table_def) +create_fkeys_def_destroy(struct create_fkeys_def *fkeys_def) { - if (table_def->new_space =3D=3D NULL) + if (fkeys_def->count =3D=3D 0) return; struct fk_constraint_parse *fk; - rlist_foreach_entry(fk, &table_def->new_fkey, link) + rlist_foreach_entry(fk, &fkeys_def->fkeys, link) sql_expr_list_delete(sql_get(), fk->selfref_cols); } =20 diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index a5a258805..1a0cfb9c5 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 =3D sql_flags; parser->line_count =3D 1; parser->line_pos =3D 1; + parser->has_autoinc =3D false; region_create(&parser->region, &cord()->slabc); } =20 @@ -211,7 +212,7 @@ sql_parser_destroy(Parse *parser) sql *db =3D parser->db; sqlDbFree(db, parser->aLabel); sql_expr_list_delete(db, parser->pConstExpr); - create_table_def_destroy(&parser->create_table_def); + create_fkeys_def_destroy(&parser->create_fkeys_def); if (db !=3D NULL) { assert(db->lookaside.bDisable >=3D parser->disableLookaside); diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 5913d7614..3d54835d2 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_fkeys_def create_fkeys_def; + struct create_checks_def create_checks_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;