[tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Feb 15 23:13:33 MSK 2019
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.
More information about the Tarantool-patches
mailing list