From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 82D0824466 for ; Mon, 14 Jan 2019 09:05:00 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id BUNQkL8RPIPm for ; Mon, 14 Jan 2019 09:05:00 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id BFF6C23FE7 for ; Mon, 14 Jan 2019 09:04:59 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 1/6] sql: move constraint name to struct contraint_parse References: <499ebc6e21ade22cde794f8470bf5900131d42f5.1547035183.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <834ae33b-dddd-3f51-f6f1-ef5bcc24e240@tarantool.org> Date: Mon, 14 Jan 2019 17:04:48 +0300 MIME-Version: 1.0 In-Reply-To: <499ebc6e21ade22cde794f8470bf5900131d42f5.1547035183.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Nikita Pettik Hi! Thanks for the patch! See 3 comments below. On 09/01/2019 15:13, Nikita Pettik wrote: > Before this patch name of constraint being parsed was held as a separate > struct Token within parser context. Meanwhile, we also need to store > name of table which constraint is related to (in order to implement > ALTER TABLE ADD CONSTRAINT with several options: foreign key, unique, > primary key, etc). Hence, lets move constraint name to a separate > structure and save it alongside with the name of table. > > Needed for #3097 > --- > src/box/sql/alter.c | 4 ++-- > src/box/sql/build.c | 36 +++++++++++++++++++----------------- > src/box/sql/parse.y | 22 +++++++++++++--------- > src/box/sql/prepare.c | 11 +++++++++++ > src/box/sql/sqliteInt.h | 32 ++++++++++++++++++-------------- > 5 files changed, 63 insertions(+), 42 deletions(-) > > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 4110a5991..51a5d01b5 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2691,6 +2691,17 @@ struct fkey_parse { > struct rlist link; > }; > > +/** > + * Used to hold intermediate meta-information during > + * constraint creation or alteration. > + */ > +struct constraint_parse { > + /** Name of table which constraint belongs to. */ > + struct SrcList *table_name; 1. I guess, it is not a 'table_name', but a 'table' since it is struct SrcList, not const char * nor Token. > + /** Name of the constraint currently being parsed. */ > + struct Token name; > +}; 2.1. Also, I see that struct constraint_parse is not able to describe a foreign key - it has no parent table, referenced columns - you pass them separately from struct constraint_parse which looks contr-intuitive. Can you please, elaborate so it, for example, has struct fkey_parse as a member? Or at least, have both parent and child table name and cols as Token and ExprList pointers? > + > /* > * An SQL parser context. A copy of this structure is passed through > * the parser and down into all the parser action routine in order to > @@ -2781,6 +2791,10 @@ struct Parse { > TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */ > With *pWith; /* Current WITH clause, or NULL */ > With *pWithToFree; /* Free this WITH object at the end of the parse */ > + /** > + * Constraint currently being parsed. > + */ > + struct constraint_parse *constraint; 3. Make it an object, not a pointer, please. Anyway you allocate it on each prepare. > /** > * Number of FK constraints declared within > * CREATE TABLE statement. > @@ -4152,8 +4161,7 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred); > * algorithms (e.g. CASCADE, RESTRICT etc). > */ > void > -sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, > - struct Token *constraint, struct ExprList *child_cols, > +sql_create_foreign_key(struct Parse *parse_context, struct ExprList *child_cols, > struct Token *parent, struct ExprList *parent_cols, > bool is_deferred, int actions); 2.2 This is what I've described in point 2 - child part is partially moved into struct contraint_parse, whilst parent still is separate.