[tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing

["n.pettik" <korablev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4377 bytes --]



> 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’t 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 ::= …
>>   struct alter_entity_def *alter_def = malloc(sizeof(alter_def));
>> ….
>> Then we can get to “rename” rule or to “add_constraint_start”.
>> rename ::=
>> …
>> struct rename_entity_def *rename_def = malloc (sizeof(alter_def));
>> memcpy(rename_def, alter_def, sizeof(alter_def));
>> free(alter_def);
>> …
>> 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’ve got it.
I forgot about union, now it’s clear.


[-- Attachment #2: Type: text/html, Size: 19700 bytes --]