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 990312565B for ; Thu, 17 Jan 2019 20:42:31 -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 f8ACOlSY6QMD for ; Thu, 17 Jan 2019 20:42:31 -0500 (EST) Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 BEEC6210AD for ; Thu, 17 Jan 2019 20:42:30 -0500 (EST) Date: Fri, 18 Jan 2019 04:42:27 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 2/6] sql: rework ALTER TABLE grammar Message-ID: <20190118014227.GS28204@chai> References: <0117c011c631182ddd64cff7a46e2b3e940bf03c.1547035183.git.korablev@tarantool.org> <20190117115100.GM28204@chai> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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" Cc: tarantool-patches@freelists.org, Vladislav Shpilevoy * n.pettik [19/01/17 20:20]: Looks good to me. > > > > On 17 Jan 2019, at 14:51, Konstantin Osipov wrote: > > > > * Nikita Pettik [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