[tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Mar 12 15:50:46 MSK 2019
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. Anyway you are likely to be a
single maintainer of all of this for a long time.
>
>> 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 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.
2) You've moved struct Token into parse_def.h, but kept its
routines in sqlite3Int.h. Please, move them too. And please,
rebase - 'sqlite3' has already been replaced with 'sql'.
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:
commit 68499ad902d7315447e83e77b83ee8c93db5e239
Author: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
Date: Tue Mar 12 13:49:30 2019 +0300
Review fixes
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 1f7678a4b..e9643a71a 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -168,7 +168,7 @@ cmd ::= ROLLBACK TO savepoint_opt nm(X). {
//
cmd ::= create_table create_table_args.
create_table ::= createkw TABLE ifnotexists(E) nm(Y). {
- create_table_def_init(&pParse->create_table_def, Y, E);
+ create_table_def_init(&pParse->create_table_def, &Y, E);
pParse->create_table_def.new_table = sqlite3StartTable(pParse, &Y, E);
}
createkw(A) ::= CREATE(A). {disableLookaside(pParse);}
@@ -239,15 +239,15 @@ nm(A) ::= id(A). {
carglist ::= carglist cconsdef.
carglist ::= .
cconsdef ::= cconsname ccons.
-cconsname ::= cconsname_start cconsname_parse .
-cconsname_start ::= . {
- memset(&pParse->create_index_def.base, 0, sizeof(struct create_constraint_def));
+cconsname ::= CONSTRAINT nm(X). {
+ create_constraint_def_init(&pParse->create_constraint_def, NULL, &X, false,
+ false);
}
-cconsname_parse ::= CONSTRAINT nm(X). {
- create_constraint_def_init(&pParse->create_index_def.base, NULL, X, false,
+cconsname ::= . {
+ struct Token t = Token_nil;
+ create_constraint_def_init(&pParse->create_constraint_def, NULL, &t, false,
false);
}
-cconsname_parse ::= .
ccons ::= DEFAULT term(X). {sqlite3AddDefaultValue(pParse,&X);}
ccons ::= DEFAULT LP expr(X) RP. {sqlite3AddDefaultValue(pParse,&X);}
ccons ::= DEFAULT PLUS term(X). {sqlite3AddDefaultValue(pParse,&X);}
@@ -346,7 +346,7 @@ tcons ::= UNIQUE LP sortlist(X) RP. {
tcons ::= check_constraint_def .
tcons ::= FOREIGN KEY LP eidlist(FA) RP
REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). {
- ((struct create_constraint_def *) &pParse->create_fk_def)->is_deferred = D;
+ pParse->create_fk_def.base.is_deferred = D;
create_fk_def_init(&pParse->create_fk_def, FA, &T, TA, R);
sql_create_foreign_key(pParse);
}
@@ -377,8 +377,8 @@ cmd ::= drop_start(X) drop_table . {
}
drop_table ::= ifexists(E) fullname(X) . {
- drop_entity_def_init(&pParse->drop_entity_def, X, (struct Token) {}, E,
- ENTITY_TYPE_TABLE);
+ struct Token t = Token_nil;
+ drop_entity_def_init(&pParse->drop_entity_def, X, &t, E, ENTITY_TYPE_TABLE);
}
%type drop_start {bool}
@@ -394,7 +394,7 @@ ifexists(A) ::= . {A = 0;}
cmd ::= createkw(X) VIEW ifnotexists(E) nm(Y) eidlist_opt(C)
AS select(S). {
if (!pParse->parse_only) {
- create_view_def_init(&pParse->create_view_def, Y, &X, C, S, E);
+ create_view_def_init(&pParse->create_view_def, &Y, &X, C, S, E);
sql_create_view(pParse);
} else {
sql_store_select(pParse, S);
@@ -1239,7 +1239,7 @@ 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,
- sqlite3SrcListAppend(pParse->db, 0, &Y), X, NE,
+ sqlite3SrcListAppend(pParse->db, 0, &Y), &X, NE,
false);
create_index_def_init(&pParse->create_index_def, Z, U, SORT_ORDER_ASC);
sql_create_index(pParse);
@@ -1305,7 +1305,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_entity_def_init(&pParse->drop_entity_def, Y, &X, E, ENTITY_TYPE_INDEX);
sql_drop_index(pParse);
}
@@ -1357,8 +1357,8 @@ cmd ::= createkw trigger_decl(A) BEGIN trigger_cmd_list(S) END(Z). {
trigger_decl(A) ::= TRIGGER ifnotexists(NOERR) nm(B)
trigger_time(C) trigger_event(D)
ON fullname(E) foreach_clause when_clause(G). {
- create_trigger_def_init(&pParse->create_trigger_def, E, B, C, D.a, D.b,
- G, NOERR);
+ create_trigger_def_init(&pParse->create_trigger_def, E, &B, C, D.a, D.b, G,
+ NOERR);
sql_trigger_begin(pParse);
A = B; /*A-overwrites-T*/
}
@@ -1469,7 +1469,8 @@ raisetype(A) ::= FAIL. {A = ON_CONFLICT_ACTION_FAIL;}
//////////////////////// DROP TRIGGER statement //////////////////////////////
cmd ::= DROP TRIGGER ifexists(NOERR) fullname(X). {
- drop_entity_def_init(&pParse->drop_entity_def, X, (struct Token){}, NOERR,
+ struct Token t = Token_nil;
+ drop_entity_def_init(&pParse->drop_entity_def, X, &t, NOERR,
ENTITY_TYPE_TRIGGER);
sql_drop_trigger(pParse);
}
@@ -1480,20 +1481,20 @@ cmd ::= ANALYZE nm(X). {sqlite3Analyze(pParse, &X);}
//////////////////////// ALTER TABLE table ... ////////////////////////////////
cmd ::= ALTER TABLE fullname(X) RENAME TO nm(Z). {
- rename_entity_def_init(&pParse->rename_entity_def, X, Z);
+ rename_entity_def_init(&pParse->rename_entity_def, X, &Z);
sql_alter_table_rename(pParse);
}
cmd ::= ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY
LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R)
defer_subclause_opt(D). {
- create_constraint_def_init(&pParse->create_fk_def.base, X, Z, D, false);
+ create_constraint_def_init(&pParse->create_fk_def.base, X, &Z, D, false);
create_fk_def_init(&pParse->create_fk_def, FA, &T, TA, R);
sql_create_foreign_key(pParse);
}
cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
- drop_entity_def_init(&pParse->drop_entity_def, X, Z, false, ENTITY_TYPE_FK);
+ drop_entity_def_init(&pParse->drop_entity_def, X, &Z, false, ENTITY_TYPE_FK);
sql_drop_foreign_key(pParse);
}
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index c3ed2f72d..c7640c523 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -84,6 +84,8 @@ struct Token {
bool isReserved;
};
+#define Token_nil ((struct Token) {NULL, 0, false})
+
/**
* Structure representing foreign keys constraints appeared
* within CREATE TABLE statement. Used only during parsing.
@@ -131,7 +133,11 @@ enum entity_type {
ENTITY_TYPE_TRIGGER,
ENTITY_TYPE_CK,
ENTITY_TYPE_FK,
- entity_type_MAX
+ /**
+ * For assertion checks that constraint definition is
+ * created before initialization of a term constraint.
+ */
+ ENTITY_TYPE_CONSTRAINT,
};
enum alter_action {
@@ -246,92 +252,91 @@ struct create_index_def {
/** Basic initialisers of parse structures.*/
static inline void
alter_entity_def_init(struct alter_entity_def *alter_def,
- struct SrcList *entity_name)
+ struct SrcList *entity_name, enum entity_type type,
+ enum alter_action action)
{
alter_def->entity_name = entity_name;
+ alter_def->entity_type = type;
+ alter_def->alter_action = action;
}
static inline void
rename_entity_def_init(struct rename_entity_def *rename_def,
- struct SrcList *table_name, struct Token new_name)
+ struct SrcList *table_name, struct Token *new_name)
{
- alter_entity_def_init(&rename_def->base, table_name);
- rename_def->new_name = new_name;
- struct alter_entity_def *alter_def =
- (struct alter_entity_def *) rename_def;
- alter_def->entity_type = ENTITY_TYPE_TABLE;
- alter_def->alter_action = ALTER_ACTION_RENAME;
+ alter_entity_def_init(&rename_def->base, table_name, ENTITY_TYPE_TABLE,
+ ALTER_ACTION_RENAME);
+ rename_def->new_name = *new_name;
}
static inline void
-create_entity_def_init(struct create_entity_def *create_def, struct Token name,
- bool if_not_exist)
+create_entity_def_init(struct create_entity_def *create_def,
+ enum entity_type type, struct SrcList *parent_name,
+ struct Token *name, bool if_not_exist)
{
- create_def->name = name;
+ alter_entity_def_init(&create_def->base, parent_name, type,
+ ALTER_ACTION_CREATE);
+ create_def->name = *name;
create_def->if_not_exist = if_not_exist;
}
static inline void
create_constraint_def_init(struct create_constraint_def *constr_def,
- struct SrcList *parent_name, struct Token name,
+ struct SrcList *parent_name, struct Token *name,
bool if_not_exists, bool is_deferred)
{
- alter_entity_def_init(&constr_def->base.base, parent_name);
- create_entity_def_init(&constr_def->base, name, if_not_exists);
+ create_entity_def_init(&constr_def->base, ENTITY_TYPE_CONSTRAINT,
+ parent_name, name, if_not_exists);
constr_def->is_deferred = is_deferred;
}
static inline void
drop_entity_def_init(struct drop_entity_def *drop_def,
- struct SrcList *parent_name, struct Token name,
+ struct SrcList *parent_name, struct Token *name,
bool if_exist, enum entity_type entity_type)
{
- alter_entity_def_init(&drop_def->base, parent_name);
- drop_def->name = name;
+ alter_entity_def_init(&drop_def->base, parent_name, entity_type,
+ ALTER_ACTION_DROP);
+ drop_def->name = *name;
drop_def->if_exist = if_exist;
- drop_def->base.entity_type = entity_type;
- drop_def->base.alter_action = ALTER_ACTION_DROP;
}
static inline void
create_trigger_def_init(struct create_trigger_def *trigger_def,
- struct SrcList *table_name, struct Token name,
+ struct SrcList *table_name, struct Token *name,
int tr_tm, int op, struct IdList *cols,
struct Expr *when, bool if_not_exists)
{
- alter_entity_def_init(&trigger_def->base.base, table_name);
- create_entity_def_init(&trigger_def->base, name, if_not_exists);
+ create_entity_def_init(&trigger_def->base, ENTITY_TYPE_TRIGGER,
+ table_name, name, if_not_exists);
trigger_def->tr_tm = tr_tm;
trigger_def->op = op;
trigger_def->cols = cols;
trigger_def->when = when;
- struct alter_entity_def *alter_def =
- (struct alter_entity_def *) trigger_def;
- alter_def->entity_type = ENTITY_TYPE_TRIGGER;
- alter_def->alter_action = ALTER_ACTION_CREATE;
}
static inline void
create_ck_def_init(struct create_ck_def *ck_def, struct ExprSpan *expr)
{
- ck_def->expr = expr;
- struct alter_entity_def *alter_def =
- (struct alter_entity_def *) ck_def;
+ 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;
- alter_def->alter_action = ALTER_ACTION_CREATE;
+ 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)
{
- index_def->cols = cols;
- index_def->idx_type = idx_type;
- index_def->sort_order = sort_order;
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;
- alter_def->alter_action = ALTER_ACTION_CREATE;
+ index_def->cols = cols;
+ index_def->idx_type = idx_type;
+ index_def->sort_order = sort_order;
}
static inline void
@@ -339,42 +344,35 @@ create_fk_def_init(struct create_fk_def *fk_def, struct ExprList *child_cols,
struct Token *parent_name, struct ExprList *parent_cols,
int actions)
{
- assert(fk_def != NULL);
+ 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;
fk_def->child_cols = child_cols;
fk_def->parent_name = parent_name;
fk_def->parent_cols = parent_cols;
fk_def->actions = actions;
- struct alter_entity_def *alter_def =
- (struct alter_entity_def *) fk_def;
- alter_def->entity_type = ENTITY_TYPE_FK;
- alter_def->alter_action = ALTER_ACTION_CREATE;
}
static inline void
-create_table_def_init(struct create_table_def *table_def, struct Token name,
+create_table_def_init(struct create_table_def *table_def, struct Token *name,
bool if_not_exists)
{
- create_entity_def_init(&table_def->base, name, if_not_exists);
+ create_entity_def_init(&table_def->base, ENTITY_TYPE_TABLE, NULL, name,
+ if_not_exists);
rlist_create(&table_def->new_fkey);
- struct alter_entity_def *alter_def =
- (struct alter_entity_def *) table_def;
- alter_def->entity_type = ENTITY_TYPE_TABLE;
- alter_def->alter_action = ALTER_ACTION_CREATE;
}
static inline void
-create_view_def_init(struct create_view_def *view_def, struct Token name,
+create_view_def_init(struct create_view_def *view_def, struct Token *name,
struct Token *create, struct ExprList *aliases,
struct Select *select, bool if_not_exists)
{
- create_entity_def_init(&view_def->base, name, if_not_exists);
+ create_entity_def_init(&view_def->base, ENTITY_TYPE_VIEW, NULL, name,
+ if_not_exists);
view_def->create_start = create;
view_def->select = select;
view_def->aliases = aliases;
- struct alter_entity_def *alter_def =
- (struct alter_entity_def *) view_def;
- alter_def->entity_type = ENTITY_TYPE_VIEW;
- alter_def->alter_action = ALTER_ACTION_CREATE;
}
static inline void
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 139c3c763..f8ec3519e 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2734,6 +2734,7 @@ 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;
More information about the Tarantool-patches
mailing list