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.