From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 D2F9C469719 for ; Sun, 4 Oct 2020 16:45:53 +0300 (MSK) References: <20200911215115.6622-1-roman.habibov@tarantool.org> <20200911215115.6622-3-roman.habibov@tarantool.org> <36bb1beb-a59a-57d9-a1ef-c0ef5af49d35@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sun, 4 Oct 2020 15:45:52 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! See 3 comments below. > 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 > @@ -1147,6 +1147,88 @@ 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 > +sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id) 1. After looking at another SQL patch I realized that this patch has the same issue - vdbe code generation methods should be started from 'vdbe_emit_' prefix. We can't invent a new prefix in every patch doing something with VDBE code. The code will eventually become unreadable. > +{ > + 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); > + } > @@ -1923,10 +1942,10 @@ sql_create_foreign_key(struct Parse *parse_context) > } > if (!is_alter) { > if (create_def->name.n == 0) { > + uint32_t i = ++parse_context->create_fkeys_def.count; > constraint_name = > sqlMPrintf(db, "fk_unnamed_%s_%d", > - space->def->name, > - ++table_def->fkey_count); > + space->def->name, i); 2. The format uses '%d', and you pass uint32_t, which is usually '%u' (not always). Why can't you use a mere 'int' for the counter? In the new struct definitions and here. > } else { > constraint_name = > sql_name_from_token(db, &create_def->name); > 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. */ 3. Lets try second time. What is 'check constraint object'? I can't find 'struct check_constraint_object' in the code. The same for foreign key constraints below. > + 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; > };