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 3325324C12 for ; Tue, 29 Jan 2019 15:04:38 -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 Hw1UyS2u8mJu for ; Tue, 29 Jan 2019 15:04:38 -0500 (EST) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id A34B824933 for ; Tue, 29 Jan 2019 15:04:37 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing From: "n.pettik" In-Reply-To: Date: Tue, 29 Jan 2019 23:04:35 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <7BDD0736-3DA8-4042-AD78-7799B2843E7B@tarantool.org> References: <0fcc585532a1f1200a7dfd4a8e911ecf9f2c94aa.1548265148.git.korablev@tarantool.org> 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 Cc: Vladislav Shpilevoy > On 29 Jan 2019, at 22:29, Vladislav Shpilevoy = wrote: >=20 > Hi! Thanks for the patchset! See 5 comments below. >=20 >> What is more, we are going to introduce ALTER TABLE ADD CONSTRAINT >> UNIQUE With new hierarchy we can extend ALTER TABLE statement with = ease: >> basic structures (alter -> create entity -> create constraint) are = the >> same for .. FOREIGN KEY/UNIQUE, but the last one will be different. >> Note that grammar for CREATE TABLE statement is not trivial and = consists >> of wide range of sub-clauses (e.g. to parse foreign key or check >> constraints). Therefore, parts of space definition are assembled >> as soon as parser processes sub-rules. For this reason, current patch >> doesn't affect CREATE TABLE handling. >=20 > 1. Why? I do not think, that "create table" port into parse_def.h is > impossible and follows from the facts above. 'Not trivial' does > not mean impossible. >=20 > As I see, you can create struct create_table_def, which will contain > Table *pNewTable, struct rlist new_fkey, uint32_t fkey_count, > bool is_new_table_autoinc. Just move some attributes from struct Parse > to this new structure. Is it possible? I think, it is worth doing, > because the code will look more consistent - any object change will go > through parse_def.h structures. Ok, it is possible. >> +struct alter_entity_def { >> + /** As a rule it is a name of table to be altered. */ >> + struct SrcList *entity_name; >> +}; >> + >> +struct rename_entity_def { >> + struct alter_entity_def *base; >=20 > 2. Please, look at how we usually do inheritance in C. > We include base structure into a child as an attribute, > not as a pointer. So you can cast the child to the parent > even without touching 'base' field. Also, this change > together with comment 4 allows to remove parse_def.c > completely. Surely, before implementing this patch I checked out inheritance examples in our code base. But here we have slightly different case (or I misunderstood concepts). For instance, when we create memtx space, we allocate enough space for base space and for memtx parent at once. When we are parsing ALTER TABLE, we are able to allocate memory only for base structure (alter_def), since we don=E2=80=99t know where we will get - whether to drop constraint or create constraint or rename. If we used similar inheritance model, we would have to realloc enough memory (for base + parent) each time we get to the next rule. What I mean: alter_table_start ::=3D =E2=80=A6 struct alter_entity_def *alter_def =3D malloc(sizeof(alter_def)); =E2=80=A6. Then we can get to =E2=80=9Crename=E2=80=9D rule or to = =E2=80=9Cadd_constraint_start=E2=80=9D. rename ::=3D =E2=80=A6 struct rename_entity_def *rename_def =3D malloc (sizeof(alter_def)); memcpy(rename_def, alter_def, sizeof(alter_def)); free(alter_def); =E2=80=A6 The same allocation and free procedures would be called inside add_constraint_start. Or, as an option, we can intensely use region allocation. But then we are going to waste a lot of memory. Is it what you talked about? Or I am wrong and you meant easier way?