[tarantool-patches] Re: [PATCH 2/6] sql: rework ALTER TABLE grammar

[Konstantin Osipov <kostja@tarantool.org>]

* n.pettik <korablev@tarantool.org> [19/01/17 20:20]:

Looks good to me.

> 
> 
> > On 17 Jan 2019, at 14:51, Konstantin Osipov <kostja@tarantool.org> wrote:
> > 
> > * Nikita Pettik <korablev@tarantool.org> [19/01/09 15:17]:
> >> +alter_table_start ::= ALTER TABLE fullname(Z) . {
> >> +  pParse->constraint->table_name = Z;
> >> +  pParse->constraint->name.n = 0;
> >> +}
> > 
> > It's bikeshed at this point, but in future you will have other
> > ALTER TABLE clauses: add/drop column, add/drop index,
> 
> Indexes out of scope: they are part of constraint routines.
> 
> > add/drop
> > constraint. It's unclear why you initialize a specific parsing
> > context (constraint definition) right in alter_table_start rule.
> > OK for now.
> 
> We must save name of table before any other processing.
> So, at the moment of parsing table’s name we don’t have
> any notion about next actions yet.
> 
> We can use the way you suggested in previous letter:
> introduce some hierarchical structures.
> For instance ALTER can be implemented in the next way:
> 
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index e38e0da08..f1a5e1974 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2707,6 +2707,105 @@ struct constraint_parse {
>         struct ExprList *cols;
>  };
>  
> +enum entity_type {
> +       ENTITY_TYPE_TABLE = 0,
> +       ENTITY_TYPE_INDEX,
> +       ENTITY_TYPE_TRIGGER,
> +       ENTITY_TYPE_CONSTRAINT,
> +       ENTITY_TYPE_COLUMN
> +};
> +
> +/***
> + * Hierarchy is following:
> + *
> + * Base structure is ALTER.
> + * ALTER is omitted only for CREATE TABLE.
> + *
> + * 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.
> + * Hence, it terms of grammar - it is terminal symbol.
> + *
> + * RENAME can be applied only to table, 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
> + */
> +struct alter_entity_def {
> +       enum entity_type entity_type;
> +       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;
> +       /** Statement comes with IF NOT EXISTS clause. */
> +       bool if_not_exist;
> +       struct Token *name;
> +};
> +
> +struct drop_entity_def {
> +       struct alter_entity_def *base;
> +       /** Statement comes with IF EXISTS clause. */
> +       bool if_exist;
> +       struct Token entity_name;
> +       /** One of CASCADE, RESTRICT */
> +       int drop_behaviour;
> +};
> +
> +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 OF trigger. */
> +       struct IdList *columns;
> +       /** When clause. */
> +       struct Expr *when;
> +       bool if_not_exist;
> +
> +};
> +struct create_table_def {
> +       struct create_entity_def *base;
> +       struct space_def *table_def;
> +       struct Select *select;
> +};
> +
> +struct create_index_def {
> +       struct create_entity_def *base;
> +       enum sort_order sort_order;
> +       enum sql_index_type idx_type;
> +       struct ExprList *col_list;
> +};
> +
> +struct create_constraint_def {
> +       struct create_entity_def *base;
> +       /** One of DEFERRED, IMMEDIATE. */
> +       int check_time;
> +};
> +
> +struct create_fk_def {
> +       struct create_constraint_def *base;
> +       struct ExprList *child_cols;
> +       struct ExprList *parent_cols;
> +       struct Token *parent_name;
> +       enum fkey_action action;
> +};
> +
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index de5429498..84597bb91 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1452,8 +1452,11 @@ cmd ::= alter_table_start alter_table_action .
>   * the separate rule.
>   */
>  alter_table_start ::= ALTER TABLE fullname(Z) . {
> -  pParse->constraint.table = Z;
> -  pParse->constraint.name.n = 0;
> +  pParse->alter_entity_def =
> +    region_alloc(pParse->region, sizeof(struct alter_entity_def));
> +  pParse->alter_entity_def->entity_type = ENTITY_TYPE_TABLE;
> +  pParse->alter_entity_def->parent_name = Z;
> +  pParse->alter_entity_def->entity_name.n = 0;
>  }
>  
>  alter_table_action ::= add_constraint_def.
> @@ -1461,25 +1464,44 @@ alter_table_action ::= drop_constraint_def.
>  alter_table_action ::= rename.
>  
>  rename ::= RENAME TO nm(Z). {
> -  sql_alter_table_rename(pParse, &Z);
> +  struct alter_entity_rename *entity_rename =
> +    region_alloc(pParse->region, sizeof(struct rename_entity_def));
> +  entity_rename->base = pParse->alter_entity_def;
> +  entity_rename->new_name = Z;
> +  pParse->alter_entity_def = (struct alter_entity_def *) entity_rename;
> +  sql_alter_table_rename(pParse);
>  }
>  
>  add_constraint_def ::= add_constraint_start constraint_def.
>  
>  add_constraint_start ::= ADD CONSTRAINT nm(Z). {
> -  pParse->constraint.name = Z;
> +  struct create_entity_def *entity_create =
> +    region_alloc(pParse->region, sizeof(struct create_entity_def));
> +  entity_create->base = pParse->alter_entity_def;
> +  entity_create->name = Z;
> +  entity_create->if_not_exist = false;
> +  pParse->alter_entity_def = (struct alter_entity_def *)entity_create;
>  }
>  
>  constraint_def ::= foreign_key_def.
>  
>  foreign_key_def ::= FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T)
>                      eidlist_opt(TA) refargs(R) defer_subclause_opt(D). {
> -  pParse->constraint.cols = FA;
> -  sql_create_foreign_key(pParse, &T, TA, D, R);
> +  struct create_fk_def *fk_def =
> +    region_alloc(pParse->region, sizeof(struct create_fk_def));
> +  fk_def->child_cols = FA;
> +  fk_def->parent_cols = TA;
> +  fk_def->action = R;
> +  fk_def->parent_name = T;
> +  sql_create_foreign_key(pParse);
>  }
>  
>  drop_constraint_def ::= DROP CONSTRAINT nm(Z). {
> -  sql_drop_foreign_key(pParse, &Z);
> +    struct drop_entity_rename *entity_drop =
> +      region_alloc(pParse->region, sizeof(struct drop_entity_def));
> +    entity_drop->base = pParse->alter_entity_def;
> +    entity_drop->new_name = Z;
> +  sql_drop_foreign_key(pParse);
>  }
> 
> It is easy to fix other create/drop statements to use
> these structures. If it is OK to you and Vlad, I can
> rework whole patch.
> 
> >> +
> >> +alter_table_action ::= add_constraint_def.
> >> +alter_table_action ::= drop_constraint_def.
> > 
> > Why use the same data structure for create and drop?
> 
> For the sake of simplicity.
> 
> > When I drop a
> > constraint I specify its name and (perhaps) table name, none of
> > other properties.
> 
> Not quite: drop constraint clause also may include drop behaviour:
> one of CASCADE or RESTRICT.
> In our implementation it is always RESTRICT.
> 
> > To capture drop context one could use:
> > 
> > struct drop_object_def {
> >    enum object_type object_type;
> >    const char *object_name;
> >    const char *parent_object_name;
> > };
> > 
> > rename of an object could be captured in a similar data structure.
> 
> Rename and drop are the easiest cases.
> 
> 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov