* [tarantool-patches] [PATCH 0/6] Introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PK
@ 2019-01-09 12:13 Nikita Pettik
2019-01-09 12:13 ` [tarantool-patches] [PATCH 1/6] sql: move constraint name to struct contraint_parse Nikita Pettik
` (5 more replies)
0 siblings, 6 replies; 24+ messages in thread
From: Nikita Pettik @ 2019-01-09 12:13 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Branch: https://github.com/tarantool/tarantool/tree/np/gh-3914-fix-create-index
Issues:
https://github.com/tarantool/tarantool/issues/3097
https://github.com/tarantool/tarantool/issues/3914
It turns out that if space is created from Lua and has no indexes,
simply CREATE INDEX statement executed from SQL will raise strange
error "Wrong space id". It appears due to fetch of new index id from
_index space. However, in case it containts no entries with given space
id it doesn't really mean that there is no space with given id. It only
implies that space with given id lacks any indexes. This issue is
resovled by using correct error message. However, we still need way to
primary key on already existing table. Hence, lets introduce ALTER TABLE
ADD CONSTRAINT PRIMARY KEY, which would create index with id == 0.
Note that CREATE INDEX is not suitable for creating PK owing to the
following scenario:
Create table with no indexes
Create index (it implicitly becomes PK since it is the first index)
Create constraint PK (fails since index with id == 0 already exists)
Indexes are out of ANSI specification, so example above must not fail
(all vendors I've checked don't fail).
Nikita Pettik (6):
sql: move constraint name to struct contraint_parse
sql: rework ALTER TABLE grammar
sql: remove start token from sql_create_index args
sql: refactor getNewIid() function
sql: fix error message for improperly created index
sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY
src/box/sql/alter.c | 4 +-
src/box/sql/build.c | 141 +++++++++++++++++++++++++------------------
src/box/sql/parse.y | 76 ++++++++++++++++-------
src/box/sql/prepare.c | 11 ++++
src/box/sql/sqliteInt.h | 37 ++++++------
test/sql-tap/alter.test.lua | 58 +++++++++++++++++-
test/sql-tap/index1.test.lua | 28 ++++++++-
7 files changed, 254 insertions(+), 101 deletions(-)
--
2.15.1
^ permalink raw reply [flat|nested] 24+ messages in thread* [tarantool-patches] [PATCH 1/6] sql: move constraint name to struct contraint_parse 2019-01-09 12:13 [tarantool-patches] [PATCH 0/6] Introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PK Nikita Pettik @ 2019-01-09 12:13 ` Nikita Pettik 2019-01-14 14:04 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-09 12:13 ` [tarantool-patches] [PATCH 2/6] sql: rework ALTER TABLE grammar Nikita Pettik ` (4 subsequent siblings) 5 siblings, 1 reply; 24+ messages in thread From: Nikita Pettik @ 2019-01-09 12:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Before this patch name of constraint being parsed was held as a separate struct Token within parser context. Meanwhile, we also need to store name of table which constraint is related to (in order to implement ALTER TABLE ADD CONSTRAINT with several options: foreign key, unique, primary key, etc). Hence, lets move constraint name to a separate structure and save it alongside with the name of table. Needed for #3097 --- src/box/sql/alter.c | 4 ++-- src/box/sql/build.c | 36 +++++++++++++++++++----------------- src/box/sql/parse.y | 22 +++++++++++++--------- src/box/sql/prepare.c | 11 +++++++++++ src/box/sql/sqliteInt.h | 32 ++++++++++++++++++-------------- 5 files changed, 63 insertions(+), 42 deletions(-) diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c index d0ce9d893..dfd71792b 100644 --- a/src/box/sql/alter.c +++ b/src/box/sql/alter.c @@ -38,9 +38,9 @@ #include "box/schema.h" void -sql_alter_table_rename(struct Parse *parse, struct SrcList *src_tab, - struct Token *new_name_tk) +sql_alter_table_rename(struct Parse *parse, struct Token *new_name_tk) { + struct SrcList *src_tab = parse->constraint->table_name; assert(src_tab->nSrc == 1); struct sqlite3 *db = parse->db; char *new_name = sqlite3NameFromToken(db, new_name_tk); diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 49b90b5d0..963b16626 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -566,7 +566,7 @@ sqlite3AddColumn(Parse * pParse, Token * pName, struct type_def *type_def) column_def->affinity = type_def->type; column_def->type = sql_affinity_to_field_type(column_def->affinity); p->def->field_count++; - pParse->constraintName.n = 0; + pParse->constraint->name.n = 0; } void @@ -774,9 +774,9 @@ sql_add_check_constraint(struct Parse *parser, struct ExprSpan *span) sqlite3DbFree(parser->db, expr->u.zToken); goto release_expr; } - if (parser->constraintName.n) { + if (parser->constraint->name.n) { sqlite3ExprListSetName(parser, table->def->opts.checks, - &parser->constraintName, 1); + &parser->constraint->name, 1); } } else { release_expr: @@ -1800,8 +1800,7 @@ 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, +sql_create_foreign_key(struct Parse *parse_context, struct ExprList *child_cols, struct Token *parent, struct ExprList *parent_cols, bool is_deferred, int actions) { @@ -1834,10 +1833,10 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, } else { child_cols_count = child_cols->nExpr; } - assert(!is_alter || (child != NULL && child->nSrc == 1)); struct space *child_space = NULL; if (is_alter) { - const char *child_name = child->a[0].zName; + const char *child_name = + parse_context->constraint->table_name->a[0].zName; child_space = space_by_name(child_name); if (child_space == NULL) { diag_set(ClientError, ER_NO_SUCH_SPACE, child_name); @@ -1884,18 +1883,21 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, goto exit_create_fk; } } - if (constraint == NULL && !is_alter) { - if (parse_context->constraintName.n == 0) { + if (!is_alter) { + if (parse_context->constraint->name.n == 0) { constraint_name = sqlite3MPrintf(db, "FK_CONSTRAINT_%d_%s", ++parse_context->fkey_count, new_tab->def->name); } else { - struct Token *cnstr_nm = &parse_context->constraintName; + struct Token *cnstr_nm = + &parse_context->constraint->name; constraint_name = sqlite3NameFromToken(db, cnstr_nm); } } else { - constraint_name = sqlite3NameFromToken(db, constraint); + constraint_name = + sqlite3NameFromToken(db, + &parse_context->constraint->name); } if (constraint_name == NULL) goto exit_create_fk; @@ -2016,11 +2018,11 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred) } void -sql_drop_foreign_key(struct Parse *parse_context, struct SrcList *table, - struct Token *constraint) +sql_drop_foreign_key(struct Parse *parse_context, struct Token *constraint) { - assert(table != NULL && table->nSrc == 1); - const char *table_name = table->a[0].zName; + const char *table_name = + parse_context->constraint->table_name->a[0].zName; + assert(table_name != NULL); struct space *child = space_by_name(table_name); if (child == NULL) { diag_set(ClientError, ER_NO_SUCH_SPACE, table_name); @@ -2311,10 +2313,10 @@ sql_create_index(struct Parse *parse, struct Token *token, } } else { char *constraint_name = NULL; - if (parse->constraintName.z != NULL) + if (parse->constraint->name.z != NULL) constraint_name = sqlite3NameFromToken(db, - &parse->constraintName); + &parse->constraint->name); /* * This naming is temporary. Now it's not diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 50bb2ba01..f4fdf58f2 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -237,8 +237,8 @@ nm(A) ::= id(A). { carglist ::= carglist cconsdef. carglist ::= . cconsdef ::= cconsname ccons. -cconsname ::= CONSTRAINT nm(X). {pParse->constraintName = X;} -cconsname ::= . {pParse->constraintName.n = 0;} +cconsname ::= CONSTRAINT nm(X). {pParse->constraint->name = X;} +cconsname ::= . {pParse->constraint->name.n = 0;} ccons ::= DEFAULT term(X). {sqlite3AddDefaultValue(pParse,&X);} ccons ::= DEFAULT LP expr(X) RP. {sqlite3AddDefaultValue(pParse,&X);} ccons ::= DEFAULT PLUS term(X). {sqlite3AddDefaultValue(pParse,&X);} @@ -267,7 +267,7 @@ ccons ::= UNIQUE. {sql_create_index(pParse,0,0,0,0, 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);} + {sql_create_foreign_key(pParse, NULL, &T, TA, false, R);} ccons ::= defer_subclause(D). {fkey_change_defer_mode(pParse, D);} ccons ::= COLLATE id(C). {sqlite3AddCollateType(pParse, &C);} @@ -308,8 +308,8 @@ init_deferred_pred_opt(A) ::= INITIALLY DEFERRED. {A = 1;} init_deferred_pred_opt(A) ::= INITIALLY IMMEDIATE. {A = 0;} tconsdef ::= tconsname tcons. -tconsname ::= CONSTRAINT nm(X). {pParse->constraintName = X;} -tconsname ::= . {pParse->constraintName.n = 0;} +tconsname ::= CONSTRAINT nm(X). {pParse->constraint->name = X;} +tconsname ::= . {pParse->constraint->name.n = 0;} tcons ::= PRIMARY KEY LP sortlist(X) autoinc(I) RP. {sqlite3AddPrimaryKey(pParse,X,I,0);} tcons ::= UNIQUE LP sortlist(X) RP. @@ -320,7 +320,7 @@ tcons ::= CHECK LP expr(E) RP onconf. {sql_add_check_constraint(pParse,&E);} tcons ::= FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { - sql_create_foreign_key(pParse, NULL, NULL, FA, &T, TA, D, R); + sql_create_foreign_key(pParse, FA, &T, TA, D, R); } %type defer_subclause_opt {int} defer_subclause_opt(A) ::= . {A = 0;} @@ -1445,17 +1445,21 @@ cmd ::= ANALYZE nm(X). {sqlite3Analyze(pParse, &X);} //////////////////////// ALTER TABLE table ... //////////////////////////////// cmd ::= ALTER TABLE fullname(X) RENAME TO nm(Z). { - sql_alter_table_rename(pParse,X,&Z); + pParse->constraint->table_name = X; + sql_alter_table_rename(pParse, &Z); } cmd ::= ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { - sql_create_foreign_key(pParse, X, &Z, FA, &T, TA, D, R); + pParse->constraint->table_name = X; + pParse->constraint->name = Z; + sql_create_foreign_key(pParse, FA, &T, TA, D, R); } cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). { - sql_drop_foreign_key(pParse, X, &Z); + pParse->constraint->table_name = X; + sql_drop_foreign_key(pParse, &Z); } //////////////////////// COMMON TABLE EXPRESSIONS //////////////////////////// diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index 824578e45..0a2f59954 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -58,6 +58,17 @@ sqlite3Prepare(sqlite3 * db, /* Database handle. */ Parse sParse; /* Parsing context */ sql_parser_create(&sParse, db); sParse.pReprepare = pReprepare; + sParse.constraint = region_alloc(&sParse.region, + sizeof(*sParse.constraint)); + if (sParse.constraint == NULL) { + diag_set(OutOfMemory, sizeof(*sParse.constraint), "region", + "constraint"); + sql_parser_destroy(&sParse); + rc = SQLITE_NOMEM; + return rc; + } + sParse.constraint->name.z = NULL; + sParse.constraint->name.n = 0; assert(ppStmt && *ppStmt == 0); /* assert( !db->mallocFailed ); // not true with SQLITE_USE_ALLOCA */ diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 4110a5991..51a5d01b5 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2691,6 +2691,17 @@ struct fkey_parse { struct rlist link; }; +/** + * Used to hold intermediate meta-information during + * constraint creation or alteration. + */ +struct constraint_parse { + /** Name of table which constraint belongs to. */ + struct SrcList *table_name; + /** Name of the constraint currently being parsed. */ + struct Token name; +}; + /* * An SQL parser context. A copy of this structure is passed through * the parser and down into all the parser action routine in order to @@ -2728,7 +2739,6 @@ struct Parse { int nLabel; /* Number of labels used */ int *aLabel; /* Space to hold the labels */ ExprList *pConstExpr; /* Constant expressions */ - Token constraintName; /* Name of the constraint currently being parsed */ int nMaxArg; /* Max args passed to user function by sub-program */ int nSelect; /* Number of SELECT statements seen */ int nSelectIndent; /* How far to indent SELECTTRACE() output */ @@ -2781,6 +2791,10 @@ struct Parse { TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */ With *pWith; /* Current WITH clause, or NULL */ With *pWithToFree; /* Free this WITH object at the end of the parse */ + /** + * Constraint currently being parsed. + */ + struct constraint_parse *constraint; /** * Number of FK constraints declared within * CREATE TABLE statement. @@ -4136,11 +4150,6 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred); * OR to handle <CREATE TABLE ...> * * @param parse_context Parsing context. - * @param child Name of table to be altered. NULL on CREATE TABLE - * statement processing. - * @param constraint Name of the constraint to be created. May be - * NULL on CREATE TABLE statement processing. - * Then, auto-generated name is used. * @param child_cols Columns of child table involved in FK. * May be NULL on CREATE TABLE statement processing. * If so, the last column added is used. @@ -4152,8 +4161,7 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred); * algorithms (e.g. CASCADE, RESTRICT etc). */ void -sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, - struct Token *constraint, struct ExprList *child_cols, +sql_create_foreign_key(struct Parse *parse_context, struct ExprList *child_cols, struct Token *parent, struct ExprList *parent_cols, bool is_deferred, int actions); @@ -4162,12 +4170,10 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, * <ALTER TABLE table DROP CONSTRAINT constraint> SQL statement. * * @param parse_context Parsing context. - * @param table Table to be altered. * @param constraint Name of constraint to be dropped. */ void -sql_drop_foreign_key(struct Parse *parse_context, struct SrcList *table, - struct Token *constraint); +sql_drop_foreign_key(struct Parse *parse_context, struct Token *constraint); void sqlite3Detach(Parse *, Expr *); int sqlite3AtoF(const char *z, double *, int); @@ -4385,12 +4391,10 @@ extern int sqlite3PendingByte; * command. * * @param parse Current parsing context. - * @param src_tab The table to rename. * @param new_name_tk Token containing new name of the table. */ void -sql_alter_table_rename(struct Parse *parse, struct SrcList *src_tab, - struct Token *new_name_tk); +sql_alter_table_rename(struct Parse *parse, struct Token *new_name_tk); /** * Return the length (in bytes) of the token that begins at z[0]. -- 2.15.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 1/6] sql: move constraint name to struct contraint_parse 2019-01-09 12:13 ` [tarantool-patches] [PATCH 1/6] sql: move constraint name to struct contraint_parse Nikita Pettik @ 2019-01-14 14:04 ` Vladislav Shpilevoy 2019-01-16 20:06 ` n.pettik 0 siblings, 1 reply; 24+ messages in thread From: Vladislav Shpilevoy @ 2019-01-14 14:04 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Hi! Thanks for the patch! See 3 comments below. On 09/01/2019 15:13, Nikita Pettik wrote: > Before this patch name of constraint being parsed was held as a separate > struct Token within parser context. Meanwhile, we also need to store > name of table which constraint is related to (in order to implement > ALTER TABLE ADD CONSTRAINT with several options: foreign key, unique, > primary key, etc). Hence, lets move constraint name to a separate > structure and save it alongside with the name of table. > > Needed for #3097 > --- > src/box/sql/alter.c | 4 ++-- > src/box/sql/build.c | 36 +++++++++++++++++++----------------- > src/box/sql/parse.y | 22 +++++++++++++--------- > src/box/sql/prepare.c | 11 +++++++++++ > src/box/sql/sqliteInt.h | 32 ++++++++++++++++++-------------- > 5 files changed, 63 insertions(+), 42 deletions(-) > > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 4110a5991..51a5d01b5 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2691,6 +2691,17 @@ struct fkey_parse { > struct rlist link; > }; > > +/** > + * Used to hold intermediate meta-information during > + * constraint creation or alteration. > + */ > +struct constraint_parse { > + /** Name of table which constraint belongs to. */ > + struct SrcList *table_name; 1. I guess, it is not a 'table_name', but a 'table' since it is struct SrcList, not const char * nor Token. > + /** Name of the constraint currently being parsed. */ > + struct Token name; > +}; 2.1. Also, I see that struct constraint_parse is not able to describe a foreign key - it has no parent table, referenced columns - you pass them separately from struct constraint_parse which looks contr-intuitive. Can you please, elaborate so it, for example, has struct fkey_parse as a member? Or at least, have both parent and child table name and cols as Token and ExprList pointers? > + > /* > * An SQL parser context. A copy of this structure is passed through > * the parser and down into all the parser action routine in order to > @@ -2781,6 +2791,10 @@ struct Parse { > TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */ > With *pWith; /* Current WITH clause, or NULL */ > With *pWithToFree; /* Free this WITH object at the end of the parse */ > + /** > + * Constraint currently being parsed. > + */ > + struct constraint_parse *constraint; 3. Make it an object, not a pointer, please. Anyway you allocate it on each prepare. > /** > * Number of FK constraints declared within > * CREATE TABLE statement. > @@ -4152,8 +4161,7 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred); > * algorithms (e.g. CASCADE, RESTRICT etc). > */ > void > -sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, > - struct Token *constraint, struct ExprList *child_cols, > +sql_create_foreign_key(struct Parse *parse_context, struct ExprList *child_cols, > struct Token *parent, struct ExprList *parent_cols, > bool is_deferred, int actions); 2.2 This is what I've described in point 2 - child part is partially moved into struct contraint_parse, whilst parent still is separate. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 1/6] sql: move constraint name to struct contraint_parse 2019-01-14 14:04 ` [tarantool-patches] " Vladislav Shpilevoy @ 2019-01-16 20:06 ` n.pettik 2019-01-16 20:54 ` Vladislav Shpilevoy 2019-01-17 10:56 ` Konstantin Osipov 0 siblings, 2 replies; 24+ messages in thread From: n.pettik @ 2019-01-16 20:06 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy >> Before this patch name of constraint being parsed was held as a separate >> struct Token within parser context. Meanwhile, we also need to store >> name of table which constraint is related to (in order to implement >> ALTER TABLE ADD CONSTRAINT with several options: foreign key, unique, >> primary key, etc). Hence, lets move constraint name to a separate >> structure and save it alongside with the name of table. >> Needed for #3097 >> --- >> src/box/sql/alter.c | 4 ++-- >> src/box/sql/build.c | 36 +++++++++++++++++++----------------- >> src/box/sql/parse.y | 22 +++++++++++++--------- >> src/box/sql/prepare.c | 11 +++++++++++ >> src/box/sql/sqliteInt.h | 32 ++++++++++++++++++-------------- >> 5 files changed, 63 insertions(+), 42 deletions(-) >> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h >> index 4110a5991..51a5d01b5 100644 >> --- a/src/box/sql/sqliteInt.h >> +++ b/src/box/sql/sqliteInt.h >> @@ -2691,6 +2691,17 @@ struct fkey_parse { >> struct rlist link; >> }; >> +/** >> + * Used to hold intermediate meta-information during >> + * constraint creation or alteration. >> + */ >> +struct constraint_parse { >> + /** Name of table which constraint belongs to. */ >> + struct SrcList *table_name; > > 1. I guess, it is not a 'table_name', but a 'table' since it > is struct SrcList, not const char * nor Token. Ok: diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c index dfd71792b..32e8b9ae0 100644 --- a/src/box/sql/alter.c +++ b/src/box/sql/alter.c @@ -40,7 +40,7 @@ void sql_alter_table_rename(struct Parse *parse, struct Token *new_name_tk) { - struct SrcList *src_tab = parse->constraint->table_name; + struct SrcList *src_tab = parse->constraint->table; assert(src_tab->nSrc == 1); struct sqlite3 *db = parse->db; char *new_name = sqlite3NameFromToken(db, new_name_tk); diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 963b16626..4367149d0 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1836,7 +1836,7 @@ sql_create_foreign_key(struct Parse *parse_context, struct ExprList *child_cols, struct space *child_space = NULL; if (is_alter) { const char *child_name = - parse_context->constraint->table_name->a[0].zName; + parse_context->constraint->table->a[0].zName; child_space = space_by_name(child_name); if (child_space == NULL) { diag_set(ClientError, ER_NO_SUCH_SPACE, child_name); @@ -2021,7 +2021,7 @@ void sql_drop_foreign_key(struct Parse *parse_context, struct Token *constraint) { const char *table_name = - parse_context->constraint->table_name->a[0].zName; + parse_context->constraint->table->a[0].zName; assert(table_name != NULL); struct space *child = space_by_name(table_name); if (child == NULL) { diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index f4fdf58f2..c3677765e 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1445,20 +1445,20 @@ cmd ::= ANALYZE nm(X). {sqlite3Analyze(pParse, &X);} //////////////////////// ALTER TABLE table ... //////////////////////////////// cmd ::= ALTER TABLE fullname(X) RENAME TO nm(Z). { - pParse->constraint->table_name = X; + pParse->constraint->table = X; sql_alter_table_rename(pParse, &Z); } cmd ::= ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { - pParse->constraint->table_name = X; + pParse->constraint->table = X; pParse->constraint->name = Z; sql_create_foreign_key(pParse, FA, &T, TA, D, R); } cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). { - pParse->constraint->table_name = X; + pParse->constraint->table = X; sql_drop_foreign_key(pParse, &Z); } diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 51a5d01b5..981e3c660 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2697,7 +2697,7 @@ struct fkey_parse { */ struct constraint_parse { /** Name of table which constraint belongs to. */ - struct SrcList *table_name; + struct SrcList *table; /** Name of the constraint currently being parsed. */ struct Token name; }; > >> + /** Name of the constraint currently being parsed. */ >> + struct Token name; >> +}; > > 2.1. Also, I see that struct constraint_parse is not able to > describe a foreign key - it has no parent table, referenced > columns - you pass them separately from struct constraint_parse > which looks contr-intuitive. Can you please, elaborate so it, > for example, has struct fkey_parse as a member? Or at least, > have both parent and child table name and cols as Token and > ExprList pointers? According to ANSI only three constraints can be added: UNIQUE/PK, FK and CHECK. Only name of table and name of constraint are shared among features of these constraints: CHECK also has ExprSpan and UNIQUE - columns (child). I guess it is fair to move child cols to this structure, but other members seems to be redundant. Hence, I will add parent name and parent colls only if you insist. diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 963b16626..af961592a 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -727,8 +726,10 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ &token, 0)); if (list == NULL) goto primary_key_exit; - sql_create_index(pParse, 0, 0, list, 0, SORT_ORDER_ASC, + pParse->constraint.cols = list; + sql_create_index(pParse, 0, 0, 0, SORT_ORDER_ASC, false, SQL_INDEX_TYPE_CONSTRAINT_PK); + pParse->constraint.cols = NULL; if (db->mallocFailed) goto primary_key_exit; } else if (autoInc) { @@ -736,8 +737,10 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ "INTEGER PRIMARY KEY or INT PRIMARY KEY"); goto primary_key_exit; } else { - sql_create_index(pParse, 0, 0, pList, 0, sortOrder, false, + pParse->constraint.cols = pList; + sql_create_index(pParse, 0, 0, 0, sortOrder, false, SQL_INDEX_TYPE_CONSTRAINT_PK); + pParse->constraint.cols = NULL; pList = 0; if (pParse->nErr > 0) goto primary_key_exit; @@ -1800,9 +1803,9 @@ columnno_by_name(struct Parse *parse_context, const struct space *space, } void -sql_create_foreign_key(struct Parse *parse_context, 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 Token *parent, + struct ExprList *parent_cols, bool is_deferred, + int actions) { struct sqlite3 *db = parse_context->db; /* @@ -1827,6 +1830,7 @@ sql_create_foreign_key(struct Parse *parse_context, struct ExprList *child_cols, /* Whether we are processing ALTER TABLE or CREATE TABLE. */ bool is_alter = new_tab == NULL; uint32_t child_cols_count; + struct ExprList *child_cols = parse_context->constraint.cols; @@ -2226,7 +2231,7 @@ constraint_is_named(const char *name) void sql_create_index(struct Parse *parse, struct Token *token, - struct SrcList *tbl_name, struct ExprList *col_list, + struct SrcList *tbl_name, MAYBE_UNUSED struct Token *start, enum sort_order sort_order, bool if_not_exist, enum sql_index_type idx_type) { /* The index to be created. */ @@ -2235,6 +2240,7 @@ sql_create_index(struct Parse *parse, struct Token *token, char *name = NULL; struct sqlite3 *db = parse->db; assert(!db->init.busy); + struct ExprList *col_list = parse->constraint.cols; diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index f4fdf58f2..d0d436884 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -262,12 +262,12 @@ ccons ::= NULL onconf(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, +ccons ::= UNIQUE. {sql_create_index(pParse,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, &T, TA, false, R);} + {sql_create_foreign_key(pParse, &T, TA, false, R);} ccons ::= defer_subclause(D). {fkey_change_defer_mode(pParse, D);} ccons ::= COLLATE id(C). {sqlite3AddCollateType(pParse, &C);} -tcons ::= UNIQUE LP sortlist(X) RP. - {sql_create_index(pParse,0,0,X,0, - SORT_ORDER_ASC,false, - SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} +tcons ::= UNIQUE LP sortlist(X) RP. { + pParse->constraint.cols = X; + sql_create_index(pParse,0,0,0, SORT_ORDER_ASC,false, + SQL_INDEX_TYPE_CONSTRAINT_UNIQUE); +} tcons ::= CHECK LP expr(E) RP onconf. {sql_add_check_constraint(pParse,&E);} tcons ::= FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { - sql_create_foreign_key(pParse, FA, &T, TA, D, R); + pParse->constraint.cols = FA; + sql_create_foreign_key(pParse, &T, TA, D, R); } @@ -1211,7 +1213,8 @@ paren_exprlist(A) ::= LP exprlist(X) RP. {A = X;} // cmd ::= createkw(S) uniqueflag(U) INDEX ifnotexists(NE) nm(X) ON nm(Y) LP sortlist(Z) RP. { - sql_create_index(pParse, &X, sqlite3SrcListAppend(pParse->db,0,&Y), Z, &S, + pParse->constraint.cols = Z; + sql_create_index(pParse, &X, sqlite3SrcListAppend(pParse->db,0,&Y), &S, SORT_ORDER_ASC, NE, U); } cmd ::= ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { - pParse->constraint->table_name = X; - pParse->constraint->name = Z; - sql_create_foreign_key(pParse, FA, &T, TA, D, R); + pParse->constraint.table = X; + pParse->constraint.name = Z; + pParse->constraint.cols = FA; + sql_create_foreign_key(pParse, &T, TA, D, R); } diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 51a5d01b5..7b98a960a 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2697,9 +2697,14 @@ struct fkey_parse { */ struct constraint_parse { /** Name of table which constraint belongs to. */ - struct SrcList *table_name; + struct SrcList *table; /** Name of the constraint currently being parsed. */ struct Token name; + /** + * List of columns involved in constraint definition: + * indexed or referencing columns. + */ + struct ExprList *cols; @@ -3509,7 +3514,6 @@ void sqlite3IdListDelete(sqlite3 *, IdList *); * @param parse All information about this parse. * @param token Index name. May be NULL. * @param tbl_name Table to index. Use pParse->pNewTable ifNULL. - * @param col_list A list of columns to be indexed. * @param start The CREATE token that begins this statement. * @param sort_order Sort order of primary key when pList==NULL. * @param if_not_exist Omit error if index already exists. @@ -3517,7 +3521,7 @@ void sqlite3IdListDelete(sqlite3 *, IdList *); */ void sql_create_index(struct Parse *parse, struct Token *token, - struct SrcList *tbl_name, struct ExprList *col_list, + struct SrcList *tbl_name, struct Token *start, enum sort_order sort_order, bool if_not_exist, enum sql_index_type idx_type); @@ -4150,9 +4154,6 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred); * OR to handle <CREATE TABLE ...> * * @param parse_context Parsing context. - * @param child_cols Columns of child table involved in FK. - * May be NULL on CREATE TABLE statement processing. - * If so, the last column added is used. * @param parent Name of referenced table. * @param parent_cols List of referenced columns. If NULL, columns * which make up PK of referenced table are used. @@ -4161,9 +4162,9 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred); * algorithms (e.g. CASCADE, RESTRICT etc). */ void -sql_create_foreign_key(struct Parse *parse_context, 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 Token *parent, + struct ExprList *parent_cols, bool is_deferred, + int actions); >> + >> /* >> * An SQL parser context. A copy of this structure is passed through >> * the parser and down into all the parser action routine in order to >> @@ -2781,6 +2791,10 @@ struct Parse { >> TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */ >> With *pWith; /* Current WITH clause, or NULL */ >> With *pWithToFree; /* Free this WITH object at the end of the parse */ >> + /** >> + * Constraint currently being parsed. >> + */ >> + struct constraint_parse *constraint; > > 3. Make it an object, not a pointer, please. Anyway you allocate it on > each prepare. Ok: diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index 0a2f59954..824578e45 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -58,17 +58,6 @@ sqlite3Prepare(sqlite3 * db, /* Database handle. */ Parse sParse; /* Parsing context */ sql_parser_create(&sParse, db); sParse.pReprepare = pReprepare; - sParse.constraint = region_alloc(&sParse.region, - sizeof(*sParse.constraint)); - if (sParse.constraint == NULL) { - diag_set(OutOfMemory, sizeof(*sParse.constraint), "region", - "constraint"); - sql_parser_destroy(&sParse); - rc = SQLITE_NOMEM; - return rc; - } - sParse.constraint->name.z = NULL; - sParse.constraint->name.n = 0; diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 51a5d01b5..03ce7a4d9 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2794,7 +2794,7 @@ struct Parse { /** * Constraint currently being parsed. */ - struct constraint_parse *constraint; + struct constraint_parse constraint; See other monotonic changes at the end of letter. > >> /** >> * Number of FK constraints declared within >> * CREATE TABLE statement. >> @@ -4152,8 +4161,7 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred); >> * algorithms (e.g. CASCADE, RESTRICT etc). >> */ >> void >> -sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, >> - struct Token *constraint, struct ExprList *child_cols, >> +sql_create_foreign_key(struct Parse *parse_context, struct ExprList *child_cols, >> struct Token *parent, struct ExprList *parent_cols, >> bool is_deferred, int actions); > > 2.2 This is what I've described in point 2 - child part is partially moved into > struct contraint_parse, whilst parent still is separate. Whole patch: diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c index d0ce9d893..aa14bab06 100644 --- a/src/box/sql/alter.c +++ b/src/box/sql/alter.c @@ -38,9 +38,9 @@ #include "box/schema.h" void -sql_alter_table_rename(struct Parse *parse, struct SrcList *src_tab, - struct Token *new_name_tk) +sql_alter_table_rename(struct Parse *parse, struct Token *new_name_tk) { + struct SrcList *src_tab = parse->constraint.table; assert(src_tab->nSrc == 1); struct sqlite3 *db = parse->db; char *new_name = sqlite3NameFromToken(db, new_name_tk); diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 49b90b5d0..1861659d6 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -566,7 +566,7 @@ sqlite3AddColumn(Parse * pParse, Token * pName, struct type_def *type_def) column_def->affinity = type_def->type; column_def->type = sql_affinity_to_field_type(column_def->affinity); p->def->field_count++; - pParse->constraintName.n = 0; + pParse->constraint.name.n = 0; } void @@ -727,8 +727,10 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ &token, 0)); if (list == NULL) goto primary_key_exit; - sql_create_index(pParse, 0, 0, list, 0, SORT_ORDER_ASC, + pParse->constraint.cols = list; + sql_create_index(pParse, 0, 0, 0, SORT_ORDER_ASC, false, SQL_INDEX_TYPE_CONSTRAINT_PK); + pParse->constraint.cols = NULL; if (db->mallocFailed) goto primary_key_exit; } else if (autoInc) { @@ -736,8 +738,10 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ "INTEGER PRIMARY KEY or INT PRIMARY KEY"); goto primary_key_exit; } else { - sql_create_index(pParse, 0, 0, pList, 0, sortOrder, false, + pParse->constraint.cols = pList; + sql_create_index(pParse, 0, 0, 0, sortOrder, false, SQL_INDEX_TYPE_CONSTRAINT_PK); + pParse->constraint.cols = NULL; pList = 0; if (pParse->nErr > 0) goto primary_key_exit; @@ -774,9 +778,9 @@ sql_add_check_constraint(struct Parse *parser, struct ExprSpan *span) sqlite3DbFree(parser->db, expr->u.zToken); goto release_expr; } - if (parser->constraintName.n) { + if (parser->constraint.name.n) { sqlite3ExprListSetName(parser, table->def->opts.checks, - &parser->constraintName, 1); + &parser->constraint.name, 1); } } else { release_expr: @@ -1800,10 +1804,9 @@ 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 Token *parent, + struct ExprList *parent_cols, bool is_deferred, + int actions) { struct sqlite3 *db = parse_context->db; /* @@ -1828,16 +1831,17 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, /* Whether we are processing ALTER TABLE or CREATE TABLE. */ bool is_alter = new_tab == NULL; uint32_t child_cols_count; + struct ExprList *child_cols = parse_context->constraint.cols; if (child_cols == NULL) { assert(!is_alter); child_cols_count = 1; } else { child_cols_count = child_cols->nExpr; } - assert(!is_alter || (child != NULL && child->nSrc == 1)); struct space *child_space = NULL; if (is_alter) { - const char *child_name = child->a[0].zName; + const char *child_name = + parse_context->constraint.table->a[0].zName; child_space = space_by_name(child_name); if (child_space == NULL) { diag_set(ClientError, ER_NO_SUCH_SPACE, child_name); @@ -1884,18 +1888,21 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, goto exit_create_fk; } } - if (constraint == NULL && !is_alter) { - if (parse_context->constraintName.n == 0) { + if (!is_alter) { + if (parse_context->constraint.name.n == 0) { constraint_name = sqlite3MPrintf(db, "FK_CONSTRAINT_%d_%s", ++parse_context->fkey_count, new_tab->def->name); } else { - struct Token *cnstr_nm = &parse_context->constraintName; + struct Token *cnstr_nm = + &parse_context->constraint.name; constraint_name = sqlite3NameFromToken(db, cnstr_nm); } } else { - constraint_name = sqlite3NameFromToken(db, constraint); + constraint_name = + sqlite3NameFromToken(db, + &parse_context->constraint.name); } if (constraint_name == NULL) goto exit_create_fk; @@ -2016,11 +2023,11 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred) } void -sql_drop_foreign_key(struct Parse *parse_context, struct SrcList *table, - struct Token *constraint) +sql_drop_foreign_key(struct Parse *parse_context, struct Token *constraint) { - assert(table != NULL && table->nSrc == 1); - const char *table_name = table->a[0].zName; + const char *table_name = + parse_context->constraint.table->a[0].zName; + assert(table_name != NULL); struct space *child = space_by_name(table_name); if (child == NULL) { diag_set(ClientError, ER_NO_SUCH_SPACE, table_name); @@ -2224,7 +2231,7 @@ constraint_is_named(const char *name) void sql_create_index(struct Parse *parse, struct Token *token, - struct SrcList *tbl_name, struct ExprList *col_list, + struct SrcList *tbl_name, MAYBE_UNUSED struct Token *start, enum sort_order sort_order, bool if_not_exist, enum sql_index_type idx_type) { /* The index to be created. */ @@ -2233,6 +2240,7 @@ sql_create_index(struct Parse *parse, struct Token *token, char *name = NULL; struct sqlite3 *db = parse->db; assert(!db->init.busy); + struct ExprList *col_list = parse->constraint.cols; if (db->mallocFailed || parse->nErr > 0) goto exit_create_index; @@ -2311,10 +2319,10 @@ sql_create_index(struct Parse *parse, struct Token *token, } } else { char *constraint_name = NULL; - if (parse->constraintName.z != NULL) + if (parse->constraint.name.z != NULL) constraint_name = sqlite3NameFromToken(db, - &parse->constraintName); + &parse->constraint.name); /* * This naming is temporary. Now it's not diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 50bb2ba01..d0d436884 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -237,8 +237,8 @@ nm(A) ::= id(A). { carglist ::= carglist cconsdef. carglist ::= . cconsdef ::= cconsname ccons. -cconsname ::= CONSTRAINT nm(X). {pParse->constraintName = X;} -cconsname ::= . {pParse->constraintName.n = 0;} +cconsname ::= CONSTRAINT nm(X). {pParse->constraint.name = X;} +cconsname ::= . {pParse->constraint.name.n = 0;} ccons ::= DEFAULT term(X). {sqlite3AddDefaultValue(pParse,&X);} ccons ::= DEFAULT LP expr(X) RP. {sqlite3AddDefaultValue(pParse,&X);} ccons ::= DEFAULT PLUS term(X). {sqlite3AddDefaultValue(pParse,&X);} @@ -262,12 +262,12 @@ ccons ::= NULL onconf(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, +ccons ::= UNIQUE. {sql_create_index(pParse,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);} + {sql_create_foreign_key(pParse, &T, TA, false, R);} ccons ::= defer_subclause(D). {fkey_change_defer_mode(pParse, D);} ccons ::= COLLATE id(C). {sqlite3AddCollateType(pParse, &C);} @@ -308,19 +308,21 @@ init_deferred_pred_opt(A) ::= INITIALLY DEFERRED. {A = 1;} init_deferred_pred_opt(A) ::= INITIALLY IMMEDIATE. {A = 0;} tconsdef ::= tconsname tcons. -tconsname ::= CONSTRAINT nm(X). {pParse->constraintName = X;} -tconsname ::= . {pParse->constraintName.n = 0;} +tconsname ::= CONSTRAINT nm(X). {pParse->constraint.name = X;} +tconsname ::= . {pParse->constraint.name.n = 0;} tcons ::= PRIMARY KEY LP sortlist(X) autoinc(I) RP. {sqlite3AddPrimaryKey(pParse,X,I,0);} -tcons ::= UNIQUE LP sortlist(X) RP. - {sql_create_index(pParse,0,0,X,0, - SORT_ORDER_ASC,false, - SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} +tcons ::= UNIQUE LP sortlist(X) RP. { + pParse->constraint.cols = X; + sql_create_index(pParse,0,0,0, SORT_ORDER_ASC,false, + SQL_INDEX_TYPE_CONSTRAINT_UNIQUE); +} tcons ::= CHECK LP expr(E) RP onconf. {sql_add_check_constraint(pParse,&E);} tcons ::= FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { - sql_create_foreign_key(pParse, NULL, NULL, FA, &T, TA, D, R); + pParse->constraint.cols = FA; + sql_create_foreign_key(pParse, &T, TA, D, R); } %type defer_subclause_opt {int} defer_subclause_opt(A) ::= . {A = 0;} @@ -1211,7 +1213,8 @@ paren_exprlist(A) ::= LP exprlist(X) RP. {A = X;} // cmd ::= createkw(S) uniqueflag(U) INDEX ifnotexists(NE) nm(X) ON nm(Y) LP sortlist(Z) RP. { - sql_create_index(pParse, &X, sqlite3SrcListAppend(pParse->db,0,&Y), Z, &S, + pParse->constraint.cols = Z; + sql_create_index(pParse, &X, sqlite3SrcListAppend(pParse->db,0,&Y), &S, SORT_ORDER_ASC, NE, U); } @@ -1445,17 +1448,22 @@ cmd ::= ANALYZE nm(X). {sqlite3Analyze(pParse, &X);} //////////////////////// ALTER TABLE table ... //////////////////////////////// cmd ::= ALTER TABLE fullname(X) RENAME TO nm(Z). { - sql_alter_table_rename(pParse,X,&Z); + pParse->constraint.table = X; + sql_alter_table_rename(pParse, &Z); } cmd ::= ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { - sql_create_foreign_key(pParse, X, &Z, FA, &T, TA, D, R); + pParse->constraint.table = X; + pParse->constraint.name = Z; + pParse->constraint.cols = FA; + sql_create_foreign_key(pParse, &T, TA, D, R); } cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). { - sql_drop_foreign_key(pParse, X, &Z); + pParse->constraint.table = X; + sql_drop_foreign_key(pParse, &Z); } //////////////////////// COMMON TABLE EXPRESSIONS //////////////////////////// diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 4110a5991..e38e0da08 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2691,6 +2691,22 @@ struct fkey_parse { struct rlist link; }; +/** + * Used to hold intermediate meta-information during + * constraint creation or alteration. + */ +struct constraint_parse { + /** Name of table which constraint belongs to. */ + struct SrcList *table; + /** Name of the constraint currently being parsed. */ + struct Token name; + /** + * List of columns involved in constraint definition: + * indexed or referencing columns. + */ + struct ExprList *cols; +}; + /* * An SQL parser context. A copy of this structure is passed through * the parser and down into all the parser action routine in order to @@ -2728,7 +2744,6 @@ struct Parse { int nLabel; /* Number of labels used */ int *aLabel; /* Space to hold the labels */ ExprList *pConstExpr; /* Constant expressions */ - Token constraintName; /* Name of the constraint currently being parsed */ int nMaxArg; /* Max args passed to user function by sub-program */ int nSelect; /* Number of SELECT statements seen */ int nSelectIndent; /* How far to indent SELECTTRACE() output */ @@ -2781,6 +2796,10 @@ struct Parse { TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */ With *pWith; /* Current WITH clause, or NULL */ With *pWithToFree; /* Free this WITH object at the end of the parse */ + /** + * Constraint currently being parsed. + */ + struct constraint_parse constraint; /** * Number of FK constraints declared within * CREATE TABLE statement. @@ -3495,7 +3514,6 @@ void sqlite3IdListDelete(sqlite3 *, IdList *); * @param parse All information about this parse. * @param token Index name. May be NULL. * @param tbl_name Table to index. Use pParse->pNewTable ifNULL. - * @param col_list A list of columns to be indexed. * @param start The CREATE token that begins this statement. * @param sort_order Sort order of primary key when pList==NULL. * @param if_not_exist Omit error if index already exists. @@ -3503,7 +3521,7 @@ void sqlite3IdListDelete(sqlite3 *, IdList *); */ void sql_create_index(struct Parse *parse, struct Token *token, - struct SrcList *tbl_name, struct ExprList *col_list, + struct SrcList *tbl_name, struct Token *start, enum sort_order sort_order, bool if_not_exist, enum sql_index_type idx_type); @@ -4136,14 +4154,6 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred); * OR to handle <CREATE TABLE ...> * * @param parse_context Parsing context. - * @param child Name of table to be altered. NULL on CREATE TABLE - * statement processing. - * @param constraint Name of the constraint to be created. May be - * NULL on CREATE TABLE statement processing. - * Then, auto-generated name is used. - * @param child_cols Columns of child table involved in FK. - * May be NULL on CREATE TABLE statement processing. - * If so, the last column added is used. * @param parent Name of referenced table. * @param parent_cols List of referenced columns. If NULL, columns * which make up PK of referenced table are used. @@ -4152,22 +4162,19 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred); * algorithms (e.g. CASCADE, RESTRICT etc). */ 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 Token *parent, + struct ExprList *parent_cols, bool is_deferred, + int actions); /** * Function called from parser to handle * <ALTER TABLE table DROP CONSTRAINT constraint> SQL statement. * * @param parse_context Parsing context. - * @param table Table to be altered. * @param constraint Name of constraint to be dropped. */ void -sql_drop_foreign_key(struct Parse *parse_context, struct SrcList *table, - struct Token *constraint); +sql_drop_foreign_key(struct Parse *parse_context, struct Token *constraint); void sqlite3Detach(Parse *, Expr *); int sqlite3AtoF(const char *z, double *, int); @@ -4385,12 +4392,10 @@ extern int sqlite3PendingByte; * command. * * @param parse Current parsing context. - * @param src_tab The table to rename. * @param new_name_tk Token containing new name of the table. */ void -sql_alter_table_rename(struct Parse *parse, struct SrcList *src_tab, - struct Token *new_name_tk); +sql_alter_table_rename(struct Parse *parse, struct Token *new_name_tk); /** * Return the length (in bytes) of the token that begins at z[0]. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 1/6] sql: move constraint name to struct contraint_parse 2019-01-16 20:06 ` n.pettik @ 2019-01-16 20:54 ` Vladislav Shpilevoy 2019-01-17 10:56 ` Konstantin Osipov 1 sibling, 0 replies; 24+ messages in thread From: Vladislav Shpilevoy @ 2019-01-16 20:54 UTC (permalink / raw) To: n.pettik, tarantool-patches Hi! Thanks for the fixes! See 2 comments below. > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 51a5d01b5..981e3c660 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2697,7 +2697,7 @@ struct fkey_parse { > */ > struct constraint_parse { > /** Name of table which constraint belongs to. */ > - struct SrcList *table_name; > + struct SrcList *table; > /** Name of the constraint currently being parsed. */ > struct Token name; > }; > >> >>> + /** Name of the constraint currently being parsed. */ >>> + struct Token name; >>> +}; >> >> 2.1. Also, I see that struct constraint_parse is not able to >> describe a foreign key - it has no parent table, referenced >> columns - you pass them separately from struct constraint_parse >> which looks contr-intuitive. Can you please, elaborate so it, >> for example, has struct fkey_parse as a member? Or at least, >> have both parent and child table name and cols as Token and >> ExprList pointers? > > According to ANSI only three constraints can be added: > UNIQUE/PK, FK and CHECK. 1. But you've named foreign key system space _fk_constraint ... And here you have struct constraint_parse, which is not able to fit a whole fk *constraint*. > Only name of table and name > of constraint are shared among features of these constraints: > CHECK also has ExprSpan and UNIQUE - columns (child). > I guess it is fair to move child cols to this structure, > but other members seems to be redundant. > Hence, I will add parent name and parent colls > only if you insist. If I were you I would add here both child and parent attributes, is_deferred, action, but who am I to insist on anything? Please, consult server team chat whether we should consider foreign keys as constraints. Or do nothing. I am not a manager, so it is just an advice. I do not insist on elaborating this structure further. (Although it looks unfinished to me.) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 963b16626..af961592a 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > > @@ -727,8 +726,10 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ > &token, 0)); > if (list == NULL) > goto primary_key_exit; > - sql_create_index(pParse, 0, 0, list, 0, SORT_ORDER_ASC, > + pParse->constraint.cols = list; > + sql_create_index(pParse, 0, 0, 0, SORT_ORDER_ASC, > false, SQL_INDEX_TYPE_CONSTRAINT_PK); > + pParse->constraint.cols = NULL; 2. I think, that it is safer to nullify pParse->constraint.cols in the same place where it is deleted, inside sql_create_index. What do you think? Also, it removes code duplication here and in the hunk below. > if (db->mallocFailed) > goto primary_key_exit; > } else if (autoInc) { > @@ -736,8 +737,10 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ > "INTEGER PRIMARY KEY or INT PRIMARY KEY"); > goto primary_key_exit; > } else { > - sql_create_index(pParse, 0, 0, pList, 0, sortOrder, false, > + pParse->constraint.cols = pList; > + sql_create_index(pParse, 0, 0, 0, sortOrder, false, > SQL_INDEX_TYPE_CONSTRAINT_PK); > + pParse->constraint.cols = NULL; > pList = 0; > if (pParse->nErr > 0) > goto primary_key_exit; > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 1/6] sql: move constraint name to struct contraint_parse 2019-01-16 20:06 ` n.pettik 2019-01-16 20:54 ` Vladislav Shpilevoy @ 2019-01-17 10:56 ` Konstantin Osipov 2019-01-17 17:14 ` n.pettik 1 sibling, 1 reply; 24+ messages in thread From: Konstantin Osipov @ 2019-01-17 10:56 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy * n.pettik <korablev@tarantool.org> [19/01/16 23:09]: > struct constraint_parse { > /** Name of table which constraint belongs to. */ > - struct SrcList *table_name; > + struct SrcList *table; > /** Name of the constraint currently being parsed. */ > struct Token name; > + /** > + * List of columns involved in constraint definition: > + * indexed or referencing columns. > + */ > + struct ExprList *cols; Why not use constraint_def here and all future parser-specific objects? _def is our widely used abbreviation for "definition". A parser creates a constraint definition, and then it is used to create a runtime constraint object - struct constraint. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 1/6] sql: move constraint name to struct contraint_parse 2019-01-17 10:56 ` Konstantin Osipov @ 2019-01-17 17:14 ` n.pettik 0 siblings, 0 replies; 24+ messages in thread From: n.pettik @ 2019-01-17 17:14 UTC (permalink / raw) To: tarantool-patches; +Cc: Konstantin Osipov, Vladislav Shpilevoy > On 17 Jan 2019, at 13:56, Konstantin Osipov <kostja@tarantool.org> wrote: > > * n.pettik <korablev@tarantool.org> [19/01/16 23:09]: >> struct constraint_parse { >> /** Name of table which constraint belongs to. */ >> - struct SrcList *table_name; >> + struct SrcList *table; >> /** Name of the constraint currently being parsed. */ >> struct Token name; >> + /** >> + * List of columns involved in constraint definition: >> + * indexed or referencing columns. >> + */ >> + struct ExprList *cols; > > Why not use constraint_def here and all future parser-specific > objects? _def is our widely used abbreviation for "definition". A > parser creates a constraint definition, and then it is used to > create a runtime constraint object - struct constraint. Do you suggest only rename this structure to constraint_def? We don’t have constraint_def for the reason that every constraint is stored in its own way: unique constraints as a part of indexes; FK constraints are represented by fkey/fkey_def; check constraints are stored as raw strings in space format. And they don’t have nothing in common except for name (meanwhile in most cases it is auto-generated name) and name of table they are related to. If you suggest to use *for example* fkey_def instead of fkey_parse: fkey_parse is used only within parser and contains parser specific members: list of columns as they come from parser, indication of self-referencing dependency etc. I guess we are able to fit all these things in struct fkey_def, but is it worth it? Note that fkey_parse is used ONLY during creation of FK, other manipulations depend on fkey_def/fkey from struct space. Also, see snippet in the second letter, mb it is exactly what you want. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] [PATCH 2/6] sql: rework ALTER TABLE grammar 2019-01-09 12:13 [tarantool-patches] [PATCH 0/6] Introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PK Nikita Pettik 2019-01-09 12:13 ` [tarantool-patches] [PATCH 1/6] sql: move constraint name to struct contraint_parse Nikita Pettik @ 2019-01-09 12:13 ` Nikita Pettik 2019-01-14 14:05 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-17 11:51 ` Konstantin Osipov 2019-01-09 12:13 ` [tarantool-patches] [PATCH 3/6] sql: remove start token from sql_create_index args Nikita Pettik ` (3 subsequent siblings) 5 siblings, 2 replies; 24+ messages in thread From: Nikita Pettik @ 2019-01-09 12:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Since ALTER TABLE ADD CONSTRAINT can be used to add various constraint types (foreign key, unique etc), we should rework its grammar. As a reference for it lets use one from ANSI. Needed for #3097 --- src/box/sql/parse.y | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index f4fdf58f2..874a67a9b 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -318,10 +318,8 @@ tcons ::= UNIQUE LP sortlist(X) RP. SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} tcons ::= CHECK LP expr(E) RP onconf. {sql_add_check_constraint(pParse,&E);} -tcons ::= FOREIGN KEY LP eidlist(FA) RP - REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { - sql_create_foreign_key(pParse, FA, &T, TA, D, R); -} +tcons ::= foreign_key_def. + %type defer_subclause_opt {int} defer_subclause_opt(A) ::= . {A = 0;} defer_subclause_opt(A) ::= defer_subclause(A). @@ -1444,22 +1442,38 @@ cmd ::= ANALYZE. {sqlite3Analyze(pParse, 0);} cmd ::= ANALYZE nm(X). {sqlite3Analyze(pParse, &X);} //////////////////////// ALTER TABLE table ... //////////////////////////////// -cmd ::= ALTER TABLE fullname(X) RENAME TO nm(Z). { - pParse->constraint->table_name = X; +cmd ::= alter_table_start alter_table_action . + +alter_table_start ::= ALTER TABLE fullname(Z) . { + pParse->constraint->table_name = Z; + pParse->constraint->name.n = 0; +} + +alter_table_action ::= add_constraint_def. +alter_table_action ::= drop_constraint_def. +/** RENAME is ANSI extension, so it comes as a very special case. */ +alter_table_action ::= rename. + +rename ::= RENAME TO nm(Z). { sql_alter_table_rename(pParse, &Z); } -cmd ::= ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY - LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R) - defer_subclause_opt(D). { - pParse->constraint->table_name = X; - pParse->constraint->name = Z; - sql_create_foreign_key(pParse, FA, &T, TA, D, R); +add_constraint_def ::= add_constraint_start constraint_def. + +add_constraint_start ::= ADD CONSTRAINT nm(Z). { + pParse->constraint->name = Z; } -cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). { - pParse->constraint->table_name = X; - sql_drop_foreign_key(pParse, &Z); +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). { + sql_create_foreign_key(pParse, FA, &T, TA, D, R); +} + + +drop_constraint_def ::= DROP CONSTRAINT nm(Z). { + sql_drop_foreign_key(pParse, &Z); } //////////////////////// COMMON TABLE EXPRESSIONS //////////////////////////// -- 2.15.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 2/6] sql: rework ALTER TABLE grammar 2019-01-09 12:13 ` [tarantool-patches] [PATCH 2/6] sql: rework ALTER TABLE grammar Nikita Pettik @ 2019-01-14 14:05 ` Vladislav Shpilevoy 2019-01-16 20:06 ` n.pettik 2019-01-17 11:51 ` Konstantin Osipov 1 sibling, 1 reply; 24+ messages in thread From: Vladislav Shpilevoy @ 2019-01-14 14:05 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Thanks for the patch! See 5 comments below. On 09/01/2019 15:13, Nikita Pettik wrote: > Since ALTER TABLE ADD CONSTRAINT can be used to add various constraint > types (foreign key, unique etc), we should rework its grammar. 1. But you still can not add a unique constraint via ADD CONSTRAINT ... The reworked grammar can be used for foreign keys only, until the last commit. > As a reference for it lets use one from ANSI. > > Needed for #3097 > --- > src/box/sql/parse.y | 44 +++++++++++++++++++++++++++++--------------- > 1 file changed, 29 insertions(+), 15 deletions(-) > > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index f4fdf58f2..874a67a9b 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -318,10 +318,8 @@ tcons ::= UNIQUE LP sortlist(X) RP. > SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} > tcons ::= CHECK LP expr(E) RP onconf. > {sql_add_check_constraint(pParse,&E);} > -tcons ::= FOREIGN KEY LP eidlist(FA) RP > - REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { > - sql_create_foreign_key(pParse, FA, &T, TA, D, R); > -} > +tcons ::= foreign_key_def. 2. Sorry for a nit, but I do not like _def for non-struct things. Why not just foreign_key? For example, 'expr' rule is just 'expr', not 'expr_def'. The same about other new _def prefixes here and in the last commit. > + > %type defer_subclause_opt {int} > defer_subclause_opt(A) ::= . {A = 0;} > defer_subclause_opt(A) ::= defer_subclause(A). > @@ -1444,22 +1442,38 @@ cmd ::= ANALYZE. {sqlite3Analyze(pParse, 0);} > cmd ::= ANALYZE nm(X). {sqlite3Analyze(pParse, &X);} > > //////////////////////// ALTER TABLE table ... //////////////////////////////// > -cmd ::= ALTER TABLE fullname(X) RENAME TO nm(Z). { > - pParse->constraint->table_name = X; > +cmd ::= alter_table_start alter_table_action . 3. The same. How about 'alter_table_action' -> 'alter_table'? Also, as I understand, you did not inline alter_table_start here because you need its C code be executed before alter_table_action, right? Then how about a comment on it? > + > +alter_table_start ::= ALTER TABLE fullname(Z) . { > + pParse->constraint->table_name = Z; > + pParse->constraint->name.n = 0; > +} > + > +alter_table_action ::= add_constraint_def. > +alter_table_action ::= drop_constraint_def. > +/** RENAME is ANSI extension, so it comes as a very special case. */ > +alter_table_action ::= rename. 4. It is an extension but why is it special? It is just one of the ways of table alteration. > + > +rename ::= RENAME TO nm(Z). { > sql_alter_table_rename(pParse, &Z); > } > > -cmd ::= ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY > - LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R) > - defer_subclause_opt(D). { > - pParse->constraint->table_name = X; > - pParse->constraint->name = Z; > - sql_create_foreign_key(pParse, FA, &T, TA, D, R); > +add_constraint_def ::= add_constraint_start constraint_def. > + > +add_constraint_start ::= ADD CONSTRAINT nm(Z). { > + pParse->constraint->name = Z; > } > > -cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). { > - pParse->constraint->table_name = X; > - sql_drop_foreign_key(pParse, &Z); > +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). { > + sql_create_foreign_key(pParse, FA, &T, TA, D, R); > +} > + > + 5. Double empty-line. > +drop_constraint_def ::= DROP CONSTRAINT nm(Z). { > + sql_drop_foreign_key(pParse, &Z); > } > > //////////////////////// COMMON TABLE EXPRESSIONS //////////////////////////// > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 2/6] sql: rework ALTER TABLE grammar 2019-01-14 14:05 ` [tarantool-patches] " Vladislav Shpilevoy @ 2019-01-16 20:06 ` n.pettik 2019-01-16 20:54 ` Vladislav Shpilevoy 0 siblings, 1 reply; 24+ messages in thread From: n.pettik @ 2019-01-16 20:06 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy > On 09/01/2019 15:13, Nikita Pettik wrote: >> Since ALTER TABLE ADD CONSTRAINT can be used to add various constraint >> types (foreign key, unique etc), we should rework its grammar. > > 1. But you still can not add a unique constraint via ADD CONSTRAINT ... > The reworked grammar can be used for foreign keys only, until the last > commit. Yep, and what is your objection? Sorry, really can’t understand. >> As a reference for it lets use one from ANSI. >> Needed for #3097 >> --- >> src/box/sql/parse.y | 44 +++++++++++++++++++++++++++++--------------- >> 1 file changed, 29 insertions(+), 15 deletions(-) >> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y >> index f4fdf58f2..874a67a9b 100644 >> --- a/src/box/sql/parse.y >> +++ b/src/box/sql/parse.y >> @@ -318,10 +318,8 @@ tcons ::= UNIQUE LP sortlist(X) RP. >> SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} >> tcons ::= CHECK LP expr(E) RP onconf. >> {sql_add_check_constraint(pParse,&E);} >> -tcons ::= FOREIGN KEY LP eidlist(FA) RP >> - REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { >> - sql_create_foreign_key(pParse, FA, &T, TA, D, R); >> -} >> +tcons ::= foreign_key_def. > > 2. Sorry for a nit, but I do not like _def for non-struct things. Why not > just foreign_key? For example, 'expr' rule is just 'expr', not 'expr_def'. > The same about other new _def prefixes here and in the last commit. I took these names from ANSI grammar: <referential constraint definition> ::= … I can expose it to foreign_key_definition, for instance. > >> + >> %type defer_subclause_opt {int} >> defer_subclause_opt(A) ::= . {A = 0;} >> defer_subclause_opt(A) ::= defer_subclause(A). >> @@ -1444,22 +1442,38 @@ cmd ::= ANALYZE. {sqlite3Analyze(pParse, 0);} >> cmd ::= ANALYZE nm(X). {sqlite3Analyze(pParse, &X);} >> //////////////////////// ALTER TABLE table ... //////////////////////////////// >> -cmd ::= ALTER TABLE fullname(X) RENAME TO nm(Z). { >> - pParse->constraint->table_name = X; >> +cmd ::= alter_table_start alter_table_action . > > 3. The same. How about 'alter_table_action' -> 'alter_table’? I used _action for the same reason: <alter table statement> ::= ALTER TABLE <table name> <alter table action> (11.10 alter table statement) To be honest I like this naming, so I am going to keep it as is. > Also, as I understand, you did not inline alter_table_start here > because you need its C code be executed before alter_table_action, > right? Then how about a comment on it? Yep, I just followed the same pattern as for CREATE TABLE. Came up with poor comment: diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 696400f51..de5429498 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1446,6 +1446,11 @@ cmd ::= ANALYZE nm(X). {sqlite3Analyze(pParse, &X);} //////////////////////// ALTER TABLE table ... //////////////////////////////// cmd ::= alter_table_start alter_table_action . +/** + * We should get name of the table before processing + * any other rules. So, we've put this routine at + * the separate rule. + */ alter_table_start ::= ALTER TABLE fullname(Z) . { pParse->constraint.table = Z; pParse->constraint.name.n = 0; >> + >> +alter_table_start ::= ALTER TABLE fullname(Z) . { >> + pParse->constraint->table_name = Z; >> + pParse->constraint->name.n = 0; >> +} >> + >> +alter_table_action ::= add_constraint_def. >> +alter_table_action ::= drop_constraint_def. >> +/** RENAME is ANSI extension, so it comes as a very special case. */ >> +alter_table_action ::= rename. > > 4. It is an extension but why is it special? It is just one of > the ways of table alteration. Because other actions starts from ADD/DROP/ALTER (which we still don’t have). But ok, I’ve removed comment: diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 696400f51..82736e76a 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1453,7 +1453,6 @@ alter_table_start ::= ALTER TABLE fullname(Z) . { alter_table_action ::= add_constraint_def. alter_table_action ::= drop_constraint_def. -/** RENAME is ANSI extension, so it comes as a very special case. */ alter_table_action ::= rename. > >> + >> +rename ::= RENAME TO nm(Z). { >> sql_alter_table_rename(pParse, &Z); >> } >> -cmd ::= ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY >> - LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R) >> - defer_subclause_opt(D). { >> - pParse->constraint->table_name = X; >> - pParse->constraint->name = Z; >> - sql_create_foreign_key(pParse, FA, &T, TA, D, R); >> +add_constraint_def ::= add_constraint_start constraint_def. >> + >> +add_constraint_start ::= ADD CONSTRAINT nm(Z). { >> + pParse->constraint->name = Z; >> } >> -cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). { >> - pParse->constraint->table_name = X; >> - sql_drop_foreign_key(pParse, &Z); >> +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). { >> + sql_create_foreign_key(pParse, FA, &T, TA, D, R); >> +} >> + >> + > > 5. Double empty-line. Fixed: @@ -1474,7 +1473,6 @@ foreign_key_def ::= FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T) sql_create_foreign_key(pParse, &T, TA, D, R); } - drop_constraint_def ::= DROP CONSTRAINT nm(Z). { sql_drop_foreign_key(pParse, &Z); } ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 2/6] sql: rework ALTER TABLE grammar 2019-01-16 20:06 ` n.pettik @ 2019-01-16 20:54 ` Vladislav Shpilevoy 0 siblings, 0 replies; 24+ messages in thread From: Vladislav Shpilevoy @ 2019-01-16 20:54 UTC (permalink / raw) To: n.pettik, tarantool-patches Thanks for the fixes! See 2 comments below. On 16/01/2019 23:06, n.pettik wrote: > >> On 09/01/2019 15:13, Nikita Pettik wrote: >>> Since ALTER TABLE ADD CONSTRAINT can be used to add various constraint >>> types (foreign key, unique etc), we should rework its grammar. >> >> 1. But you still can not add a unique constraint via ADD CONSTRAINT ... >> The reworked grammar can be used for foreign keys only, until the last >> commit. > > Yep, and what is your objection? > Sorry, really can’t understand. 1. You've stated that CONSTRAINT can be used for indexes, but it can not in this commit. It *could*, and it is used further, but on this commit it is not possible. I know, it is nit, but it slightly cuts my ear. > >>> As a reference for it lets use one from ANSI. >>> Needed for #3097 >>> --- >>> src/box/sql/parse.y | 44 +++++++++++++++++++++++++++++--------------- >>> 1 file changed, 29 insertions(+), 15 deletions(-) >>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y >>> index f4fdf58f2..874a67a9b 100644 >>> --- a/src/box/sql/parse.y >>> +++ b/src/box/sql/parse.y >>> @@ -318,10 +318,8 @@ tcons ::= UNIQUE LP sortlist(X) RP. >>> SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} >>> tcons ::= CHECK LP expr(E) RP onconf. >>> {sql_add_check_constraint(pParse,&E);} >>> -tcons ::= FOREIGN KEY LP eidlist(FA) RP >>> - REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { >>> - sql_create_foreign_key(pParse, FA, &T, TA, D, R); >>> -} >>> +tcons ::= foreign_key_def. >> >> 2. Sorry for a nit, but I do not like _def for non-struct things. Why not >> just foreign_key? For example, 'expr' rule is just 'expr', not 'expr_def'. >> The same about other new _def prefixes here and in the last commit. > > I took these names from ANSI grammar: > > <referential constraint definition> ::= … > > I can expose it to foreign_key_definition, for instance. 2. ANSI is good source of grammar, but I am not sure if it is so good for naming. In Tarantool we use _def for structures, describing an arising object. > >> >>> + >>> %type defer_subclause_opt {int} >>> defer_subclause_opt(A) ::= . {A = 0;} >>> defer_subclause_opt(A) ::= defer_subclause(A). >>> @@ -1444,22 +1442,38 @@ cmd ::= ANALYZE. {sqlite3Analyze(pParse, 0);} >>> cmd ::= ANALYZE nm(X). {sqlite3Analyze(pParse, &X);} >>> //////////////////////// ALTER TABLE table ... //////////////////////////////// >>> -cmd ::= ALTER TABLE fullname(X) RENAME TO nm(Z). { >>> - pParse->constraint->table_name = X; >>> +cmd ::= alter_table_start alter_table_action . >> >> 3. The same. How about 'alter_table_action' -> 'alter_table’? > > I used _action for the same reason: > > <alter table statement> ::= ALTER TABLE <table name> <alter table action> > (11.10 alter table statement) > > To be honest I like this naming, so I am going to keep it as is. I've explained my comment above and now it is up to you. I am not rigorous about names. I understand your way of naming and it is decent. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 2/6] sql: rework ALTER TABLE grammar 2019-01-09 12:13 ` [tarantool-patches] [PATCH 2/6] sql: rework ALTER TABLE grammar Nikita Pettik 2019-01-14 14:05 ` [tarantool-patches] " Vladislav Shpilevoy @ 2019-01-17 11:51 ` Konstantin Osipov 2019-01-17 17:14 ` n.pettik 1 sibling, 1 reply; 24+ messages in thread From: Konstantin Osipov @ 2019-01-17 11:51 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik * 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, add/drop constraint. It's unclear why you initialize a specific parsing context (constraint definition) right in alter_table_start rule. OK for now. > + > +alter_table_action ::= add_constraint_def. > +alter_table_action ::= drop_constraint_def. Why use the same data structure for create and drop? When I drop a constraint I specify its name and (perhaps) table name, none of other properties. 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. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 2/6] sql: rework ALTER TABLE grammar 2019-01-17 11:51 ` Konstantin Osipov @ 2019-01-17 17:14 ` n.pettik 2019-01-18 1:42 ` Konstantin Osipov 0 siblings, 1 reply; 24+ messages in thread From: n.pettik @ 2019-01-17 17:14 UTC (permalink / raw) To: tarantool-patches; +Cc: Konstantin Osipov, Vladislav Shpilevoy > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 2/6] sql: rework ALTER TABLE grammar 2019-01-17 17:14 ` n.pettik @ 2019-01-18 1:42 ` Konstantin Osipov 0 siblings, 0 replies; 24+ messages in thread From: Konstantin Osipov @ 2019-01-18 1:42 UTC (permalink / raw) To: n.pettik; +Cc: tarantool-patches, Vladislav Shpilevoy * 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] [PATCH 3/6] sql: remove start token from sql_create_index args 2019-01-09 12:13 [tarantool-patches] [PATCH 0/6] Introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PK Nikita Pettik 2019-01-09 12:13 ` [tarantool-patches] [PATCH 1/6] sql: move constraint name to struct contraint_parse Nikita Pettik 2019-01-09 12:13 ` [tarantool-patches] [PATCH 2/6] sql: rework ALTER TABLE grammar Nikita Pettik @ 2019-01-09 12:13 ` Nikita Pettik 2019-01-09 12:13 ` [tarantool-patches] [PATCH 4/6] sql: refactor getNewIid() function Nikita Pettik ` (2 subsequent siblings) 5 siblings, 0 replies; 24+ messages in thread From: Nikita Pettik @ 2019-01-09 12:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik It is not used anymore and can be removed. Part of #3097 --- src/box/sql/build.c | 10 ++++------ src/box/sql/parse.y | 9 +++++---- src/box/sql/sqliteInt.h | 5 ++--- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 963b16626..9d31cb736 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -727,7 +727,7 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ &token, 0)); if (list == NULL) goto primary_key_exit; - sql_create_index(pParse, 0, 0, list, 0, SORT_ORDER_ASC, + sql_create_index(pParse, 0, 0, list, SORT_ORDER_ASC, false, SQL_INDEX_TYPE_CONSTRAINT_PK); if (db->mallocFailed) goto primary_key_exit; @@ -736,7 +736,7 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ "INTEGER PRIMARY KEY or INT PRIMARY KEY"); goto primary_key_exit; } else { - sql_create_index(pParse, 0, 0, pList, 0, sortOrder, false, + sql_create_index(pParse, 0, 0, pList, sortOrder, false, SQL_INDEX_TYPE_CONSTRAINT_PK); pList = 0; if (pParse->nErr > 0) @@ -2227,8 +2227,8 @@ 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) { + enum sort_order sort_order, bool if_not_exist, + enum sql_index_type idx_type) { /* The index to be created. */ struct index *index = NULL; /* Name of the index. */ @@ -2269,7 +2269,6 @@ sql_create_index(struct Parse *parse, struct Token *token, if (parse->pNewTable == NULL) goto exit_create_index; assert(token == NULL); - assert(start == NULL); space = parse->pNewTable->space; def = parse->pNewTable->def; } @@ -2524,7 +2523,6 @@ sql_create_index(struct Parse *parse, struct Token *token, P4_SPACEPTR); sqlite3VdbeChangeP5(vdbe, OPFLAG_SEEKEQ); - assert(start != NULL); int index_id = getNewIid(parse, def->id, cursor); sqlite3VdbeAddOp1(vdbe, OP_Close, cursor); vdbe_emit_create_index(parse, def, index->def, diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 874a67a9b..0b1cef597 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -262,7 +262,7 @@ ccons ::= NULL onconf(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, +ccons ::= UNIQUE. {sql_create_index(pParse,0,0,0, SORT_ORDER_ASC, false, SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} ccons ::= CHECK LP expr(X) RP. {sql_add_check_constraint(pParse,&X);} @@ -313,7 +313,7 @@ tconsname ::= . {pParse->constraint->name.n = 0;} tcons ::= PRIMARY KEY LP sortlist(X) autoinc(I) RP. {sqlite3AddPrimaryKey(pParse,X,I,0);} tcons ::= UNIQUE LP sortlist(X) RP. - {sql_create_index(pParse,0,0,X,0, + {sql_create_index(pParse,0,0,X, SORT_ORDER_ASC,false, SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} tcons ::= CHECK LP expr(E) RP onconf. @@ -1207,9 +1207,10 @@ paren_exprlist(A) ::= LP exprlist(X) RP. {A = X;} ///////////////////////////// The CREATE INDEX command /////////////////////// // -cmd ::= createkw(S) uniqueflag(U) INDEX ifnotexists(NE) nm(X) +cmd ::= createkw uniqueflag(U) INDEX ifnotexists(NE) nm(X) ON nm(Y) LP sortlist(Z) RP. { - sql_create_index(pParse, &X, sqlite3SrcListAppend(pParse->db,0,&Y), Z, &S, + + sql_create_index(pParse, &X, sqlite3SrcListAppend(pParse->db,0,&Y), Z, SORT_ORDER_ASC, NE, U); } diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 51a5d01b5..67243ca01 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3510,7 +3510,6 @@ void sqlite3IdListDelete(sqlite3 *, IdList *); * @param token Index name. May be NULL. * @param tbl_name Table to index. Use pParse->pNewTable ifNULL. * @param col_list A list of columns to be indexed. - * @param start The CREATE token that begins this statement. * @param sort_order Sort order of primary key when pList==NULL. * @param if_not_exist Omit error if index already exists. * @param idx_type The index type. @@ -3518,8 +3517,8 @@ void sqlite3IdListDelete(sqlite3 *, IdList *); void sql_create_index(struct Parse *parse, struct Token *token, struct SrcList *tbl_name, struct ExprList *col_list, - struct Token *start, enum sort_order sort_order, - bool if_not_exist, enum sql_index_type idx_type); + enum sort_order sort_order, bool if_not_exist, + enum sql_index_type idx_type); /** * This routine will drop an existing named index. This routine -- 2.15.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] [PATCH 4/6] sql: refactor getNewIid() function 2019-01-09 12:13 [tarantool-patches] [PATCH 0/6] Introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PK Nikita Pettik ` (2 preceding siblings ...) 2019-01-09 12:13 ` [tarantool-patches] [PATCH 3/6] sql: remove start token from sql_create_index args Nikita Pettik @ 2019-01-09 12:13 ` Nikita Pettik 2019-01-14 14:05 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-09 12:13 ` [tarantool-patches] [PATCH 5/6] sql: fix error message for improperly created index Nikita Pettik 2019-01-09 12:13 ` [tarantool-patches] [PATCH 6/6] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY Nikita Pettik 5 siblings, 1 reply; 24+ messages in thread From: Nikita Pettik @ 2019-01-09 12:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik This commit includes no functional changes. Lets simply rewrite getNewIid() function according to Tarantool codestyle. Part of #3914 --- src/box/sql/build.c | 72 ++++++++++++++++++++++++++--------------------------- 1 file changed, 35 insertions(+), 37 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 9d31cb736..ca9d469fd 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2044,45 +2044,43 @@ sql_drop_foreign_key(struct Parse *parse_context, struct Token *constraint) sqlite3VdbeChangeP5(sqlite3GetVdbe(parse_context), OPFLAG_NCHANGE); } -/* - * Generate code to determine next free Iid in the space identified by - * the iSpaceId. Return register number holding the result. +/** + * Generate code to determine next free index id in + * the space identified by the @space_id. + * Return register holding the result. + * + * Overall VDBE program logic is following: + * + * 1 Seek for space id in _index, goto l1 if seeks fails. + * 2 Goto l2. + * 3 l1: Halt. + * 4 l2: Fetch index id from _index record. */ static int -getNewIid(Parse * pParse, int iSpaceId, int iCursor) +generate_index_id(struct Parse *parse, uint32_t space_id, int cursor) { - Vdbe *v = sqlite3GetVdbe(pParse); - int iRes = ++pParse->nMem; - int iKey = ++pParse->nMem; - int iSeekInst, iGotoInst; - - sqlite3VdbeAddOp2(v, OP_Integer, iSpaceId, iKey); - iSeekInst = sqlite3VdbeAddOp4Int(v, OP_SeekLE, iCursor, 0, iKey, 1); - sqlite3VdbeAddOp4Int(v, OP_IdxLT, iCursor, 0, iKey, 1); - - /* - * If SeekLE succeeds, the control falls through here, skipping - * IdxLt. - * - * If it fails (no entry with the given key prefix: invalid spaceId) - * VDBE jumps to the next code block (jump target is IMM, fixed up - * later with sqlite3VdbeJumpHere()). - */ - iGotoInst = sqlite3VdbeAddOp0(v, OP_Goto); /* Jump over Halt */ - - /* Invalid spaceId detected. Halt now. */ - sqlite3VdbeJumpHere(v, iSeekInst); - sqlite3VdbeJumpHere(v, iSeekInst + 1); - sqlite3VdbeAddOp4(v, - OP_Halt, SQLITE_ERROR, ON_CONFLICT_ACTION_FAIL, 0, - sqlite3MPrintf(pParse->db, "Invalid space id: %d", - iSpaceId), P4_DYNAMIC); - - /* Fetch iid from the row and ++it. */ - sqlite3VdbeJumpHere(v, iGotoInst); - sqlite3VdbeAddOp3(v, OP_Column, iCursor, 1, iRes); - sqlite3VdbeAddOp2(v, OP_AddImm, iRes, 1); - return iRes; + struct Vdbe *v = sqlite3GetVdbe(parse); + int key_reg = ++parse->nMem; + + sqlite3VdbeAddOp2(v, OP_Integer, space_id, key_reg); + int seek_adr = sqlite3VdbeAddOp4Int(v, OP_SeekLE, cursor, 0, + key_reg, 1); + sqlite3VdbeAddOp4Int(v, OP_IdxLT, cursor, 0, key_reg, 1); + /* Jump over Halt block. */ + int goto_succ_addr = sqlite3VdbeAddOp0(v, OP_Goto); + /* Invalid space id handling block starts here. */ + sqlite3VdbeJumpHere(v, seek_adr); + sqlite3VdbeJumpHere(v, seek_adr + 1); + sqlite3VdbeAddOp4(v, OP_Halt, SQLITE_ERROR, ON_CONFLICT_ACTION_FAIL, 0, + sqlite3MPrintf(parse->db, "Invalid space id: %d", + space_id), P4_DYNAMIC); + + sqlite3VdbeJumpHere(v, goto_succ_addr); + /* Fetch iid from the row and increment it. */ + int iid_reg = ++parse->nMem; + sqlite3VdbeAddOp3(v, OP_Column, cursor, 1, iid_reg); + sqlite3VdbeAddOp2(v, OP_AddImm, iid_reg, 1); + return iid_reg; } /** @@ -2523,7 +2521,7 @@ sql_create_index(struct Parse *parse, struct Token *token, P4_SPACEPTR); sqlite3VdbeChangeP5(vdbe, OPFLAG_SEEKEQ); - int index_id = getNewIid(parse, def->id, cursor); + int index_id = generate_index_id(parse, def->id, cursor); sqlite3VdbeAddOp1(vdbe, OP_Close, cursor); vdbe_emit_create_index(parse, def, index->def, def->id, index_id); -- 2.15.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 4/6] sql: refactor getNewIid() function 2019-01-09 12:13 ` [tarantool-patches] [PATCH 4/6] sql: refactor getNewIid() function Nikita Pettik @ 2019-01-14 14:05 ` Vladislav Shpilevoy 0 siblings, 0 replies; 24+ messages in thread From: Vladislav Shpilevoy @ 2019-01-14 14:05 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Thanks for the patch! See 1 comment below. On 09/01/2019 15:13, Nikita Pettik wrote: > This commit includes no functional changes. Lets simply rewrite > getNewIid() function according to Tarantool codestyle. > > Part of #3914 > --- > src/box/sql/build.c | 72 ++++++++++++++++++++++++++--------------------------- > 1 file changed, 35 insertions(+), 37 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 9d31cb736..ca9d469fd 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > - /* Fetch iid from the row and ++it. */ > - sqlite3VdbeJumpHere(v, iGotoInst); > - sqlite3VdbeAddOp3(v, OP_Column, iCursor, 1, iRes); > - sqlite3VdbeAddOp2(v, OP_AddImm, iRes, 1); > - return iRes; > + struct Vdbe *v = sqlite3GetVdbe(parse); > + int key_reg = ++parse->nMem; > + > + sqlite3VdbeAddOp2(v, OP_Integer, space_id, key_reg); > + int seek_adr = sqlite3VdbeAddOp4Int(v, OP_SeekLE, cursor, 0, > + key_reg, 1); > + sqlite3VdbeAddOp4Int(v, OP_IdxLT, cursor, 0, key_reg, 1); > + /* Jump over Halt block. */ > + int goto_succ_addr = sqlite3VdbeAddOp0(v, OP_Goto); > + /* Invalid space id handling block starts here. */ > + sqlite3VdbeJumpHere(v, seek_adr); > + sqlite3VdbeJumpHere(v, seek_adr + 1); > + sqlite3VdbeAddOp4(v, OP_Halt, SQLITE_ERROR, ON_CONFLICT_ACTION_FAIL, 0, > + sqlite3MPrintf(parse->db, "Invalid space id: %d", > + space_id), P4_DYNAMIC); > + > + sqlite3VdbeJumpHere(v, goto_succ_addr); > + /* Fetch iid from the row and increment it. */ > + int iid_reg = ++parse->nMem; > + sqlite3VdbeAddOp3(v, OP_Column, cursor, 1, iid_reg); 1 -> BOX_INDEX_FIELD_ID. > + sqlite3VdbeAddOp2(v, OP_AddImm, iid_reg, 1); > + return iid_reg; > } > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] [PATCH 5/6] sql: fix error message for improperly created index 2019-01-09 12:13 [tarantool-patches] [PATCH 0/6] Introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PK Nikita Pettik ` (3 preceding siblings ...) 2019-01-09 12:13 ` [tarantool-patches] [PATCH 4/6] sql: refactor getNewIid() function Nikita Pettik @ 2019-01-09 12:13 ` Nikita Pettik 2019-01-14 14:06 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-09 12:13 ` [tarantool-patches] [PATCH 6/6] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY Nikita Pettik 5 siblings, 1 reply; 24+ messages in thread From: Nikita Pettik @ 2019-01-09 12:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Table can be created without any indexes (for instance, from Lua-land). On the other hand, bytecode generated for CREATE INDEX statement attempts at finding entry in _index space with given space id. In case it is not found error "wrong space id" is raised. On the other hand, it is obvious that such message is appeared when table doesn't have any created indexes yet. Hence, lets fix this message to indicate that primary key should be created before any secondary indexes. Closes #3914 --- src/box/sql/build.c | 4 ++-- test/sql-tap/index1.test.lua | 28 +++++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index ca9d469fd..1d7b6c827 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2072,8 +2072,8 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor) sqlite3VdbeJumpHere(v, seek_adr); sqlite3VdbeJumpHere(v, seek_adr + 1); sqlite3VdbeAddOp4(v, OP_Halt, SQLITE_ERROR, ON_CONFLICT_ACTION_FAIL, 0, - sqlite3MPrintf(parse->db, "Invalid space id: %d", - space_id), P4_DYNAMIC); + "can not add a secondary key before primary", + P4_STATIC); sqlite3VdbeJumpHere(v, goto_succ_addr); /* Fetch iid from the row and increment it. */ diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua index 49f61a52a..ccb6753c9 100755 --- a/test/sql-tap/index1.test.lua +++ b/test/sql-tap/index1.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(70) +test:plan(73) --!./tcltestrunner.lua -- 2001 September 15 @@ -1016,5 +1016,31 @@ if (0 > 0) end +test:do_test( + "index-22.1.0", + function() + format = {} + format[1] = { name = 'id', type = 'scalar'} + format[2] = { name = 'f2', type = 'scalar'} + s = box.schema.create_space('T', {format = format}) + end, + {}) + +test:do_catchsql_test( + "alter-8.1.1", + [[ + CREATE UNIQUE INDEX pk ON t("id"); + ]], { + 1, "can not add a secondary key before primary" + }) + +test:do_catchsql_test( + "alter-8.2", + [[ + ALTER TABLE t ADD CONSTRAINT pk PRIMARY KEY("id"); + CREATE UNIQUE INDEX i ON t("id"); + ]], { + 0 +}) test:finish_test() -- 2.15.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 5/6] sql: fix error message for improperly created index 2019-01-09 12:13 ` [tarantool-patches] [PATCH 5/6] sql: fix error message for improperly created index Nikita Pettik @ 2019-01-14 14:06 ` Vladislav Shpilevoy 2019-01-16 20:06 ` n.pettik 0 siblings, 1 reply; 24+ messages in thread From: Vladislav Shpilevoy @ 2019-01-14 14:06 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Thanks for the patch! See 1 comment below. On 09/01/2019 15:13, Nikita Pettik wrote: > Table can be created without any indexes (for instance, from Lua-land). > On the other hand, bytecode generated for CREATE INDEX statement > attempts at finding entry in _index space with given space id. > In case it is not found error "wrong space id" is raised. On the other > hand, it is obvious that such message is appeared when table doesn't > have any created indexes yet. Hence, lets fix this message to indicate > that primary key should be created before any secondary indexes. > > Closes #3914 The test index1.test.lua fails. I guess, because you introduce ADD CONSTRAINT PRIMARY KEY only in the next commit. Lets move this part of the test into the latter. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 5/6] sql: fix error message for improperly created index 2019-01-14 14:06 ` [tarantool-patches] " Vladislav Shpilevoy @ 2019-01-16 20:06 ` n.pettik 0 siblings, 0 replies; 24+ messages in thread From: n.pettik @ 2019-01-16 20:06 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy > On 14 Jan 2019, at 17:06, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: > > Thanks for the patch! See 1 comment below. > > On 09/01/2019 15:13, Nikita Pettik wrote: >> Table can be created without any indexes (for instance, from Lua-land). >> On the other hand, bytecode generated for CREATE INDEX statement >> attempts at finding entry in _index space with given space id. >> In case it is not found error "wrong space id" is raised. On the other >> hand, it is obvious that such message is appeared when table doesn't >> have any created indexes yet. Hence, lets fix this message to indicate >> that primary key should be created before any secondary indexes. >> Closes #3914 > > The test index1.test.lua fails. I guess, because you introduce > ADD CONSTRAINT PRIMARY KEY only in the next commit. Lets move this > part of the test into the latter. I guess I accidentally put it in this patch. Moved to the next one. diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua index ccb6753c9..121381747 100755 --- a/test/sql-tap/index1.test.lua +++ b/test/sql-tap/index1.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(73) +test:plan(72) --!./tcltestrunner.lua -- 2001 September 15 @@ -1034,13 +1034,4 @@ test:do_catchsql_test( 1, "can not add a secondary key before primary" }) -test:do_catchsql_test( - "alter-8.2", - [[ - ALTER TABLE t ADD CONSTRAINT pk PRIMARY KEY("id"); - CREATE UNIQUE INDEX i ON t("id"); - ]], { - 0 -}) - ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] [PATCH 6/6] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY 2019-01-09 12:13 [tarantool-patches] [PATCH 0/6] Introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PK Nikita Pettik ` (4 preceding siblings ...) 2019-01-09 12:13 ` [tarantool-patches] [PATCH 5/6] sql: fix error message for improperly created index Nikita Pettik @ 2019-01-09 12:13 ` Nikita Pettik 2019-01-14 14:06 ` [tarantool-patches] " Vladislav Shpilevoy 5 siblings, 1 reply; 24+ messages in thread From: Nikita Pettik @ 2019-01-09 12:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik Table (aka space) can be created without indexes at least from Lua-land (note that according ANSI SQL table may lack PK). Since there were no facilities to create primary key constraint on already existing table, lets extend ADD CONSTRAINT statement with UNIQUE and PRIMARY KEY clauses. In this case, UNIQUE works exactly in the same way as CREATE UNIQUE INDEX ... statement does. In Tarantool primary index is an index with id == 0, so during execution of ADD CONSTRAINT PRIMARY KEY we check that there is no any entries in _index space and create that one. Otherwise, error is raised. Part of #3097 Follow-up #3914 --- src/box/sql/build.c | 29 +++++++++++++++++++++-- src/box/sql/parse.y | 15 ++++++++++++ test/sql-tap/alter.test.lua | 58 ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 99 insertions(+), 3 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 1d7b6c827..0314be957 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2083,6 +2083,19 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor) return iid_reg; } +static void +pk_check_existence(struct Parse *parse, uint32_t space_id, int cursor) +{ + struct Vdbe *v = sqlite3GetVdbe(parse); + int tmp_reg = ++parse->nMem; + sqlite3VdbeAddOp2(v, OP_Integer, space_id, tmp_reg); + int found_addr = sqlite3VdbeAddOp4Int(v, OP_NotFound, cursor, 0, + tmp_reg, 1); + sqlite3VdbeAddOp4(v, OP_Halt, SQLITE_ERROR, ON_CONFLICT_ACTION_FAIL, 0, + "multiple primary keys are not allowed", P4_STATIC); + sqlite3VdbeJumpHere(v, found_addr); +} + /** * Add new index to table's indexes list. * We follow convention that PK comes first in list. @@ -2520,8 +2533,20 @@ sql_create_index(struct Parse *parse, struct Token *token, (void *)space_by_id(BOX_INDEX_ID), P4_SPACEPTR); sqlite3VdbeChangeP5(vdbe, OPFLAG_SEEKEQ); - - int index_id = generate_index_id(parse, def->id, cursor); + int index_id; + /* + * In case we are creating PRIMARY KEY constraint + * (via ALTER TABLE) we must ensure that table + * doesn't feature any indexes. Otherwise, + * we can immediately halt execution of VDBE. + */ + if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK) { + pk_check_existence(parse, def->id, cursor); + index_id = parse->nMem++; + sqlite3VdbeAddOp2(vdbe, OP_Integer, 0, index_id); + } else { + index_id = generate_index_id(parse, def->id, cursor); + } sqlite3VdbeAddOp1(vdbe, OP_Close, cursor); vdbe_emit_create_index(parse, def, index->def, def->id, index_id); diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 0b1cef597..82aeee2b5 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1466,6 +1466,7 @@ add_constraint_start ::= ADD CONSTRAINT nm(Z). { } constraint_def ::= foreign_key_def. +constraint_def ::= unique_def. foreign_key_def ::= FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). { @@ -1473,6 +1474,20 @@ foreign_key_def ::= FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T) } +unique_def ::= unique_spec(U) LP sortlist(X) RP. { + sql_create_index(pParse, &pParse->constraint->name, + pParse->constraint->table_name, X, SORT_ORDER_ASC, false, U); + } + +%type unique_spec {int} +unique_spec(U) ::= UNIQUE. { U = SQL_INDEX_TYPE_CONSTRAINT_UNIQUE; } +unique_spec(U) ::= PRIMARY KEY. { U = SQL_INDEX_TYPE_CONSTRAINT_PK; } + +/** + * Currently, to drop unique constraint it is required + * to use drop index (since fk and unique constraints don't + * share name space). + */ drop_constraint_def ::= DROP CONSTRAINT nm(Z). { sql_drop_foreign_key(pParse, &Z); } diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua index 1aad555c0..925753749 100755 --- a/test/sql-tap/alter.test.lua +++ b/test/sql-tap/alter.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(43) +test:plan(50) test:do_execsql_test( "alter-1.1", @@ -517,6 +517,62 @@ test:do_catchsql_test( -- </alter-7.11> }) +test:do_test( + "alter-8.1.0", + function() + format = {} + format[1] = { name = 'id', type = 'scalar'} + format[2] = { name = 'f2', type = 'scalar'} + s = box.schema.create_space('T', {format = format}) + end, + {}) + +test:do_catchsql_test( + "alter-8.1.1", + [[ + ALTER TABLE t ADD CONSTRAINT pk PRIMARY KEY("id"); + ]], { + 0 + }) + +test:do_test( + "alter-8.1.2", + function() + i = box.space.T.index[0] + return i.id + end, 0) + +test:do_catchsql_test( + "alter-8.2", + [[ + ALTER TABLE t ADD CONSTRAINT pk1 PRIMARY KEY("f2"); + ]], { + 1, "multiple primary keys are not allowed" + }) + +test:do_catchsql_test( + "alter-8.3.1", + [[ + ALTER TABLE t ADD CONSTRAINT i1 UNIQUE("f2"); + ]], { + 0 + }) + +test:do_test( + "alter-8.3.2", + function() + i = box.space.T.index[1] + return i.id + end, 1) + +test:do_catchsql_test( + "alter-8.4", + [[ + DROP INDEX i1 ON t; + DROP INDEX pk ON t; + ]], { + 0 +}) -- Commented due to #2953 -- -- 2.15.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 6/6] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY 2019-01-09 12:13 ` [tarantool-patches] [PATCH 6/6] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY Nikita Pettik @ 2019-01-14 14:06 ` Vladislav Shpilevoy 2019-01-16 20:06 ` n.pettik 0 siblings, 1 reply; 24+ messages in thread From: Vladislav Shpilevoy @ 2019-01-14 14:06 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik Thanks for the patch! See 3 comments below. On 09/01/2019 15:13, Nikita Pettik wrote: > Table (aka space) can be created without indexes at least from Lua-land > (note that according ANSI SQL table may lack PK). Since there were no > facilities to create primary key constraint on already existing table, > lets extend ADD CONSTRAINT statement with UNIQUE and PRIMARY KEY > clauses. In this case, UNIQUE works exactly in the same way as CREATE > UNIQUE INDEX ... statement does. In Tarantool primary index is an index > with id == 0, so during execution of ADD CONSTRAINT PRIMARY KEY we check > that there is no any entries in _index space and create that one. > Otherwise, error is raised. > > Part of #3097 > Follow-up #3914 > --- > src/box/sql/build.c | 29 +++++++++++++++++++++-- > src/box/sql/parse.y | 15 ++++++++++++ > test/sql-tap/alter.test.lua | 58 ++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 99 insertions(+), 3 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 1d7b6c827..0314be957 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2083,6 +2083,19 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor) > return iid_reg; > } > > +static void > +pk_check_existence(struct Parse *parse, uint32_t space_id, int cursor) 1. It is worth mentioning in a comment that the cursor is opened on _index space. Or rename it to _index_cursor. It do not mind if in such a case you omit a comment. > +{ > + struct Vdbe *v = sqlite3GetVdbe(parse); > + int tmp_reg = ++parse->nMem; 2. Why not to use a truly temp register? As I know, we have facilities for it. Parse.nMem are not temporary. Parse.nTempReg - is. > + sqlite3VdbeAddOp2(v, OP_Integer, space_id, tmp_reg); > + int found_addr = sqlite3VdbeAddOp4Int(v, OP_NotFound, cursor, 0, > + tmp_reg, 1); > + sqlite3VdbeAddOp4(v, OP_Halt, SQLITE_ERROR, ON_CONFLICT_ACTION_FAIL, 0, > + "multiple primary keys are not allowed", P4_STATIC); > + sqlite3VdbeJumpHere(v, found_addr); > +} > + > /** > * Add new index to table's indexes list. > * We follow convention that PK comes first in list. > diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua > index 1aad555c0..925753749 100755 > --- a/test/sql-tap/alter.test.lua > +++ b/test/sql-tap/alter.test.lua > @@ -517,6 +517,62 @@ test:do_catchsql_test( > -- </alter-7.11> > }) > > +test:do_test( > + "alter-8.1.0", > + function() > + format = {} > + format[1] = { name = 'id', type = 'scalar'} > + format[2] = { name = 'f2', type = 'scalar'} > + s = box.schema.create_space('T', {format = format}) > + end, > + {}) > + > +test:do_catchsql_test( > + "alter-8.1.1", > + [[ > + ALTER TABLE t ADD CONSTRAINT pk PRIMARY KEY("id"); > + ]], { > + 0 > + }) > + > +test:do_test( > + "alter-8.1.2", > + function() > + i = box.space.T.index[0] > + return i.id 3. Why not return box.space.T.index[0].id? > + end, 0) > + > +test:do_catchsql_test( > + "alter-8.2", > + [[ > + ALTER TABLE t ADD CONSTRAINT pk1 PRIMARY KEY("f2"); > + ]], { > + 1, "multiple primary keys are not allowed" > + }) > + > +test:do_catchsql_test( > + "alter-8.3.1", > + [[ > + ALTER TABLE t ADD CONSTRAINT i1 UNIQUE("f2"); > + ]], { > + 0 > + }) > + > +test:do_test( > + "alter-8.3.2", > + function() > + i = box.space.T.index[1] > + return i.id > + end, 1) > + > +test:do_catchsql_test( > + "alter-8.4", > + [[ > + DROP INDEX i1 ON t; > + DROP INDEX pk ON t; > + ]], { > + 0 > +}) > > -- Commented due to #2953 > -- > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 6/6] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY 2019-01-14 14:06 ` [tarantool-patches] " Vladislav Shpilevoy @ 2019-01-16 20:06 ` n.pettik 2019-01-16 20:54 ` Vladislav Shpilevoy 0 siblings, 1 reply; 24+ messages in thread From: n.pettik @ 2019-01-16 20:06 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladislav Shpilevoy >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index 1d7b6c827..0314be957 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -2083,6 +2083,19 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor) >> return iid_reg; >> } >> +static void >> +pk_check_existence(struct Parse *parse, uint32_t space_id, int cursor) > > 1. It is worth mentioning in a comment that the cursor is opened on _index > space. Or rename it to _index_cursor. It do not mind if in such a case you > omit a comment. I’d prefer second option: diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 080914576..cdeb85bd7 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2089,12 +2089,12 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor) } static void -pk_check_existence(struct Parse *parse, uint32_t space_id, int cursor) +pk_check_existence(struct Parse *parse, uint32_t space_id, int _index_cursor) { struct Vdbe *v = sqlite3GetVdbe(parse); int tmp_reg = ++parse->nMem; sqlite3VdbeAddOp2(v, OP_Integer, space_id, tmp_reg); - int found_addr = sqlite3VdbeAddOp4Int(v, OP_NotFound, cursor, 0, + int found_addr = sqlite3VdbeAddOp4Int(v, OP_NotFound, _index_cursor, 0, > >> +{ >> + struct Vdbe *v = sqlite3GetVdbe(parse); >> + int tmp_reg = ++parse->nMem; > > 2. Why not to use a truly temp register? As I know, we have facilities for > it. Parse.nMem are not temporary. Parse.nTempReg - is. I am afraid that they don’t work. I greped through code a bit and found no real usages of tmp regs. What is more: int sqlite3GetTempReg(Parse * pParse) { if (pParse->nTempReg == 0) { return ++pParse->nMem; } return pParse->aTempReg[--pParse->nTempReg]; } It always returns real register since nTempReg == 0 and is never incremented. I’ve opened issue: https://github.com/tarantool/tarantool/issues/3942 https://github.com/tarantool/tarantool/issues/3943 >> + sqlite3VdbeAddOp2(v, OP_Integer, space_id, tmp_reg); >> + int found_addr = sqlite3VdbeAddOp4Int(v, OP_NotFound, cursor, 0, >> + tmp_reg, 1); >> + sqlite3VdbeAddOp4(v, OP_Halt, SQLITE_ERROR, ON_CONFLICT_ACTION_FAIL, 0, >> + "multiple primary keys are not allowed", P4_STATIC); >> + sqlite3VdbeJumpHere(v, found_addr); >> +} >> + >> /** >> * Add new index to table's indexes list. >> * We follow convention that PK comes first in list. >> diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua >> index 1aad555c0..925753749 100755 >> --- a/test/sql-tap/alter.test.lua >> +++ b/test/sql-tap/alter.test.lua >> @@ -517,6 +517,62 @@ test:do_catchsql_test( >> -- </alter-7.11> >> }) >> +test:do_test( >> + "alter-8.1.0", >> + function() >> + format = {} >> + format[1] = { name = 'id', type = 'scalar'} >> + format[2] = { name = 'f2', type = 'scalar'} >> + s = box.schema.create_space('T', {format = format}) >> + end, >> + {}) >> + >> +test:do_catchsql_test( >> + "alter-8.1.1", >> + [[ >> + ALTER TABLE t ADD CONSTRAINT pk PRIMARY KEY("id"); >> + ]], { >> + 0 >> + }) >> + >> +test:do_test( >> + "alter-8.1.2", >> + function() >> + i = box.space.T.index[0] >> + return i.id > > 3. Why not return box.space.T.index[0].id? … diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua index 925753749..318b0f68d 100755 --- a/test/sql-tap/alter.test.lua +++ b/test/sql-tap/alter.test.lua @@ -538,8 +538,7 @@ test:do_catchsql_test( test:do_test( "alter-8.1.2", function() - i = box.space.T.index[0] - return i.id + return box.space.T.index[0].id end, 0) ^ permalink raw reply [flat|nested] 24+ messages in thread
* [tarantool-patches] Re: [PATCH 6/6] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY 2019-01-16 20:06 ` n.pettik @ 2019-01-16 20:54 ` Vladislav Shpilevoy 0 siblings, 0 replies; 24+ messages in thread From: Vladislav Shpilevoy @ 2019-01-16 20:54 UTC (permalink / raw) To: n.pettik, tarantool-patches >>> + sqlite3VdbeAddOp2(v, OP_Integer, space_id, tmp_reg); >>> + int found_addr = sqlite3VdbeAddOp4Int(v, OP_NotFound, cursor, 0, >>> + tmp_reg, 1); >>> + sqlite3VdbeAddOp4(v, OP_Halt, SQLITE_ERROR, ON_CONFLICT_ACTION_FAIL, 0, >>> + "multiple primary keys are not allowed", P4_STATIC); >>> + sqlite3VdbeJumpHere(v, found_addr); >>> +} >>> + >>> /** >>> * Add new index to table's indexes list. >>> * We follow convention that PK comes first in list. >>> diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua >>> index 1aad555c0..925753749 100755 >>> --- a/test/sql-tap/alter.test.lua >>> +++ b/test/sql-tap/alter.test.lua >>> @@ -517,6 +517,62 @@ test:do_catchsql_test( >>> -- </alter-7.11> >>> }) >>> +test:do_test( >>> + "alter-8.1.0", >>> + function() >>> + format = {} >>> + format[1] = { name = 'id', type = 'scalar'} >>> + format[2] = { name = 'f2', type = 'scalar'} >>> + s = box.schema.create_space('T', {format = format}) >>> + end, >>> + {}) >>> + >>> +test:do_catchsql_test( >>> + "alter-8.1.1", >>> + [[ >>> + ALTER TABLE t ADD CONSTRAINT pk PRIMARY KEY("id"); >>> + ]], { >>> + 0 >>> + }) >>> + >>> +test:do_test( >>> + "alter-8.1.2", >>> + function() >>> + i = box.space.T.index[0] >>> + return i.id >> >> 3. Why not return box.space.T.index[0].id? > > … I know, nit, but I do not like excess useless lines. Cuts the eye, sorry. > > diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua > index 925753749..318b0f68d 100755 > --- a/test/sql-tap/alter.test.lua > +++ b/test/sql-tap/alter.test.lua > @@ -538,8 +538,7 @@ test:do_catchsql_test( > test:do_test( > "alter-8.1.2", > function() > - i = box.space.T.index[0] > - return i.id > + return box.space.T.index[0].id > end, 0) > > ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-01-18 1:42 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-09 12:13 [tarantool-patches] [PATCH 0/6] Introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PK Nikita Pettik 2019-01-09 12:13 ` [tarantool-patches] [PATCH 1/6] sql: move constraint name to struct contraint_parse Nikita Pettik 2019-01-14 14:04 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-16 20:06 ` n.pettik 2019-01-16 20:54 ` Vladislav Shpilevoy 2019-01-17 10:56 ` Konstantin Osipov 2019-01-17 17:14 ` n.pettik 2019-01-09 12:13 ` [tarantool-patches] [PATCH 2/6] sql: rework ALTER TABLE grammar Nikita Pettik 2019-01-14 14:05 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-16 20:06 ` n.pettik 2019-01-16 20:54 ` Vladislav Shpilevoy 2019-01-17 11:51 ` Konstantin Osipov 2019-01-17 17:14 ` n.pettik 2019-01-18 1:42 ` Konstantin Osipov 2019-01-09 12:13 ` [tarantool-patches] [PATCH 3/6] sql: remove start token from sql_create_index args Nikita Pettik 2019-01-09 12:13 ` [tarantool-patches] [PATCH 4/6] sql: refactor getNewIid() function Nikita Pettik 2019-01-14 14:05 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-09 12:13 ` [tarantool-patches] [PATCH 5/6] sql: fix error message for improperly created index Nikita Pettik 2019-01-14 14:06 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-16 20:06 ` n.pettik 2019-01-09 12:13 ` [tarantool-patches] [PATCH 6/6] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY Nikita Pettik 2019-01-14 14:06 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-16 20:06 ` n.pettik 2019-01-16 20:54 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox