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 CD50526191 for ; Tue, 29 Jan 2019 16:26:02 -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 3BRcGJ9XvxVL for ; Tue, 29 Jan 2019 16:26:02 -0500 (EST) Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 E63B32610E for ; Tue, 29 Jan 2019 16:26:00 -0500 (EST) From: "n.pettik" Message-Id: <83A4D524-1B9B-4E09-BF14-87615268DE8D@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_C894B6F6-4A65-4447-BE36-D8DF37D64758" 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 Date: Wed, 30 Jan 2019 00:25:57 +0300 In-Reply-To: <80c8045d-bb51-7c2a-1681-9e4bd25f1c42@tarantool.org> References: <0fcc585532a1f1200a7dfd4a8e911ecf9f2c94aa.1548265148.git.korablev@tarantool.org> <7BDD0736-3DA8-4042-AD78-7799B2843E7B@tarantool.org> <80c8045d-bb51-7c2a-1681-9e4bd25f1c42@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 --Apple-Mail=_C894B6F6-4A65-4447-BE36-D8DF37D64758 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 29 Jan 2019, at 23:20, Vladislav Shpilevoy = wrote: > On 29/01/2019 23:04, n.pettik wrote: >>> 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. >=20 > You haven't, if struct Parse stores a union of all > terminal defs instead of a pointer. Then this field > will have enough memory to fit any def, with all its > base attributes. See the comment 4 from the original > email. And the comment below. >=20 >> 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? >=20 > No, you have no realloc anything, if you just remove '*' from > all 'base' members and use union of all terminal symbols > in struct Parse as I described. Then this union member in struct > Parse will have enough memory to fit any terminal symbol, with > all its inheritance levels (because of 'base' attributes, stored > by value). >=20 > It slightly similar to struct port. Base struct port has > padding, which is able to fit any child attributes, so we > can create base struct port, and then 'develop' it to a more > concrete port without reallocs. Here you will have a similar > situation, if fix my comment 4. Thx for explanation, I=E2=80=99ve got it. I forgot about union, now it=E2=80=99s clear. --Apple-Mail=_C894B6F6-4A65-4447-BE36-D8DF37D64758 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On 29 Jan 2019, at 23:20, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
On 29/01/2019 = 23:04, n.pettik wrote:
On= 29 Jan 2019, at 22:29, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:

Hi! Thanks for the patchset! See 5 comments below.

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.

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.

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;

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.

You haven't, if struct Parse stores a union of all
terminal defs instead of a = pointer. Then this field
will have enough memory to fit any def, with all = its
base = attributes. See the comment 4 from the original
email. And the comment = below.

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?

No, you have no realloc = anything, if you just remove '*' from
all 'base' members and use union of all terminal = symbols
in struct = Parse as I described. Then this union member in struct
Parse will have enough memory to = fit any terminal symbol, with
all its inheritance levels (because of 'base' attributes, = stored
by = value).

It slightly = similar to struct port. Base struct port has
padding, which is able to fit = any child attributes, so we
can create base struct port, and then 'develop' it to a = more
concrete port = without reallocs. Here you will have a similar
situation, if fix my comment = 4.

Thx for = explanation, I=E2=80=99ve got it.
I forgot about union, now = it=E2=80=99s clear.

= --Apple-Mail=_C894B6F6-4A65-4447-BE36-D8DF37D64758--