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

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

> On 12 Mar 2019, at 15:50, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the fixes! See my comments below,
> fixes at the end of the email, and on the branch
> (it is np/gh-3914-fix-create-index-v2).
> 
>>> Below I listed the calls you do to create each 'terminal' structure.
>>> 
>>> 	TABLE RENAME:
>>> 	alter_entity_def_init
>>> 	rename_entity_def_init
>>> 
>>> 	TABLE CREATE
>>> 	alter_entity_def_init
>>> 	create_entity_def_init
>>> 	create_table_def_init
>>> 
>>> 	TABLE DROP:
>>> 	alter_entity_def_init
>>> 	drop_entity_def_init
>>> 
>>> 	TABLE ADD FK:
>>> 	alter_entity_def_init
>>> 	create_entity_def_init
>>> 	create_constraint_def_init
>>> 	create_fk_def_init
>>> 
>>> 	DROP FK:
>>> 	alter_entity_def_init
>>> 	drop_entity_def_init
>>> 
>>> 	DROP INDEX:
>>> 	alter_entity_def_init
>>> 	drop_entity_def_init
>>> 
>>> 	ADD INDEX:
>>> 	alter_entity_def_init
>>> 	create_entity_def_init
>>> 	create_index_def_init
>>> 
>>> 	CHECK:
>>>        alter_entity_def_init
>>>        create_entity_def_init
>>> 	create_ck_def_init
>>> 
>>> 	CREATE TRIGGER:
>>> 	alter_entity_def_init
>>> 	create_entity_def_init
>>> 	create_trigger_def_init
>>> 
>>> 	DROP TRIGGER:
>>> 	alter_entity_def_init
>>> 	drop_entity_def_init
>>> 
>>> Here are some problems with what you said earlier and with
>>> the whole structure.
>>> 
>>> Firstly, some of sequences are not finished with a an explicit
>>> terminal. For example, *all* DROPs are finished with an abstract
>>> drop_entity_def_init() taking an entity type explicitly. Despite
>>> the words you said earlier that you initialize everything in steps.
>>> So in the case of DROP the rule is violated already. I think, that
>>> you should define static inline helpers in parse_def.h with names
>>> drop_table_def_init(), drop_index_def_init(), drop_check_def_init()
>>> etc.
>> Drop procedure (i.e. arguments filling) is the same for all entities
>> (this situation is likely to remain unchanged). Does it make any
>> sense to duplicate code without any necessity? What these
>> helpers are supposed to do?
> 
> Nothing functional. Just syntax sugar and API consistency. Otherwise
> your statement, that we have a tree of structures with terminal nodes,
> is false. For other nodes you have _table/index/fk etc suffix, but
> drop is just 'entity'. For me it looks odd. The same for rename_entity.
> 
> Strictly speaking, your hierarchy of structures is even not a tree -
> in your code create_table_def and create_view_def store alter_entity_def,
> but do not initialize nor use it.
> 
> Additionally, for not named constraints you do not call
> alter_entity_def_init nor even create_constraint_def_init - you just
> memset all the structure. So your tree in fact is really corrupted and
> ill, sorry.
> 
> Talking of duplication - each wrapper around drop_entity and
> rename_entity would take 11 simple lines:
> 
>    struct drop_<term>_def {
>            struct drop_entity_def base;
>    }
> 
>    static inline void
>    drop_<term>_def_init(struct drop_<term>_def *def,
>                         struct SrcList *parent_name, struct Token *name,
>                         bool if_exist)
>    {
>            drop_entity_def_init(&def->base, parent_name, name, if_exist,
>                                 ENTITY_TYPE_<term>);
>    }
> 
> For compiler it is nothing - functions are inlined, for binary file
> as well. For code and naming consistency it is a big deal. IMO. But
> probably I should stop teaching here - you can leave it as is in terms
> of list entities or listen to my objectives.

Just want to hear pros of this approach (cons are obvious).
Implemented your proposal (also fixed a bit parsing of
DROP TABLE statement):

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 0bc967bf7..2a825284e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1680,20 +1680,21 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 }
 
 /**
- * This routine is called to do the work of a DROP TABLE statement.
+ * This routine is called to do the work of a DROP TABLE and
+ * DROP VIEW statements.
  *
  * @param parse_context Current parsing context.
- * @param is_view True, if statement is really 'DROP VIEW'.
  */
 void
