[tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Mar 25 14:25:04 MSK 2019
Hi! Thanks for the fixes!
On 14/03/2019 21:13, n.pettik wrote:
>
>
>> On 12 Mar 2019, at 15:50, Vladislav Shpilevoy <v.shpilevoy at 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).
For me cons are not obvious. Pros - more proper code structure.
>> 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 :)
It is not a threat. It is a fact. At this moment you are a single
not-trainee developer on SQL. And I do not see any forthcoming
changes in that.
>
>>>> 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…
The very first approach was not only about this attribute. Also
it was about struct space *new_space, uint32_t fk_constraint_count,
struct rlist new_fk_constraint, bool is_new_table_autoinc.
>> 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[]:
Ok, understand. Thanks.
See 4 comments below.
>
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 996f55d37..aa16cb874 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -178,15 +179,16 @@ ifnotexists(A) ::= IF NOT EXISTS. {A = 1;}
>
> create_table_args ::= LP columnlist RP(E). {
> sqlEndTable(pParse,&E,0);
> + create_table_def_destroy(&pParse->create_table_def);
> }
> create_table_args ::= AS select(S). {
> sqlEndTable(pParse,0,S);
1. Why do not you destroy table def here?
> 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
2. As I understand, now table def leaks if parser stops after sqlStartTable
but before sqlEndTable.
3. sql_create_index comment still refers to parse->new_space.
4. Please, write a comment above struct create_table_def create_table_def in
struct Parse why it is not in the union.
More information about the Tarantool-patches
mailing list