> 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: >>> >>> 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.