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 A337928563 for ; Fri, 15 Feb 2019 15:13:36 -0500 (EST) 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 AWDnjYX8dDSZ for ; Fri, 15 Feb 2019 15:13:36 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 C932B28445 for ; Fri, 15 Feb 2019 15:13:35 -0500 (EST) 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> From: Vladislav Shpilevoy Message-ID: Date: Fri, 15 Feb 2019 23:13:33 +0300 MIME-Version: 1.0 In-Reply-To: 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: "n.pettik" , tarantool-patches@freelists.org Hi! Thanks for the fixes! > suggest ‘_prepare’, ‘_assebmle’, ‘_fill' > If you are sure about ‘_create' naming, I will fix it. > >>> + sqlite3StartTable(pParse); >>> } >>> createkw(A) ::= CREATE(A). {disableLookaside(pParse);} >>> @@ -237,8 +239,15 @@ nm(A) ::= id(A). { >>> carglist ::= carglist cconsdef. >>> carglist ::= . >>> cconsdef ::= cconsname ccons. >>> -cconsname ::= CONSTRAINT nm(X). {pParse->constraintName = X;} >>> -cconsname ::= . {pParse->constraintName.n = 0;} >>> +cconsname ::= cconsname_start cconsname_parse . >>> +cconsname_start ::= . { >>> + /* Prepare base members for re-usage. */ >>> + memset(&pParse->create_index_def, 0, sizeof(struct create_index_def)); >>> +} >>> +cconsname_parse ::= CONSTRAINT nm(X). { >>> + create_entity_def_init(&pParse->create_index_def, X, false); >> >> 7. Why in case of any constraint start, you reset only index_def? UNIQUE/PK >> are not the only existing constraints. >> Also, memset(0) is not scalable >> solution - you need a separate reset() function, or something like that in >> case if in future parse_def.h structures will grow in size and complexity, >> and 0 becomes not default value of some attribute. > > Well, imho it is too much. I did not understand. What is too much? > Actually, here we really care only about > zeroing several pointers and some size params and don’t care > about the rest. Yes, but here you assume, that index_def always lies in the same memory as all other constraint objects, and we will never store more pointers in the rest memory > sizeof(index_def). > This is first rule to be called when some constraint > is created. What is more, now terminal initialisers fill all fields, except > for base structs. Hence, now it is enough to reset memory layout > up to struct create_constraint_def: Your code below assumes the thing I said above, so please, add a struct create_constraint_def to union in struct Parse and use it here. Like this: diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 22b5669d8..83c0fc465 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -244,7 +244,7 @@ cconsdef ::= cconsname ccons. cconsname ::= cconsname_start cconsname_parse . cconsname_start ::= . { /* Prepare base members for re-usage. */ - memset(&pParse->create_index_def, 0, sizeof(struct create_constraint_def)); + memset(&pParse->constraint_def, 0, sizeof(pParse->constraint_def)); } cconsname_parse ::= CONSTRAINT nm(X). { create_entity_def_init(&pParse->create_index_def.base.base, X, false); diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 4e6806be8..8566fe404 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2731,6 +2731,7 @@ struct Parse { * from parse.y */ union { + struct create_constraint_def constraint_def; struct create_ck_def create_ck_def; struct create_fk_def create_fk_def; struct create_index_def create_index_def; >>> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h >>> +enum entity_type { >>> + ENTITY_TYPE_TABLE = 0, >>> + ENTITY_TYPE_INDEX, >>> + ENTITY_TYPE_TRIGGER, >>> + ENTITY_TYPE_CK, >>> + ENTITY_TYPE_FK, >>> + entity_type_MAX >> >> 9. These entity_types are useless. Even being used in asserts, >> they do not prevent errors, allowing to create an instance of >> create_entity_def, but use it as drop_entity_def, because >> base.entity_type in both cases can be, for example, >> ENTITY_TYPE_INDEX. >> >> Please, use not object entity types, but def types: >> >> ALTER_DEF_CREATE_TABLE/DROP_TABLE/RENAME.../...INDEX.../... >> >> Or split entity type and action, so as to have something like >> this: >> >> def.entity = ALTER_DEF_TABLE; >> def.action = ALTER_DEF_RENAME; > > I’d better choose this way. Sorry, I can’t spot diff > of particularly this fix, since it has been entangled with diffs > to other comments. But I will attach whole diff at the end of letter. > >> By the way, please, rename ENTITY_TYPE_CK to ENTITY_TYPE_CHECK. >> Three letters 'HEC' economy is not worth the name obfuscation. > > It is common abbreviation (the same as FK), and AFAIR was even > suggested by Konstantin in one of reviews (mb not to this patch). > It simply makes name be similar to ENTITY_TYPE_FK. I see CK here first time. We've never used this abbreviation as I remember. We use FK not because it is a policy, but because it is just too long without contraction. Otherwise we would have ENTITY_TYPE_IN instead of _INDEX, ENTITY_TYPE_TA instead of _TABLE, ENTITY_TYPE_TR instead of _TRIGGER. > @@ -264,6 +283,7 @@ create_fk_def_init(struct create_fk_def *fk_def, struct ExprList *child_cols, > fk_def->parent_name = parent_name; > fk_def->parent_cols = parent_cols; > fk_def->actions = actions; > + ((struct alter_entity_def *) fk_def)->entity_type = ENTITY_TYPE_FK; > } > > I know you asked me to choose between base struct and cast. > However, I think here cast is much more suitable: these structures > are terminated so we have to write base.base.base. > On the other hand, almost in all places one or two nesting > base usage looks better. I can revert this change with ease, > if you want to. base.base.base is bad, but just one base is good, where possible. 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. 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(). Now about create_constraint_def_init(). In your patch it is always followed by create_fk_def_init(). 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? Thirdly, about alter_entity_def_init. Even now it is *always* followed by create_*_init(), or drop_*_init(), or rename_*_init(). So I advise you to make this function internal for parse_def.h and put its calls into create/drop/rename_*_init(). The whole idea, as I understood it, was not to just split into a random steps, but so as to build an automaton: +- drop_check | +- drop_fk | +- drop_table | drop_entity ---+- drop_index / -- rename_entity --- rename_table \ create_entity -+- create_index | +- create_table | +- create_fk | +- create_check Which in future should be easily expanded to this: +- drop_check | +- drop_fk | +- drop_table | +- drop_entity --+- drop_index | | +- index_rename | | | +- table_add_column | | --+- alter_entity -+- table_rename | | | +- table_drop_column | | | +- fk_rename | +- create_entity +- create_index | +- create_table | +- create_fk | +- create_check So any rule can by finished in two calls. First defines an action, second defines an entity and action-entity details. Now some of your rules are finished in 2, some in 3, some in 4 steps, and at the end of some of them you still are not in a terminal position. Although I am almost sure, that all of my proposals above are wrong in K.O. opinion, so please, consult him. If he wants something else, then I do not understand what, and I will respond LGTM.