On 27 Mar 2019, at 17:03, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:

On 27/03/2019 16:44, n.pettik wrote:

On 27 Mar 2019, at 16:29, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
On 27/03/2019 16:00, n.pettik wrote:

On 26 Mar 2019, at 21:06, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:

Thanks for the fixes! This commit LGTM.
Lets proceed to the next patches, and start
with a rebase, which is going to be hard.

Ok. Then I would like to clarify some details to avoid wasting time.
In previous patch version, I used next (reworked) grammar to add
FK constraints using ALTER:

cmd ::= alter_table_start alter_table_action .

alter_table_start ::= ALTER TABLE fullname(Z) . (1)

alter_table_action ::= add_constraint_def.
alter_table_action ::= drop_constraint_def.
alter_table_action ::= rename.

add_constraint_def ::= add_constraint_start constraint_def.

add_constraint_start(N) ::= ADD CONSTRAINT nm(Z). (2)
constraint_def ::= foreign_key_def.

foreign_key_def ::= FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T)
                     eidlist_opt(TA) matcharg(M) refargs(R) defer_subclause_opt(D).

Now obviously I can’t use it since foreign_key_def should call
create_fk_def_init() which in turn requires table name and name
of constraint defined in rules (1) and (2).

Why I want to use grammar mentioned above: it allows to remove
code duplication. Rules to parse constraints are defined three times:

1. ccons rule - that is part of column definition: …, a INT REFERENCES t1);
2. tcons rule - that is part of CREATE TABLE: …, CONSTRAINT c FOREIGN KEY …);
3. ALTER TABLE statement

All of them use the same grammar to parse statement starting from
REFERENCES keyword. The same applies to UNIQUE and CHECK
constraints. 

IDK how to avoid using alter_entity_def_init() and create_constraint_def_init()
and at the same time divide constraint definition into several stages.

Ofc, we can still use simple approach like:

cmd ::= ALTER TABLE fullname(Z) ADD CONSTRAINT nm(Z)  FOREIGN KEY
           LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) matcharg(M)
           refargs(R) defer_subclause_opt(D)

cmd ::= ALTER TABLE fullname(Z) ADD CONSTRAINT nm(Z)  UNIQUE
           LP sortlist(X) RP

cmd ::= ALTER TABLE fullname(Z) ADD CONSTRAINT nm(Z)  PRIMARY KEY
           LP sortlist(X) RP

cmd ::= ALTER TABLE fullname(Z) ADD CONSTRAINT nm(Z) CHECK …

cmd ::= ALTER TABLE fullname(Z) RENAME TO nm(N) .

Is this OK?

Obviously, it is not. Why can't you define this?

alter_table_start(T) ::= ALTER TABLE fullname(T)
alter_add_constraint(T, N) ::= alter_table_start(T) ADD CONSTRAINT nm(N).

Lemon can’t use two aliases as rule parameters at the same time.
Instead we can introduce *another one* local struct to hold these names.

Yes, you can define a structure in parse.y to store these two parameters,
and unpack it back inside the concrete rules. It means, that such a
helper struct will never be stored anywhere out of parse.y.

Anyway my initial worry was not about duplication of ALTER TABLE CREATE CONSTRAINT,
but rather of constraints grammar (i.e. starting from FOREIGN KEY…).

For constraints grammar you can consult the standard. I do not remember
how it defines FOREIGN KEY rules, if it does at all. Personally for me
it looks ok.

cmd ::= alter_add_constraint(T, N) FOREIGN KEY ...
cmd ::= alter_add_constraint(T, N) UNIQUE LP sortlist(X) RP
cmd ::= alter_add_constraint(T, N) PRIMARY KEY LP sortlist(X) RP
cmd ::= alter_add_constraint(T, N) CHECK ...
cmd ::= alter_table_start RENAME TO nm(N) .

Then inside each cmd you have both table and constraint names.