From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 413D8246BC for ; Tue, 12 Mar 2019 08:50:50 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id IE91l0wubjeF for ; Tue, 12 Mar 2019 08:50:50 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 5766324581 for ; Tue, 12 Mar 2019 08:50:49 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing References: <0fcc585532a1f1200a7dfd4a8e911ecf9f2c94aa.1548265148.git.korablev@tarantool.org> <45655E85-4DBB-4AC4-8161-5B783C66C688@tarantool.org> <6e4d0aae-5dbe-f8d6-3bfe-7723ea2f3d9d@tarantool.org> <78138FFB-162A-4703-9A8F-CB88FD0570EE@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4841a3f7-5b21-10ea-4f58-160a5639b7d9@tarantool.org> Date: Tue, 12 Mar 2019 15:50:46 +0300 MIME-Version: 1.0 In-Reply-To: <78138FFB-162A-4703-9A8F-CB88FD0570EE@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, "n.pettik" 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__def { struct drop_entity_def base; } static inline void drop__def_init(struct drop__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_); } 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 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;