[tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Feb 4 18:25:32 MSK 2019
Hi! Thanks for the fixes! See 16 comments below.
On 31/01/2019 20:32, n.pettik wrote:
> I’ve took into consideration your comments and pushed
> new version of this patch:
>
> Branch: np/gh-3914-fix-create-index-v2
>
> If it is OK (except for minor things mb) I will add it to
> the main patch-set and rebase the rest.
>
> Author: Nikita Pettik <korablev at tarantool.org>
> Date: Wed Jan 9 12:28:09 2019 +0200
>
> sql: introduce structs assembling DDL arguments during parsing
>
> Parser's rules implementing DDL have a lot in common. For instance,
> to drop any entity it is enough to know its name and name of table it is
> related to.
> Thus, it was suggested to arrange arguments of DDL rules into
> hierarchical structure. The root of chain always includes name of table
> to be altered: all existing entities are related to some table. Then
> comes one of drop/create/rename rules. Indeed, each DDL operation can be
> classified in these terms, at least until we introduce ALTER TABLE ALTER
> CONSTRAINT statement. Drop is represented by single structure (as it was
> mentioned); rename can be applied only to table; create can be applied
> to CONSTRAINT (indexes are considered as constraints) and TRIGGER, which
> in turn are different in arguments required to create those objects. And
> so forth.
>
> What is more, we are going to introduce ALTER TABLE ADD CONSTRAINT
> UNIQUE With new hierarchy we can extend ALTER TABLE statement with ease:
> basic structures (alter -> create entity -> create constraint) are the
> same for .. FOREIGN KEY/UNIQUE, but the last one will be different.
>
> Patch itself is made up of refactoring; no functional changes are
> provided.
>
> Needed for #3097
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index f92f39d8e..b6f099cf5 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -383,18 +383,16 @@ sql_table_new(Parse *parser, char *name)
> * when the "TEMP" or "TEMPORARY" keyword occurs in between
> * CREATE and TABLE.
> *
> - * The new table record is initialized and put in pParse->pNewTable.
> + * The new table record is initialized and put in pParse->new_table.
1. No member new_table in struct Parse.
> * As more of the CREATE TABLE statement is parsed, additional action
> * routines will be called to add more information to this record.
> * At the end of the CREATE TABLE statement, the sqlite3EndTable() routine
> * is called to complete the construction of the new table record.
> *
> * @param pParse Parser context.
> - * @param pName1 First part of the name of the table or view.
> - * @param noErr Do nothing if table already exists.
> */
> void
> -sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
> +sqlite3StartTable(struct Parse *pParse)
> {
> Table *pTable;
> char *zName = 0; /* The name of the new table */
> @@ -403,8 +401,9 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
> if (v == NULL)
> goto cleanup;
> sqlite3VdbeCountChanges(v);
> -
> - zName = sqlite3NameFromToken(db, pName);
> + struct Token name =
> + ((struct create_entity_def *) &pParse->create_table_def)->name;
> + zName = sqlite3NameFromToken(db, &name);
2. Looks shorter, no?
if (v == NULL)
goto cleanup;
sqlite3VdbeCountChanges(v);
- struct Token name =
- ((struct create_entity_def *) &pParse->create_table_def)->name;
+ struct Token name = pParse->create_table_def.base.name;
zName = sqlite3NameFromToken(db, &name);
if (zName == 0)
>
> if (zName == 0)
> return;
> @@ -1788,12 +1795,14 @@ columnno_by_name(struct Parse *parse_context, const struct space *space,
> }
>
> void
> -sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
> - struct Token *constraint, struct ExprList *child_cols,
> - struct Token *parent, struct ExprList *parent_cols,
> - bool is_deferred, int actions)
> +sql_create_foreign_key(struct Parse *parse_context)
> {
> struct sqlite3 *db = parse_context->db;
> + struct create_fk_def create_fk_def = parse_context->create_fk_def;
> + struct create_constraint_def create_constr_def = create_fk_def.base;
> + struct create_entity_def create_def = create_constr_def.base;
> + struct alter_entity_def alter_def = create_def.base;
3. Please, do not copy these structs onto the stack. They are too big.
The same in all other places.
> + assert(alter_def.entity_type == ENTITY_TYPE_FK);
> /*
> * When this function is called second time during
> * <CREATE TABLE ...> statement (i.e. at VDBE runtime),
> @@ -2211,19 +2227,29 @@ constraint_is_named(const char *name)
> }
>
> void
> -sql_create_index(struct Parse *parse, struct Token *token,
> - struct SrcList *tbl_name, struct ExprList *col_list,
> - MAYBE_UNUSED struct Token *start, enum sort_order sort_order,
> - bool if_not_exist, enum sql_index_type idx_type) {
> +sql_create_index(struct Parse *parse) {
> /* The index to be created. */
> struct index *index = NULL;
> /* Name of the index. */
> char *name = NULL;
> struct sqlite3 *db = parse->db;
> assert(!db->init.busy);
> + struct create_index_def create_idx_def = parse->create_index_def;
> + struct create_entity_def *create_entity_def =
> + (struct create_entity_def *) &create_idx_def;
4. Please, use 'base'. Code would be smaller and less erroneous than casts.
Or at least be consistent and use everywhere either cast or base.
> + struct alter_entity_def alter_entity_def = create_entity_def->base;
> + assert(alter_entity_def.entity_type == ENTITY_TYPE_INDEX);
> + /*
> + * Get list of columns to be indexed. It will be NULL if
> + * this is a primary key or unique-constraint on the most
> + * recent column added to the table under construction.
> + */
> + struct ExprList *col_list = create_idx_def.cols;
> + struct SrcList *tbl_name = alter_entity_def.entity_name;
>
> if (db->mallocFailed || parse->nErr > 0)
> goto exit_create_index;
> + enum sql_index_type idx_type = create_idx_def.idx_type;
> if (idx_type == SQL_INDEX_TYPE_UNIQUE ||
> idx_type == SQL_INDEX_TYPE_NON_UNIQUE) {
> Vdbe *v = sqlite3GetVdbe(parse);
> @@ -2299,11 +2324,10 @@ sql_create_index(struct Parse *parse, struct Token *token,
> }
> } else {
> char *constraint_name = NULL;
> - if (parse->constraintName.z != NULL)
> + if (create_entity_def->name.n > 0)
5. In struct Token comment you wrote/copy-pasted, that
if z == NULL, n is undefined. So here n > 0 does
not mean that z != NULL. Or the comment was wrong?
> constraint_name =
> sqlite3NameFromToken(db,
> - &parse->constraintName);
> -
> + &create_entity_def->name);
> /*
> * This naming is temporary. Now it's not
> * possible (since we implement UNIQUE
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 0bcf41594..e8b053361 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -168,7 +168,9 @@ cmd ::= ROLLBACK TO savepoint_opt nm(X). {
> //
> cmd ::= create_table create_table_args.
> create_table ::= createkw TABLE ifnotexists(E) nm(Y). {
> - sqlite3StartTable(pParse,&Y,E);
> + alter_entity_def_init(&pParse->create_table_def, NULL, ENTITY_TYPE_TABLE);
> + create_entity_def_init(&pParse->create_table_def, Y, E);
6. We never use 'init' suffix. Please, use 'create'.
> + 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.
By the way, 0 is already not default value of entity type. Memset above
makes create_index_def ha ENTITY_TYPE_TABLE type.
> +}
> +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);}
> @@ -260,14 +269,29 @@ ccons ::= NULL onconf(R). {
> sql_column_add_nullable_action(pParse, R);
> }
> ccons ::= NOT NULL onconf(R). {sql_column_add_nullable_action(pParse, R);}
> -ccons ::= PRIMARY KEY sortorder(Z) autoinc(I).
> - {sqlite3AddPrimaryKey(pParse,0,I,Z);}
> -ccons ::= UNIQUE. {sql_create_index(pParse,0,0,0,0,
> - SORT_ORDER_ASC, false,
> - SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);}
> -ccons ::= CHECK LP expr(X) RP. {sql_add_check_constraint(pParse,&X);}
> -ccons ::= REFERENCES nm(T) eidlist_opt(TA) refargs(R).
> - {sql_create_foreign_key(pParse, NULL, NULL, NULL, &T, TA, false, R);}
> +ccons ::= PRIMARY KEY sortorder(Z) autoinc(I). {
> + pParse->create_table_def.has_autoinc = I;
> + sqlite3AddPrimaryKey(pParse,0,Z);
> +}
> +ccons ::= UNIQUE. {
> + pParse->create_index_def.idx_type = SQL_INDEX_TYPE_CONSTRAINT_UNIQUE;
> + entity_set_type(&pParse->create_index_def, ENTITY_TYPE_INDEX);
8. Why UNIQUE sets entity type index, but PRIMARY KEY does not?
> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
> new file mode 100644
> index 000000000..7a61a27e0
> --- /dev/null
> +++ b/src/box/sql/parse_def.h
> @@ -0,0 +1,260 @@
> +#ifndef TARANTOOL_BOX_SQL_PARSE_DEF_H_INCLUDED
> +#define TARANTOOL_BOX_SQL_PARSE_DEF_H_INCLUDED
> +/*
> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + * copyright notice, this list of conditions and the
> + * following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following
> + * disclaimer in the documentation and/or other materials
> + * provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +#include "box/fkey.h"
> +
> +/**
> + * This file contains auxiliary structures and functions which
> + * are used only during parsing routine (see parse.y).
> + * Their main purpose is to assemble common parts of altered
> + * entities (such as name, or IF EXISTS clause) and pass them
> + * as a one object to further functions.
> + *
> + * Hierarchy is following:
> + *
> + * Base structure is ALTER.
> + * ALTER is omitted only for CREATE TABLE since table is filled
> + * with meta-information just-in-time of parsing:
> + * for instance, as soon as field's name and type are recognized
> + * they are added to space definition.
> + *
> + * DROP is general for all existing objects and includes
> + * name of object itself, name of parent object (table),
> + * IF EXISTS clause and may contain on-drop behaviour
> + * (CASCADE/RESTRICT, but now it is always RESTRICT).
> + * Hence, it terms of grammar - it is a terminal symbol.
> + *
> + * RENAME can be applied only to table (at least now, since it is
> + * ANSI extension), so it is also terminal symbol.
> + *
> + * CREATE in turn can be expanded to nonterminal symbol
> + * CREATE CONSTRAINT or to terminal CREATE TABLE/INDEX/TRIGGER.
> + * CREATE CONSTRAINT unfolds to FOREIGN KEY or UNIQUE/PRIMARY KEY.
> + *
> + * For instance:
> + * ALTER TABLE t ADD CONSTRAINT c FOREIGN KEY REFERENCES t2(id);
> + * ALTER *TABLE* -> CREATE ENTITY -> CREATE CONSTRAINT -> CREATE FK
> + *
> + * CREATE TRIGGER tr1 ...
> + * ALTER *TABLE* -> CREATE ENTITY -> CREATE TRIGGER
> + *
> + * All terminal symbols are stored as a union within
> + * parsing context (struct Parse).
> + */
> +
> +/**
> + * Each token coming out of the lexer is an instance of
> + * this structure. Tokens are also used as part of an expression.
> + *
> + * Note if Token.z is NULL then Token.n is undefined.
> + */
> +struct Token {
> + /** Text of the token. Not NULL-terminated! */
> + const char *z;
> + /** Number of characters in this token. */
> + unsigned int n;
> + bool isReserved;
> +};
> +
> +/**
> + * Possible SQL index types. Note that PK and UNIQUE constraints
> + * are implemented as indexes and have their own types:
> + * _CONSTRAINT_PK and _CONSTRAINT_UNIQUE.
> + */
> +enum sql_index_type {
> + SQL_INDEX_TYPE_NON_UNIQUE = 0,
> + SQL_INDEX_TYPE_UNIQUE,
> + SQL_INDEX_TYPE_CONSTRAINT_UNIQUE,
> + SQL_INDEX_TYPE_CONSTRAINT_PK,
> + sql_index_type_MAX
> +};
> +
> +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;
By the way, please, rename ENTITY_TYPE_CK to ENTITY_TYPE_CHECK.
Three letters 'HEC' economy is not worth the name obfuscation.
> +};
> +
> +struct alter_entity_def {
> + /** Type of topmost entity. */
> + enum entity_type entity_type;
> + /** As a rule it is a name of table to be altered. */
> + struct SrcList *entity_name;
> +};
> +
> +struct rename_entity_def {
> + struct alter_entity_def base;
> + struct Token new_name;
> +};
> +
> +struct create_entity_def {
> + struct alter_entity_def base;
> + struct Token name;
> + /** Statement comes with IF NOT EXISTS clause. */
> + bool if_not_exist;
> +};
> +
> +struct create_table_def {
> + struct create_entity_def base;
> + struct Table *new_table;
> + /**
> + * Number of FK constraints declared within
> + * CREATE TABLE statement.
> + */
> + uint32_t fkey_count;
> + /**
> + * Foreign key constraint appeared in CREATE TABLE stmt.
> + */
> + struct rlist new_fkey;
> + /** True, if table to be created has AUTOINCREMENT PK. */
> + bool has_autoinc;
> +};
> +
> +struct drop_entity_def {
> + struct alter_entity_def base;
> + /** Name of index/trigger/constraint to be dropped. */
> + struct Token name;
> + /** Statement comes with IF EXISTS clause. */
> + bool if_exist;
> +};
> +
> +struct create_trigger_def {
> + struct create_entity_def base;
> + /** One of TK_BEFORE, TK_AFTER, TK_INSTEAD. */
> + int tr_tm;
> + /** One of TK_INSERT, TK_UPDATE, TK_DELETE. */
> + int op;
> + /** Column list if this is an UPDATE trigger. */
> + struct IdList *cols;
> + /** When clause. */
> + struct Expr *when;
> +};
> +
> +struct create_constraint_def {
> + struct create_entity_def base;
> + /** One of DEFERRED, IMMEDIATE. */
> + bool is_deferred;
> +};
> +
> +struct create_ck_def {
> + struct create_constraint_def base;
> + /** AST representing check expression. */
> + struct ExprSpan *expr;
> +};
> +
> +struct create_fk_def {
> + struct create_constraint_def base;
> + struct ExprList *child_cols;
> + struct Token *parent_name;
> + struct ExprList *parent_cols;
> + /**
> + * Encoded actions for MATCH, ON DELETE and
> + * ON UPDATE clauses.
> + */
> + int actions;
> +};
> +
> +struct create_index_def {
> + struct create_constraint_def base;
> + /** List of indexed columns. */
> + struct ExprList *cols;
> + /** One of _PRIMARY_KEY, _UNIQUE, _NON_UNIQUE. */
> + enum sql_index_type idx_type;
> + enum sort_order sort_order;
> +};
> +
> +/**
> + * In those functions which accept void * instead of certain type
> + * it was made on purpose: it allows to avoid explicit cast before
> + * passing parameter to function. Hence, we can invoke it like:
> + * entity_set_type(create_fk_def, ...); instead of
> + * entity_set_type((struct alter_entity_def *) create_fk_def, ...);
> + */
> +static inline void
> +entity_set_type(void *entity_def, enum entity_type type)
> +{
> + struct alter_entity_def *alter_def =
> + (struct alter_entity_def *) entity_def;
> + alter_def->entity_type = type;
> +}
10. Please, do not use void * in either of these functions. Use
normal types. Also, entity_set_type() should not exist as a separate
function. Instead, each terminal structure above should have it own
create() function, which internally sets type.
> +
> +static inline void
> +entity_set_defer_mode(void *entity_def, bool is_deferred)
> +{
> + struct create_constraint_def *constr_def =
> + (struct create_constraint_def *) entity_def;
> + constr_def->is_deferred = is_deferred;
> +}
11. Same. You unsafely cast void * to create_constraint_def *.
Also, you have a special setter for is_deferred, but have no
for if_exist. And nonetheless you set if_exist manually in at
least one place. Why?
> +
> +static inline void
> +alter_entity_def_init(void *entity_def, struct SrcList *entity_name,
> + enum entity_type type)
12. This function should not be called by user. It should be part of
terminal def create() functions. It looks really weird when you do
something like this:
alter_entity_def_init(&pParse->create_fk_def)
create_entity_def_init(&pParse->create_fk_def)
entity_set_defer_mode(&pParse->create_fk_def)
So basically you called three!!! constructors. It is not scalable,
sorry.
> +{
> + struct alter_entity_def *alter_def =
> + (struct alter_entity_def *) entity_def;
> + alter_def->entity_name = entity_name;
> + alter_def->entity_type = type;
> +}
> +
> +static inline void
> +create_entity_def_init(void *entity_def, struct Token name, bool if_not_exist)
13. You if_not_exist passed through the constructor of create_entity, but
you do not do the same for is_exist and drop_entity. Why? I see the function
below, but parse.y:381 directly changes if_exist.
> +{
> + struct create_entity_def *create_def =
> + (struct create_entity_def *) entity_def;
> + create_def->name = name;
> + create_def->if_not_exist = if_not_exist;
> +}
> +
> +static inline void
> +drop_entity_def_init(struct drop_entity_def *drop_def, struct Token name,
> + bool if_exist)
> +{
> + drop_def->name = name;
> + drop_def->if_exist = if_exist;
> +}
> +
> +static inline void
> +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);
> + fk_def->child_cols = child_cols;
> + fk_def->parent_name = parent_name;
> + fk_def->parent_cols = parent_cols;
> + fk_def->actions = actions;
> +}
> +
> +#endif /* TARANTOOL_BOX_SQL_PARSE_DEF_H_INCLUDED */
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index 824578e45..3343c2942 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -273,7 +273,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
> {
> memset(parser, 0, sizeof(struct Parse));
> parser->db = db;
> - rlist_create(&parser->new_fkey);
> + rlist_create(&parser->create_table_def.new_fkey);
14. Please, introduce create_table_def_create() in parse_def.h and do
all necessary actions there.
> rlist_create(&parser->record_list);
> region_create(&parser->region, &cord()->slabc);
> }
> @@ -287,7 +287,7 @@ sql_parser_destroy(Parse *parser)
> sqlite3DbFree(db, parser->aLabel);
> sql_expr_list_delete(db, parser->pConstExpr);
> struct fkey_parse *fk;
> - rlist_foreach_entry(fk, &parser->new_fkey, link)
> + rlist_foreach_entry(fk, &parser->create_table_def.new_fkey, link)
> sql_expr_list_delete(db, fk->selfref_cols);
15. create_table_def_destroy().
> if (db != NULL) {
> assert(db->lookaside.bDisable >=
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 6462467bc..e66f0d6db 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -1616,9 +1616,9 @@ void
> sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
> struct Expr *expr, struct ExprList *expr_list)
> {
> - /* Fake SrcList for parser->pNewTable */
> + /* Fake SrcList for parser->new_table */
> SrcList sSrc;
> - /* Name context for parser->pNewTable */
> + /* Name context for parser->new_table */
16. No such member in struct Parse.
> NameContext sNC;
>
> assert(type == NC_IsCheck || type == NC_IdxExpr);
More information about the Tarantool-patches
mailing list