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 9516B25FC2 for ; Wed, 16 Jan 2019 15:54:20 -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 qE8BbdMZM42B for ; Wed, 16 Jan 2019 15:54:20 -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 3E7C22525F for ; Wed, 16 Jan 2019 15:54:20 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 1/6] sql: move constraint name to struct contraint_parse References: <499ebc6e21ade22cde794f8470bf5900131d42f5.1547035183.git.korablev@tarantool.org> <834ae33b-dddd-3f51-f6f1-ef5bcc24e240@tarantool.org> <3C226FE6-A27A-46CD-8C83-AA839AAF08F2@tarantool.org> From: Vladislav Shpilevoy Message-ID: <01339b2d-9868-3f4e-e4c3-c7b5c5c9e5dd@tarantool.org> Date: Wed, 16 Jan 2019 23:54:09 +0300 MIME-Version: 1.0 In-Reply-To: <3C226FE6-A27A-46CD-8C83-AA839AAF08F2@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: "n.pettik" , tarantool-patches@freelists.org Hi! Thanks for the fixes! See 2 comments below. > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 51a5d01b5..981e3c660 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2697,7 +2697,7 @@ struct fkey_parse { > */ > struct constraint_parse { > /** Name of table which constraint belongs to. */ > - struct SrcList *table_name; > + struct SrcList *table; > /** Name of the constraint currently being parsed. */ > struct Token name; > }; > >> >>> + /** 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? > > According to ANSI only three constraints can be added: > UNIQUE/PK, FK and CHECK. 1. But you've named foreign key system space _fk_constraint ... And here you have struct constraint_parse, which is not able to fit a whole fk *constraint*. > Only name of table and name > of constraint are shared among features of these constraints: > CHECK also has ExprSpan and UNIQUE - columns (child). > I guess it is fair to move child cols to this structure, > but other members seems to be redundant. > Hence, I will add parent name and parent colls > only if you insist. If I were you I would add here both child and parent attributes, is_deferred, action, but who am I to insist on anything? Please, consult server team chat whether we should consider foreign keys as constraints. Or do nothing. I am not a manager, so it is just an advice. I do not insist on elaborating this structure further. (Although it looks unfinished to me.) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 963b16626..af961592a 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > > @@ -727,8 +726,10 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ > &token, 0)); > if (list == NULL) > goto primary_key_exit; > - sql_create_index(pParse, 0, 0, list, 0, SORT_ORDER_ASC, > + pParse->constraint.cols = list; > + sql_create_index(pParse, 0, 0, 0, SORT_ORDER_ASC, > false, SQL_INDEX_TYPE_CONSTRAINT_PK); > + pParse->constraint.cols = NULL; 2. I think, that it is safer to nullify pParse->constraint.cols in the same place where it is deleted, inside sql_create_index. What do you think? Also, it removes code duplication here and in the hunk below. > if (db->mallocFailed) > goto primary_key_exit; > } else if (autoInc) { > @@ -736,8 +737,10 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ > "INTEGER PRIMARY KEY or INT PRIMARY KEY"); > goto primary_key_exit; > } else { > - sql_create_index(pParse, 0, 0, pList, 0, sortOrder, false, > + pParse->constraint.cols = pList; > + sql_create_index(pParse, 0, 0, 0, sortOrder, false, > SQL_INDEX_TYPE_CONSTRAINT_PK); > + pParse->constraint.cols = NULL; > pList = 0; > if (pParse->nErr > 0) > goto primary_key_exit; >