-sql_drop_table(struct Parse *parse_context, bool is_view)
+sql_drop_table(struct Parse *parse_context)
 {
-       struct drop_entity_def drop_def = parse_context->drop_entity_def;
-       assert(drop_def.base.entity_type == ENTITY_TYPE_TABLE);
+       struct drop_entity_def drop_def = parse_context->drop_table_def.base;
        assert(drop_def.base.alter_action == ALTER_ACTION_DROP);
        struct SrcList *table_name_list = drop_def.base.entity_name;
        struct Vdbe *v = sqlite3GetVdbe(parse_context);
        struct sqlite3 *db = parse_context->db;
+       bool is_view = drop_def.base.entity_type == ENTITY_TYPE_VIEW;
+       assert(is_view || drop_def.base.entity_type == ENTITY_TYPE_TABLE);
        if (v == NULL || db->mallocFailed) {
                goto exit_drop_table;
        }
@@ -2016,7 +2017,7 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred)
 void
 sql_drop_foreign_key(struct Parse *parse_context)
 {
-       struct drop_entity_def *drop_def = &parse_context->drop_entity_def;
+       struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
        assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
        assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
        const char *table_name = drop_def->base.entity_name->a[0].zName;
@@ -2555,7 +2556,7 @@ sql_create_index(struct Parse *parse) {
 void
 sql_drop_index(struct Parse *parse_context)
 {
-       struct drop_entity_def *drop_def = &parse_context->drop_entity_def;
+       struct drop_entity_def *drop_def = &parse_context->drop_index_def.base;
        assert(drop_def->base.entity_type == ENTITY_TYPE_INDEX);
        assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
        struct Vdbe *v = sqlite3GetVdbe(parse_context);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index e9643a71a..9aa45752a 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -372,19 +372,18 @@ resolvetype(A) ::= REPLACE.                  {A = ON_CONFLICT_ACTION_REPLACE;}
 ////////////////////////// The DROP TABLE /////////////////////////////////////
 //
 
-cmd ::= drop_start(X) drop_table . {
-  sql_drop_table(pParse, X);
+cmd ::= DROP TABLE ifexists(E) fullname(X) . {
+  struct Token t = Token_nil;
+  drop_table_def_init(&pParse->drop_table_def, X, &t, E);
+  sql_drop_table(pParse);
 }
 
-drop_table ::= ifexists(E) fullname(X) . {
+cmd ::= DROP VIEW ifexists(E) fullname(X) . {
   struct Token t = Token_nil;
-  drop_entity_def_init(&pParse->drop_entity_def, X, &t, E, ENTITY_TYPE_TABLE);
+  drop_view_def_init(&pParse->drop_view_def, X, &t, E);
+  sql_drop_table(pParse);
 }
 
-%type drop_start {bool}
-drop_start(X) ::= DROP VIEW . { X = true; }
-drop_start(X) ::= DROP TABLE . { X = false; }
-
 %type ifexists {int}
 ifexists(A) ::= IF EXISTS.   {A = 1;}
 ifexists(A) ::= .            {A = 0;}
@@ -1305,7 +1304,7 @@ collate(C) ::= COLLATE id.   {C = 1;}
 ///////////////////////////// The DROP INDEX command /////////////////////////
 //
 cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y).   {
-  drop_entity_def_init(&pParse->drop_entity_def, Y, &X, E, ENTITY_TYPE_INDEX);
+  drop_index_def_init(&pParse->drop_index_def, Y, &X, E);
   sql_drop_index(pParse);
 }
 
@@ -1470,8 +1469,7 @@ raisetype(A) ::= FAIL.      {A = ON_CONFLICT_ACTION_FAIL;}
 ////////////////////////  DROP TRIGGER statement //////////////////////////////
 cmd ::= DROP TRIGGER ifexists(NOERR) fullname(X). {
   struct Token t = Token_nil;
-  drop_entity_def_init(&pParse->drop_entity_def, X, &t, NOERR,
-                       ENTITY_TYPE_TRIGGER);
+  drop_trigger_def_init(&pParse->drop_trigger_def, X, &t, NOERR);
   sql_drop_trigger(pParse);
 }
 
@@ -1494,7 +1492,7 @@ cmd ::= ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY
 }
 
 cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
-  drop_entity_def_init(&pParse->drop_entity_def, X, &Z, false, ENTITY_TYPE_FK);
+  drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
   sql_drop_foreign_key(pParse);
 }
 
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index c7640c523..3fc65b065 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -204,6 +204,35 @@ struct drop_entity_def {
        bool if_exist;
 };
 
+/**
+ * Identical wrappers around drop_entity_def to make hierarchy of
+ * structures be consistent. Arguments for drop procedures are
+ * the same.
+ */
+struct drop_table_def {
+       struct drop_entity_def base;
+};
+
+struct drop_view_def {
+       struct drop_entity_def base;
+};
+
+struct drop_trigger_def {
+       struct drop_entity_def base;
+};
+
+struct drop_fk_def {
+       struct drop_entity_def base;
+};
+
+struct drop_index_def {
+       struct drop_entity_def base;
+};
+
 struct create_trigger_def {
        struct create_entity_def base;
        /** One of TK_BEFORE, TK_AFTER, TK_INSTEAD. */
@@ -301,6 +330,60 @@ drop_entity_def_init(struct drop_entity_def *drop_def,
        drop_def->if_exist = if_exist;
 }
 
+static inline void
+drop_table_def_init(struct drop_table_def *drop_table_def,
+                   struct SrcList *parent_name, struct Token *name,
+                   bool if_exist)
+{
+       drop_entity_def_init(&drop_table_def->base, parent_name, name, if_exist,
+                            ENTITY_TYPE_TABLE);
+}
+
+static inline void
+drop_view_def_init(struct drop_view_def *drop_view_def,
+                  struct SrcList *parent_name, struct Token *name,
+                  bool if_exist)
+{
+       drop_entity_def_init(&drop_view_def->base, parent_name, name, if_exist,
+                            ENTITY_TYPE_VIEW);
+}
+
+static inline void
+drop_trigger_def_init(struct drop_trigger_def *drop_trigger_def,
+                     struct SrcList *parent_name, struct Token *name,
+                     bool if_exist)
+{
+       drop_entity_def_init(&drop_trigger_def->base, parent_name, name,
+                            if_exist, ENTITY_TYPE_TRIGGER);
+}
+
+static inline void
+drop_fk_def_init(struct drop_fk_def *drop_fk_def,
+                struct SrcList *parent_name, struct Token *name,
+                bool if_exist)
+{
+       drop_entity_def_init(&drop_fk_def->base, parent_name, name, if_exist,
+                            ENTITY_TYPE_FK);
+}
+
+static inline void
+drop_index_def_init(struct drop_index_def *drop_index_def,
+                   struct SrcList *parent_name, struct Token *name,
+                   bool if_exist)
+{
+       drop_entity_def_init(&drop_index_def->base, parent_name, name, if_exist,
+                            ENTITY_TYPE_INDEX);
+}
+
 static inline void
 create_trigger_def_init(struct create_trigger_def *trigger_def,
                        struct SrcList *table_name, struct Token *name,
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index f8ec3519e..a0eb9ab38 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2738,7 +2738,12 @@ struct Parse {
                struct create_trigger_def create_trigger_def;
                struct create_view_def create_view_def;
                struct rename_entity_def rename_entity_def;
-               struct drop_entity_def drop_entity_def;
+               struct drop_fk_def drop_fk_def;
+               struct drop_index_def drop_index_def;
+               struct drop_table_def drop_table_def;
+               struct drop_trigger_def drop_trigger_def;
+               struct drop_view_def drop_view_def;
        };
        struct create_table_def create_table_def;
        /**
@@ -3417,7 +3422,7 @@ void
 sql_store_select(struct Parse *parse_context, struct Select *select);
 
 void
-sql_drop_table(struct Parse *, bool);
+sql_drop_table(struct Parse *parse);
 void sqlite3DeleteTable(sqlite3 *, Table *);
 void sqlite3Insert(Parse *, SrcList *, Select *, IdList *,
                   enum on_conflict_action);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 3456a244d..f85f0dc35 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -425,7 +425,7 @@ vdbe_code_drop_trigger(struct Parse *parser, const char *trigger_name,
 void
 sql_drop_trigger(struct Parse *parser)
 {
-       struct drop_entity_def *drop_def = &parser->drop_entity_def;
+       struct drop_entity_def *drop_def = &parser->drop_trigger_def.base;
        struct alter_entity_def *alter_def = &drop_def->base;
        assert(alter_def->entity_type == ENTITY_TYPE_TRIGGER);
        assert(alter_def->alter_action == ALTER_ACTION_DROP);

> Anyway you are likely to be a
> single maintainer of all of this for a long time.

Sounds like a threat. I’d rather not :)

>>> Secondly, some words about create_entity_def_init(). It is the
>>> most arguable function for a public usage (out of parse_def.h)
>>> because hardly I can imagine a situation when you parsed a word
>>> CREATE, but can not parse a next one: TABLE, TRIGGER, CONSTRAINT etc.
>>> And we see it in the lists above. Below I listed all of this function
>>> usage cases and how you can replace that function here.
>>> 
>>> - create_entity_def_init() in create_table ::= rule, line 172. Here
>>>  it is followed by create_table_def_init() on the next line.
>>> 
>>> - create_entity_def_init() in cconsname_parse ::= rule line 250.
>>>  This rule is a prefix for a constraint definition, so this function
>>>  can be substituted with create_constraint_def_init().
>>> 
>>> - create_entity_def_init() in cmd ::= rule (about CREATE VIEW),
>>>  line 400. Here we see, that 1) you missed ENTITY_TYPE_VIEW,
>>>  2) create_view_def_init() can be called here.
>>> 
>>> - create_entity_def_init() in cmd ::= rule (about CREATE INDEX),
>>>  line 1246. Here it is followed by create_index_def_init() on the
>>>  next line.
>>> 
>>> - create_entity_def_init() in trigger_decl ::= rule, line 1367.
>>>  Here it is followed by create_trigger_def_init().
>>> 
>>> - create_entity_def_init() in cmd ::= rule (about ALTER TABLE ADD
>>>  CONSTRAINT FOREIGN KEY), line 1500. Here it is followed by
>>>  create_constraint_def_init().
>>> 
>>> So as you can see create_entity_def_init() is not necessary to use
>>> out of parse_def.h. You can put it into
>>> create_table/index/constraint_def().
>> Ok, done. See updated version at the end of letter.
>>> Now about create_constraint_def_init(). In your patch it is always
>>> followed by create_fk_def_init().
>> I didn’t get this point. create_constraint_def_init() may come before
>> create_fk_def_init() as well as before create_index_def/create_ck_def.
> 
> The only place, where create_constraint_def_init can not be replaced with
> a next level term, is a rule "cconsname_parse ::= CONSTRAINT nm(X).". But
> you did not answer below why do you need it here.
> 
>>> But after you did the comment above
>>> about create_entity_def_init(), you will have create_constraint_def_init()
>>> in cconsname_parse ::= CONSTRAINT nm(X) rule. And I do not see any
>>> concrete reasons why can not you remove it from here and use only
>>> create_index_def_init(), create_ck_def_init() etc in ccons ::= rule.
>>> You said, that some of pointers should be nullifed, but what pointers
>>> and why? What if you do not nullify them right after CONSTRAINT word
>>> is met?
>> We had to nullify them since in previous versions members of some
>> structures might leave uninitialised. Now _init funs fill all members,
>> so there is no any problem.
> 
> I see, that you nullify them, but you did not answer my questions. Why
> can not you postpone that nullification until a next word is met -
> UNIQUE, PK, FK, CK etc? It would allow you to fold
> create_constraint_def_init and always use a term def.
> 
> What is more, you initialize named constraints twice - first you
> nullify them in cconsname_start with memset, and then call
> create_constraint_def_init.
> 
> I did my own investigation of this matter, and managed to remove
> cconsname_start rule, as well as double init. After that I have
> cconsname_parse followed by many term constraint rules, and I tried to
> figure out why can't we just inline cconsname into terms ccons and
> tcons and remove create_constraint_def_init from public. Especially
> taking into account that it is not many changes thanks to that:
> https://github.com/tarantool/tarantool/issues/3820.
> 
> It means, that we could inline 'CONSTRAINT nm(X)' in 8 places only:
> 
>    1) "ccons ::= PRIMARY KEY"
>    2) "ccons ::= UNIQUE"
>    3) "ccons ::= check_constraint_def"
>    4) "ccons ::= REFERENCES"
> 
>     + the same for tcons.
> 
> But then I realized, that cconsname helps us to keep the name
> optional. To solve that we need to somehow save nm(X) result until
> a terminal constraint is reached, or nullify it. I think, that we
> could add struct Token constraint_name to union of alter entities in
> struct Parse, and fetch it from there in terminal constraint
> definitions (PK, UNIQUE, CK, FK). It is possible, because in such
> a case this union's memory is not occupied by create_constraint_def
> until term. It will allow us to remove create_constraint_def_init
> from public usage. And to close the issue 3820 alongside.

This approach brings us to the very first implementation,
when name of constraint was saved as Token in struct Parse…

But I’ve done this (I’m still not sure whether you meant this approach or not):

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index b66ac08e2..aa16cb874 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -185,10 +185,10 @@ create_table_args ::= AS select(S). {
   sqlEndTable(pParse,0,S);
   sql_select_delete(pParse->db, S);
 }
-columnlist ::= columnlist COMMA tconsdef.
+columnlist ::= columnlist COMMA tcons.
 columnlist ::= columnlist COMMA columnname carglist.
 columnlist ::= columnname carglist.
-columnlist ::= tconsdef.
+columnlist ::= tcons.
 columnname(A) ::= nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);}
 
 // An IDENTIFIER can be a generic identifier, or one of several
@@ -236,18 +236,11 @@ nm(A) ::= id(A). {
 // "carglist" is a list of additional constraints that come after the
 // column name and column type in a CREATE TABLE statement.
 //
-carglist ::= carglist cconsdef.
+carglist ::= carglist ccons.
 carglist ::= .
-cconsdef ::= cconsname ccons.
-cconsname ::= CONSTRAINT nm(X). {
-  create_constraint_def_init(&pParse->create_constraint_def, NULL, &X, false,
-                             false);
-}
-cconsname ::= . {
-  struct Token t = Token_nil;
-  create_constraint_def_init(&pParse->create_constraint_def, NULL, &t, false,
-                             false);
-}
+%type cconsname { struct Token }
+cconsname(N) ::= CONSTRAINT nm(X). { N = X; }
+cconsname(N) ::= . { N = Token_nil; }
 ccons ::= DEFAULT term(X).            {sqlAddDefaultValue(pParse,&X);}
 ccons ::= DEFAULT LP expr(X) RP.      {sqlAddDefaultValue(pParse,&X);}
 ccons ::= DEFAULT PLUS term(X).       {sqlAddDefaultValue(pParse,&X);}
@@ -269,27 +262,29 @@ ccons ::= NULL onconf(R).        {
         sql_column_add_nullable_action(pParse, R);
 }
 ccons ::= NOT NULL onconf(R).    {sql_column_add_nullable_action(pParse, R);}
-ccons ::= PRIMARY KEY sortorder(Z) autoinc(I). {
+ccons ::= cconsname(N) PRIMARY KEY sortorder(Z) autoinc(I). {
   pParse->create_table_def.has_autoinc = I;
-  create_index_def_init(&pParse->create_index_def, NULL,
-                        SQL_INDEX_TYPE_CONSTRAINT_PK, Z);
+  create_index_def_init(&pParse->create_index_def, NULL, &N, NULL,
+                        SQL_INDEX_TYPE_CONSTRAINT_PK, Z, false);
   sqlAddPrimaryKey(pParse);
 }
-ccons ::= UNIQUE. {
-  create_index_def_init(&pParse->create_index_def, NULL,
-                        SQL_INDEX_TYPE_CONSTRAINT_UNIQUE, SORT_ORDER_ASC);
+ccons ::= cconsname(N) UNIQUE. {
+  create_index_def_init(&pParse->create_index_def, NULL, &N, NULL,
+                        SQL_INDEX_TYPE_CONSTRAINT_UNIQUE, SORT_ORDER_ASC,
+                        false);
   sql_create_index(pParse);
 }
 
 ccons ::= check_constraint_def .
 
-check_constraint_def ::= CHECK LP expr(X) RP. {
-  create_ck_def_init(&pParse->create_ck_def, &X);
+check_constraint_def ::= cconsname(N) CHECK LP expr(X) RP. {
+  create_ck_def_init(&pParse->create_ck_def, &N, &X);
   sql_add_check_constraint(pParse);
 }
 
-ccons ::= REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R). {
-  create_fk_def_init(&pParse->create_fk_def, NULL, &T, TA, M, R);
+ccons ::= cconsname(N) REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R). {
+  create_fk_def_init(&pParse->create_fk_def, NULL, &N, NULL, &T, TA, M, R,
+                     false);
   sql_create_foreign_key(pParse);
 }
 ccons ::= defer_subclause(D).    {fk_constraint_change_defer_mode(pParse, D);}
@@ -337,23 +332,22 @@ init_deferred_pred_opt(A) ::= .                       {A = 0;}
 init_deferred_pred_opt(A) ::= INITIALLY DEFERRED.     {A = 1;}
 init_deferred_pred_opt(A) ::= INITIALLY IMMEDIATE.    {A = 0;}
 
-tconsdef ::= cconsname tcons.
-tcons ::= PRIMARY KEY LP sortlist(X) autoinc(I) RP. {
+tcons ::= cconsname(N) PRIMARY KEY LP sortlist(X) autoinc(I) RP. {
   pParse->create_table_def.has_autoinc = I;
-  create_index_def_init(&pParse->create_index_def, X,
-                        SQL_INDEX_TYPE_CONSTRAINT_PK, SORT_ORDER_ASC);
+  create_index_def_init(&pParse->create_index_def, NULL, &N, X,
+                        SQL_INDEX_TYPE_CONSTRAINT_PK, SORT_ORDER_ASC, false);
   sqlAddPrimaryKey(pParse);
 }
-tcons ::= UNIQUE LP sortlist(X) RP. {
-  create_index_def_init(&pParse->create_index_def, X,
-                        SQL_INDEX_TYPE_CONSTRAINT_UNIQUE, SORT_ORDER_ASC);
+tcons ::= cconsname(N) UNIQUE LP sortlist(X) RP. {
+  create_index_def_init(&pParse->create_index_def, NULL, &N, X,
+                        SQL_INDEX_TYPE_CONSTRAINT_UNIQUE, SORT_ORDER_ASC,
+                        false);
   sql_create_index(pParse);
 }
 tcons ::= check_constraint_def .
-tcons ::= FOREIGN KEY LP eidlist(FA) RP
+tcons ::= cconsname(N) FOREIGN KEY LP eidlist(FA) RP
           REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R) defer_subclause_opt(D). {
-  pParse->create_fk_def.base.is_deferred = D;
-  create_fk_def_init(&pParse->create_fk_def, FA, &T, TA, M, R);
+  create_fk_def_init(&pParse->create_fk_def, NULL, &N, FA, &T, TA, M, R, D);
   sql_create_foreign_key(pParse);
 }
 %type defer_subclause_opt {int}
@@ -1257,10 +1251,9 @@ paren_exprlist(A) ::= LP exprlist(X) RP.  {A = X;}
 //
 cmd ::= createkw uniqueflag(U) INDEX ifnotexists(NE) nm(X)
         ON nm(Y) LP sortlist(Z) RP. {
-  create_constraint_def_init(&pParse->create_index_def.base,
-                             sqlSrcListAppend(pParse->db, 0, &Y), &X, NE,
-                             false);
-  create_index_def_init(&pParse->create_index_def, Z, U, SORT_ORDER_ASC);
+  create_index_def_init(&pParse->create_index_def,
+                        sqlSrcListAppend(pParse->db, 0, &Y), &X, Z, U,
+                        SORT_ORDER_ASC, NE);
   sql_create_index(pParse);
 }
 
@@ -1494,8 +1487,7 @@ cmd ::= ALTER TABLE fullname(X) RENAME TO nm(Z). {
 cmd ::= ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY
         LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) matcharg(M)
         refargs(R) defer_subclause_opt(D). {
-  create_constraint_def_init(&pParse->create_fk_def.base, X, &Z, D, false);
-  create_fk_def_init(&pParse->create_fk_def, FA, &T, TA, M, R);
+  create_fk_def_init(&pParse->create_fk_def, X, &Z, FA, &T, TA, M, R, D);
   sql_create_foreign_key(pParse);
 }
 
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index becbf99f4..374d655fb 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -320,9 +320,10 @@ create_entity_def_init(struct create_entity_def *create_def,
 static inline void
 create_constraint_def_init(struct create_constraint_def *constr_def,
                           struct SrcList *parent_name, struct Token *name,
-                          bool if_not_exists, bool is_deferred)
+                          bool if_not_exists, bool is_deferred,
+                          enum entity_type entity_type)
 {
-       create_entity_def_init(&constr_def->base, ENTITY_TYPE_CONSTRAINT,
+       create_entity_def_init(&constr_def->base, entity_type,
                               parent_name, name, if_not_exists);
        constr_def->is_deferred = is_deferred;
 }
@@ -405,38 +406,35 @@ create_trigger_def_init(struct create_trigger_def *trigger_def,
 }
 
 static inline void
-create_ck_def_init(struct create_ck_def *ck_def, struct ExprSpan *expr)
+create_ck_def_init(struct create_ck_def *ck_def, struct Token *name,
+                  struct ExprSpan *expr)
 {
-       struct alter_entity_def *alter_def = (struct alter_entity_def *) ck_def;
-       assert(alter_def->alter_action == ALTER_ACTION_CREATE);
-       assert(alter_def->entity_type == ENTITY_TYPE_CONSTRAINT);
-       alter_def->entity_type = ENTITY_TYPE_CK;
+       create_constraint_def_init(&ck_def->base, NULL, name, false,
+                                  false, ENTITY_TYPE_CK);
        ck_def->expr = expr;
 }
 
 static inline void
-create_index_def_init(struct create_index_def *index_def, struct ExprList *cols,
-                     enum sql_index_type idx_type, enum sort_order sort_order)
+create_index_def_init(struct create_index_def *index_def,
+                     struct SrcList *table_name,  struct Token *name,
+                     struct ExprList *cols, enum sql_index_type idx_type,
+                     enum sort_order sort_order, bool if_not_exists)
 {
-       struct alter_entity_def *alter_def =
-               (struct alter_entity_def *) index_def;
-       assert(alter_def->alter_action == ALTER_ACTION_CREATE);
-       assert(alter_def->entity_type == ENTITY_TYPE_CONSTRAINT);
-       alter_def->entity_type = ENTITY_TYPE_INDEX;
+       create_constraint_def_init(&index_def->base, table_name, name,
+                                  if_not_exists, false, ENTITY_TYPE_INDEX);
        index_def->cols = cols;
        index_def->idx_type = idx_type;
        index_def->sort_order = sort_order;
 }
 
 static inline void
-create_fk_def_init(struct create_fk_def *fk_def, struct ExprList *child_cols,
+create_fk_def_init(struct create_fk_def *fk_def, struct SrcList *table_name,
+                  struct Token *name, struct ExprList *child_cols,
                   struct Token *parent_name, struct ExprList *parent_cols,
-                  int match, int actions)
+                  int match, int actions, bool is_deferred)
 {
-       struct alter_entity_def *alter_def = (struct alter_entity_def *) fk_def;
-       assert(alter_def->alter_action == ALTER_ACTION_CREATE);
-       assert(alter_def->entity_type == ENTITY_TYPE_CONSTRAINT);
-       alter_def->entity_type = ENTITY_TYPE_FK;
+       create_constraint_def_init(&fk_def->base, table_name, name,
+                                  false, is_deferred, ENTITY_TYPE_FK);
        fk_def->child_cols = child_cols;
        fk_def->parent_name = parent_name;
        fk_def->parent_cols = parent_cols;

diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 97bf30ec6..2048e12ea 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2684,7 +2684,6 @@ struct Parse {
                struct create_ck_def create_ck_def;
                struct create_fk_def create_fk_def;
                struct create_index_def create_index_def;
-               struct create_constraint_def create_constraint_def;
                struct create_trigger_def create_trigger_def;
                struct create_view_def create_view_def;
                struct rename_entity_def rename_entity_def;

diff --git a/test/sql/misc.result b/test/sql/misc.result
index ef104c1c5..5afae3360 100644
--- a/test/sql/misc.result
+++ b/test/sql/misc.result
@@ -40,3 +40,17 @@ box.sql.execute('\n\n\n\t\t\t   ')
 ---
 - error: 'syntax error: empty request'
 ...
+-- gh-3820: only table constraints can have a name.
+--
+box.sql.execute('CREATE TABLE test (id INTEGER PRIMARY KEY, b INTEGER CONSTRAINT c1 NULL)')
+---
+- error: keyword "NULL" is reserved
+...
+box.sql.execute('CREATE TABLE test (id INTEGER PRIMARY KEY, b INTEGER CONSTRAINT c1 DEFAULT 300)')
+---
+- error: keyword "DEFAULT" is reserved
+...
+box.sql.execute('CREATE TABLE test (id INTEGER PRIMARY KEY, b TEXT CONSTRAINT c1 COLLATE "binary")')
+---
+- error: keyword "COLLATE" is reserved
+...
diff --git a/test/sql/misc.test.lua b/test/sql/misc.test.lua
index 994e64f3a..e79e4841b 100644
--- a/test/sql/misc.test.lua
+++ b/test/sql/misc.test.lua
@@ -11,3 +11,9 @@ box.sql.execute(';')
 box.sql.execute('')
 box.sql.execute('     ;')
 box.sql.execute('\n\n\n\t\t\t   ')
+
+-- gh-3820: only table constraints can have a name.
+--
+box.sql.execute('CREATE TABLE test (id INTEGER PRIMARY KEY, b INTEGER CONSTRAINT c1 NULL)')
+box.sql.execute('CREATE TABLE test (id INTEGER PRIMARY KEY, b INTEGER CONSTRAINT c1 DEFAULT 300)')
+box.sql.execute('CREATE TABLE test (id INTEGER PRIMARY KEY, b TEXT CONSTRAINT c1 COLLATE "binary")’)

So, what I did:

1. Incapsulated call of create_constraint_def into parse_def.h
2. Fixed 3820 issue: moved cconsname to rules which are related to constraints
3. Avoided adding struct Token constraint_name to struct Parse and removed
    struct create_constraint_def from union.

> This is what I wanted to hear from you.
> 
> Besides, I have some lowlevel comments on the code.
> 
> 1) Usually we do not pass structures by value, even such small
> as struct Token.

Ok, I see you’ve already fixed this point, so skipped.

> 2) You've moved struct Token into parse_def.h, but kept its
> routines in sqlite3Int.h. Please, move them too.

It’s not that easy. For instance, sqlTokenInit() uses sqlStrlen30(),
which declaration is placed in sqlInt.h. Ok, we can use common
strlen(). Next, sqlNameFromToken() uses sqlDbStrNDup() and
sqlNormalizeName(), which in turn again are declared in sqlInt.h
We can’t include sqlInt.h, otherwise we will get cyclic dependencies.
Also I wanted to move sqlJoinType() (it is used only in parse.y and
involves tokens routine), but it uses struct Parse to set error.
I guess after replacing saving error message in struct Parse with
diag_set(), we will be able to move it to parse.y

So, the only things I’ve really managed to move are
sqlTokenInit() and array sqlIntTokens[]:

diff --git a/src/box/sql/CMakeLists.txt b/src/box/sql/CMakeLists.txt
index 87fb83358..b9dbe141a 100644
--- a/src/box/sql/CMakeLists.txt
+++ b/src/box/sql/CMakeLists.txt
@@ -43,6 +43,7 @@ add_library(sql STATIC
     malloc.c
     os.c
     os_unix.c
+    parse_def.c
     pragma.c
     prepare.c
     printf.c
diff --git a/src/box/sql/global.c b/src/box/sql/global.c
index 95ad71c38..159b35e64 100644
--- a/src/box/sql/global.c
+++ b/src/box/sql/global.c
@@ -222,14 +222,6 @@ SQL_WSD struct sqlConfig sqlConfig = {
  */
 FuncDefHash sqlBuiltinFunctions;
 
-/*
- * Constant tokens for values 0 and 1.
- */
-const Token sqlIntTokens[] = {
-       {"0", 1, false},
-       {"1", 1, false}
-};
-
 /*
  * The value of the "pending" byte must be 0x40000000 (1 byte past the
  * 1-gibabyte boundary) in a compatible database.  sql never uses
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index b68ca64c2..becbf99f4 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -87,6 +87,13 @@ struct Token {
        bool isReserved;
 };
 
+/** Constant tokens for values 0 and 1. */
+extern const struct Token sqlIntTokens[];
+
+/** Generate a Token object from a string. */
+void
+sqlTokenInit(struct Token *p, char *z);
+
 #define Token_nil ((struct Token) {NULL, 0, false})
 
 /**
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index e3740b46b..97bf30ec6 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3151,7 +3151,6 @@ void sqlSetString(char **, sql *, const char *);
 void sqlErrorMsg(Parse *, const char *, ...);
 void sqlDequote(char *);
 void sqlNormalizeName(char *z);
-void sqlTokenInit(Token *, char *);
 int sqlKeywordCode(const unsigned char *, int);
 int sqlRunParser(Parse *, const char *, char **);
 
diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index c89e2e8ab..f6ec763bc 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -311,16 +311,6 @@ sqlNormalizeName(char *z)
        }
 }
 
-/*
- * Generate a Token object from a string
- */
-void
-sqlTokenInit(Token * p, char *z)
-{
-       p->z = z;
-       p->n = sqlStrlen30(z);
-}
-
 /* Convenient short-hand */
 #define UpperToLower sqlUpperToLower 

diff --git a/src/box/sql/parse_def.c b/src/box/sql/parse_def.c
new file mode 100644
index 000000000..7310726ff
--- /dev/null
+++ b/src/box/sql/parse_def.c
@@ -0,0 +1,45 @@
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#include <string.h>
+
+#include "parse_def.h"
+
+const struct Token sqlIntTokens[] = {
+       {"0", 1, false},
+       {"1", 1, false}
+};
+
+void
+sqlTokenInit(struct Token *p, char *z)
+{
+       p->z = z;
+       p->n = strlen(z);
+}

I’m not sure whether these tiny things deserve their own
implementation file. On the other hand, it is to be extended
in future (when we decide to split whole sqlInt.h into ordinary
headers).

> And please,
> rebase - 'sqlite3' has already been replaced with 'sql’.

Ok, rebase has been done.

> 3) Since now and after point 2 we have internal and long functions
> in parse_def, I think it is time to introduce parse_def.c and hide
> them here.
> 
> ====================================================================
> 
> My diff below and on the branch:

Thx, applied as quite straightforward.