* [Tarantool-patches] [PATCH] sql: support constraint drop
@ 2020-01-09 10:15 Roman Khabibov
2020-01-13 17:00 ` Nikita Pettik
2020-02-20 23:09 ` [Tarantool-patches] [PATCH] " Vladislav Shpilevoy
0 siblings, 2 replies; 13+ messages in thread
From: Roman Khabibov @ 2020-01-09 10:15 UTC (permalink / raw)
To: tarantool-patches
Extend <ALTER TABLE> statement to drop table constraints by their
names.
Closes #4120
@TarantoolBot document
Title: Drop table constraints in SQL
Now, it is possible to drop table constraints (PRIMARY KEY,
UNIQUE, FOREIGN KEY, CHECK) using
<ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
by their names.
For example:
tarantool> box.execute([[CREATE TABLE test (
a INTEGER PRIMARY KEY,
b INTEGER,
CONSTRAINT cnstr CHECK (a >= 0)
);]])
---
- row_count: 1
...
tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
---
- row_count: 1
...
The same for all the other constraints.
---
Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4120-drop-constraint
Issue: https://github.com/tarantool/tarantool/issues/4120
src/box/errcode.h | 2 +-
src/box/sql/alter.c | 2 +-
src/box/sql/build.c | 127 ++++++++++++++++++++---------------
src/box/sql/parse.y | 4 +-
src/box/sql/parse_def.h | 15 ++---
src/box/sql/sqlInt.h | 4 +-
test/sql-tap/alter2.test.lua | 2 +-
test/sql/checks.result | 2 +-
test/sql/constraint.result | 74 ++++++++++++++++++++
test/sql/constraint.test.lua | 37 ++++++++++
10 files changed, 197 insertions(+), 72 deletions(-)
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 3065b1948..9ba42dfb4 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -221,7 +221,7 @@ struct errcode_record {
/*166 */_(ER_NO_SUCH_COLLATION, "Collation '%s' does not exist") \
/*167 */_(ER_CREATE_FK_CONSTRAINT, "Failed to create foreign key constraint '%s': %s") \
/*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \
- /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \
+ /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint '%s' does not exist in space '%s'") \
/*170 */_(ER_CONSTRAINT_EXISTS, "%s constraint '%s' already exists in space '%s'") \
/*171 */_(ER_SQL_TYPE_MISMATCH, "Type mismatch: can not convert %s to %s") \
/*172 */_(ER_ROWID_OVERFLOW, "Rowid is overflowed: too many entries in ephemeral space") \
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 973b420cf..14f6c1a97 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -121,7 +121,7 @@ sql_alter_ck_constraint_enable(struct Parse *parse)
int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 2);
sqlVdbeAddOp4(v, OP_SetDiag, ER_NO_SUCH_CONSTRAINT, 0, 0,
sqlMPrintf(db, tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
- constraint_name), P4_DYNAMIC);
+ constraint_name, tbl_name), P4_DYNAMIC);
sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
sqlVdbeJumpHere(v, addr);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index bc50ecbfa..7c4b5760e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1469,6 +1469,29 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
sql_table_delete_from(parse, src_list, where);
}
+/**
+ * Generate VDBE program to remove entry with @a index_id and @a
+ * space_id from _index space.
+ */
+static void
+vdbe_emit_index_drop(struct Parse *parse_context, uint32_t index_id,
+ uint32_t space_id)
+{
+ struct Vdbe *v = sqlGetVdbe(parse_context);
+ assert(v != NULL);
+ int record_reg = ++parse_context->nMem;
+ int space_id_reg = ++parse_context->nMem;
+ int index_id_reg = ++parse_context->nMem;
+ sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
+ sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
+ sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
+ sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
+ if (index_id == 0)
+ VdbeComment((v, "Remove primary key", index_id));
+ else
+ VdbeComment((v, "Remove secondary index iid = %u", index_id));
+}
+
/**
* Generate VDBE program to remove entry from _fk_constraint space.
*
@@ -1488,17 +1511,6 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
P4_DYNAMIC);
sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1);
- const char *error_msg =
- tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
- constraint_name);
- if (vdbe_emit_halt_with_presence_test(parse_context,
- BOX_FK_CONSTRAINT_ID, 0,
- key_reg, 2, ER_NO_SUCH_CONSTRAINT,
- error_msg, false,
- OP_Found) != 0) {
- sqlDbFree(parse_context->db, constraint_name);
- return;
- }
sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
@@ -1523,12 +1535,6 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
sqlDbStrDup(db, ck_name), P4_DYNAMIC);
- const char *error_msg =
- tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
- if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
- key_reg, 2, ER_NO_SUCH_CONSTRAINT,
- error_msg, false, OP_Found) != 0)
- return;
sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2);
sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2);
VdbeComment((v, "Delete CK constraint %s", ck_name));
@@ -1617,7 +1623,6 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
*/
int idx_rec_reg = ++parse_context->nMem;
int space_id_reg = ++parse_context->nMem;
- int index_id_reg = ++parse_context->nMem;
int space_id = space->def->id;
sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
@@ -1680,23 +1685,12 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
* secondary exist.
*/
for (uint32_t i = 1; i < index_count; ++i) {
- sqlVdbeAddOp2(v, OP_Integer,
- space->index[i]->def->iid,
- index_id_reg);
- sqlVdbeAddOp3(v, OP_MakeRecord,
- space_id_reg, 2, idx_rec_reg);
- sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID,
- idx_rec_reg);
- VdbeComment((v,
- "Remove secondary index iid = %u",
- space->index[i]->def->iid));
+ vdbe_emit_index_drop(parse_context,
+ space->index[i]->def->iid,
+ space_id);
}
}
- sqlVdbeAddOp2(v, OP_Integer, 0, index_id_reg);
- sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2,
- idx_rec_reg);
- sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, idx_rec_reg);
- VdbeComment((v, "Remove primary index"));
+ vdbe_emit_index_drop(parse_context, 0, space_id);
}
/* Delete records about the space from the _truncate. */
sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, idx_rec_reg);
@@ -2050,28 +2044,61 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
is_deferred;
}
+/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ */
void
-sql_drop_foreign_key(struct Parse *parse_context)
+sql_drop_constraint(struct Parse *parse_context)
{
- struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
- assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
+ struct drop_entity_def *drop_def =
+ &parse_context->drop_constraint_def.base;
+ assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
const char *table_name = drop_def->base.entity_name->a[0].zName;
assert(table_name != NULL);
- struct space *child = space_by_name(table_name);
- if (child == NULL) {
+ struct space *space = space_by_name(table_name);
+ if (space == NULL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
parse_context->is_aborted = true;
return;
}
- char *constraint_name =
- sql_name_from_token(parse_context->db, &drop_def->name);
- if (constraint_name == NULL) {
+ char *name = sql_name_from_token(parse_context->db, &drop_def->name);
+ if (name == NULL) {
+ parse_context->is_aborted = true;
+ return;
+ }
+ struct constraint_id *id = space_find_constraint_id(space, name);
+ if (id == NULL) {
+ diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
parse_context->is_aborted = true;
return;
}
- vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
- child->def->id);
+ uint32_t space_id = space->def->id;
+ assert(id->type == CONSTRAINT_TYPE_PK || id->type ==
+ CONSTRAINT_TYPE_UNIQUE || id->type == CONSTRAINT_TYPE_FK ||
+ id->type == CONSTRAINT_TYPE_CK);
+ switch (id->type) {
+ case CONSTRAINT_TYPE_PK:
+ case CONSTRAINT_TYPE_UNIQUE: {
+ uint32_t index_id = box_index_id_by_name(space_id, name,
+ strlen(name));
+ /*
+ * We have already sure, that this index exists,
+ * so we don't check index_id for BOX_ID_NIL.
+ */
+ vdbe_emit_index_drop(parse_context, index_id, space_id);
+ break;
+ }
+ case CONSTRAINT_TYPE_FK:
+ vdbe_emit_fk_constraint_drop(parse_context, name, space_id);
+ break;
+ case CONSTRAINT_TYPE_CK:
+ vdbe_emit_ck_constraint_drop(parse_context, name, space_id);
+ break;
+ default:
+ unreachable();
+ }
/*
* We account changes to row count only if drop of
* foreign keys take place in a separate
@@ -2704,18 +2731,8 @@ sql_drop_index(struct Parse *parse_context)
goto exit_drop_index;
}
- /*
- * Generate code to remove entry from _index space
- * But firstly, delete statistics since schema
- * changes after DDL.
- */
- int record_reg = ++parse_context->nMem;
- int space_id_reg = ++parse_context->nMem;
- int index_id_reg = ++parse_context->nMem;
- sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
- sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
- sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
- sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
+ vdbe_emit_index_drop(parse_context, index_id, space->def->id);
+ /* Delete statistics since schema changes after DDL. */
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
exit_drop_index:
sqlSrcListDelete(db, table_list);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index cfe1c0012..1a0e89703 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1763,9 +1763,9 @@ cmd ::= alter_table_start(A) RENAME TO nm(N). {
}
cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
- drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
+ drop_constraint_def_init(&pParse->drop_constraint_def, X, &Z, false);
pParse->initiateTTrans = true;
- sql_drop_foreign_key(pParse);
+ sql_drop_constraint(pParse);
}
cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). {
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 2f433e4c0..e6d45b256 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -159,10 +159,6 @@ enum entity_type {
ENTITY_TYPE_TRIGGER,
ENTITY_TYPE_CK,
ENTITY_TYPE_FK,
- /**
- * For assertion checks that constraint definition is
- * created before initialization of a term constraint.
- */
ENTITY_TYPE_CONSTRAINT,
};
@@ -265,7 +261,7 @@ struct drop_trigger_def {
struct drop_entity_def base;
};
-struct drop_fk_def {
+struct drop_constraint_def {
struct drop_entity_def base;
};
@@ -408,11 +404,12 @@ drop_trigger_def_init(struct drop_trigger_def *drop_trigger_def,
}
static inline void
-drop_fk_def_init(struct drop_fk_def *drop_fk_def, struct SrcList *parent_name,
- struct Token *name, bool if_exist)
+drop_constraint_def_init(struct drop_constraint_def *drop_constraint_def,
+ struct SrcList *parent_name, struct Token *name,
+ bool if_exist)
{
- drop_entity_def_init(&drop_fk_def->base, parent_name, name, if_exist,
- ENTITY_TYPE_FK);
+ drop_entity_def_init(&drop_constraint_def->base, parent_name, name,
+ if_exist, ENTITY_TYPE_CONSTRAINT);
}
static inline void
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d1fcf4761..2ea653c7d 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2237,7 +2237,7 @@ struct Parse {
struct create_trigger_def create_trigger_def;
struct create_view_def create_view_def;
struct rename_entity_def rename_entity_def;
- struct drop_fk_def drop_fk_def;
+ struct drop_constraint_def drop_constraint_def;
struct drop_index_def drop_index_def;
struct drop_table_def drop_table_def;
struct drop_trigger_def drop_trigger_def;
@@ -3747,7 +3747,7 @@ sql_create_foreign_key(struct Parse *parse_context);
* @param parse_context Parsing context.
*/
void
-sql_drop_foreign_key(struct Parse *parse_context);
+sql_drop_constraint(struct Parse *parse_context);
/**
* Now our SQL implementation can't operate on spaces which
diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
index 3e21a5f40..759acc9c3 100755
--- a/test/sql-tap/alter2.test.lua
+++ b/test/sql-tap/alter2.test.lua
@@ -256,7 +256,7 @@ test:do_catchsql_test(
ALTER TABLE child DROP CONSTRAINT fake;
]], {
-- <alter2-5.2>
- 1, "Constraint FAKE does not exist"
+ 1, "Constraint 'FAKE' does not exist in space 'CHILD'"
-- </alter2-5.2>
})
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 4ae0b4c10..7b18e5d6b 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -801,7 +801,7 @@ box.execute('ALTER TABLE test ADD CONSTRAINT \"some_ck\" CHECK(a < 10);')
box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"falsch\"")
---
- null
-- Constraint falsch does not exist
+- Constraint 'falsch' does not exist in space 'TEST'
...
box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"some_ck\"")
---
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
index 1585c2327..73bdec45a 100644
--- a/test/sql/constraint.result
+++ b/test/sql/constraint.result
@@ -291,6 +291,80 @@ box.execute('CREATE UNIQUE INDEX d ON t2(i);')
| - Index 'D' already exists in space 'T2'
| ...
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop PRIMARY KEY constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+ | ---
+ | - null
+ | - Can't drop primary key in space 'T2' while secondary keys exist
+ | ...
+box.space.T2.index.C ~= nil
+ | ---
+ | - true
+ | ...
+-- Drop UNIQUE constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.E ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+-- Check the error message.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - null
+ | - Constraint 'E' does not exist in space 'T2'
+ | ...
+
+--
+-- Ensure that constraint name dropped from/added into the
+-- internal space hash table during a transaction.
+--
+box.begin() \
+box.execute('CREATE UNIQUE INDEX e ON t2(i);') \
+-- Drop UNIQUE constraint. \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+-- Drop CHECK constraint. \
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+-- Drop FOREIGN KEY constraint. \
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);') \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+box.commit()
+ | ---
+ | ...
+
--
-- Cleanup.
--
diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
index 163f4309c..0cada5f09 100755
--- a/test/sql/constraint.test.lua
+++ b/test/sql/constraint.test.lua
@@ -131,6 +131,43 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
-- uniqueness, index names should be unique in a space.
box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop PRIMARY KEY constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+box.space.T2.index.C ~= nil
+-- Drop UNIQUE constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.index.E == nil
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+box.space.T2.ck_constraint.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.ck_constraint.E == nil
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+-- Check the error message.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+
+--
+-- Ensure that constraint name dropped from/added into the
+-- internal space hash table during a transaction.
+--
+box.begin() \
+box.execute('CREATE UNIQUE INDEX e ON t2(i);') \
+-- Drop UNIQUE constraint. \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+-- Drop CHECK constraint. \
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+-- Drop FOREIGN KEY constraint. \
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);') \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+box.commit()
+
--
-- Cleanup.
--
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: support constraint drop
2020-01-09 10:15 [Tarantool-patches] [PATCH] sql: support constraint drop Roman Khabibov
@ 2020-01-13 17:00 ` Nikita Pettik
2020-01-24 14:21 ` [Tarantool-patches] [PATCH 2/2] " Roman Khabibov
2020-02-20 23:09 ` [Tarantool-patches] [PATCH] " Vladislav Shpilevoy
1 sibling, 1 reply; 13+ messages in thread
From: Nikita Pettik @ 2020-01-13 17:00 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches
On 09 Jan 13:15, Roman Khabibov wrote:
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index 3065b1948..9ba42dfb4 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -221,7 +221,7 @@ struct errcode_record {
> /*166 */_(ER_NO_SUCH_COLLATION, "Collation '%s' does not exist") \
> /*167 */_(ER_CREATE_FK_CONSTRAINT, "Failed to create foreign key constraint '%s': %s") \
> /*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \
> - /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \
> + /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint '%s' does not exist in space '%s'") \
Why did you change error (in scope of this patch)?
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index bc50ecbfa..7c4b5760e 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1469,6 +1469,29 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
> sql_table_delete_from(parse, src_list, where);
> }
>
> +/**
> + * Generate VDBE program to remove entry with @a index_id and @a
> + * space_id from _index space.
> + */
> +static void
> +vdbe_emit_index_drop(struct Parse *parse_context, uint32_t index_id,
> + uint32_t space_id)
> +{
Instead of passing values of space/index ids, I'd rather pass registers
holding these values. In most cases it allows to avoid duplicating
the same intstruction (which stores id in the given register).
> + struct Vdbe *v = sqlGetVdbe(parse_context);
> + assert(v != NULL);
> + int record_reg = ++parse_context->nMem;
> + int space_id_reg = ++parse_context->nMem;
> + int index_id_reg = ++parse_context->nMem;
> + sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
> + sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
> + sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
> + sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
> + if (index_id == 0)
> + VdbeComment((v, "Remove primary key", index_id));
> + else
> + VdbeComment((v, "Remove secondary index iid = %u", index_id));
Simply without branching: "Remove index with iid = %u"
> +}
> +
> /**
> * Generate VDBE program to remove entry from _fk_constraint space.
> *
> @@ -1488,17 +1511,6 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
> sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
> P4_DYNAMIC);
> sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1);
> - const char *error_msg =
> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
> - constraint_name);
> - if (vdbe_emit_halt_with_presence_test(parse_context,
> - BOX_FK_CONSTRAINT_ID, 0,
> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
> - error_msg, false,
> - OP_Found) != 0) {
> - sqlDbFree(parse_context->db, constraint_name);
> - return;
> - }
Why did you drop this check?
> sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
> sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
> VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
> @@ -1523,12 +1535,6 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
> sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
> sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
> sqlDbStrDup(db, ck_name), P4_DYNAMIC);
> - const char *error_msg =
> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
> - if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
> - error_msg, false, OP_Found) != 0)
> - return;
Same question.
> sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2);
> sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2);
> VdbeComment((v, "Delete CK constraint %s", ck_name));
> @@ -1617,7 +1623,6 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
> */
> int idx_rec_reg = ++parse_context->nMem;
> int space_id_reg = ++parse_context->nMem;
> - int index_id_reg = ++parse_context->nMem;
> int space_id = space->def->id;
> sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
> sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
> @@ -1680,23 +1685,12 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
> * secondary exist.
> */
> for (uint32_t i = 1; i < index_count; ++i) {
> - sqlVdbeAddOp2(v, OP_Integer,
> - space->index[i]->def->iid,
> - index_id_reg);
> - sqlVdbeAddOp3(v, OP_MakeRecord,
> - space_id_reg, 2, idx_rec_reg);
> - sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID,
> - idx_rec_reg);
> - VdbeComment((v,
> - "Remove secondary index iid = %u",
> - space->index[i]->def->iid));
> + vdbe_emit_index_drop(parse_context,
> + space->index[i]->def->iid,
> + space_id);
I'd better move this refactoring to a separate patch.
> }
> }
> - sqlVdbeAddOp2(v, OP_Integer, 0, index_id_reg);
> - sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2,
> - idx_rec_reg);
> - sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, idx_rec_reg);
> - VdbeComment((v, "Remove primary index"));
> + vdbe_emit_index_drop(parse_context, 0, space_id);
> }
> /* Delete records about the space from the _truncate. */
> sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, idx_rec_reg);
> @@ -2050,28 +2044,61 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
> is_deferred;
> }
>
> +/**
> + * Emit code to drop the entry from _index or _ck_contstraint or
> + * _fk_constraint space corresponding with the constraint type.
> + */
> void
> -sql_drop_foreign_key(struct Parse *parse_context)
> +sql_drop_constraint(struct Parse *parse_context)
> {
> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
> + struct drop_entity_def *drop_def =
> + &parse_context->drop_constraint_def.base;
> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
> const char *table_name = drop_def->base.entity_name->a[0].zName;
> assert(table_name != NULL);
> - struct space *child = space_by_name(table_name);
> - if (child == NULL) {
> + struct space *space = space_by_name(table_name);
> + if (space == NULL) {
> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
> parse_context->is_aborted = true;
> return;
> }
> - char *constraint_name =
> - sql_name_from_token(parse_context->db, &drop_def->name);
> - if (constraint_name == NULL) {
> + char *name = sql_name_from_token(parse_context->db, &drop_def->name);
> + if (name == NULL) {
> + parse_context->is_aborted = true;
> + return;
> + }
> + struct constraint_id *id = space_find_constraint_id(space, name);
> + if (id == NULL) {
> + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
> parse_context->is_aborted = true;
> return;
> }
> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
> - child->def->id);
> + uint32_t space_id = space->def->id;
> + assert(id->type == CONSTRAINT_TYPE_PK || id->type ==
> + CONSTRAINT_TYPE_UNIQUE || id->type == CONSTRAINT_TYPE_FK ||
> + id->type == CONSTRAINT_TYPE_CK);
Instead of such huge checks iterating over all possible variants,
enum can be extended with _MAX last element (see enum index_type for
example) and check becomes quite simple: assert(id->type < contraint_type_MAX);
> + switch (id->type) {
> + case CONSTRAINT_TYPE_PK:
> + case CONSTRAINT_TYPE_UNIQUE: {
> + uint32_t index_id = box_index_id_by_name(space_id, name,
> + strlen(name));
> + /*
> + * We have already sure, that this index exists,
> + * so we don't check index_id for BOX_ID_NIL.
> + */
Then add assert(index_id != BOX_ID_NIL);
> + vdbe_emit_index_drop(parse_context, index_id, space_id);
> + break;
> + }
> + case CONSTRAINT_TYPE_FK:
> + vdbe_emit_fk_constraint_drop(parse_context, name, space_id);
> + break;
> + case CONSTRAINT_TYPE_CK:
> + vdbe_emit_ck_constraint_drop(parse_context, name, space_id);
> + break;
> + default:
> + unreachable();
> + }
> /*
> * We account changes to row count only if drop of
> * foreign keys take place in a separate
> @@ -2704,18 +2731,8 @@ sql_drop_index(struct Parse *parse_context)
> goto exit_drop_index;
> }
>
> - /*
> - * Generate code to remove entry from _index space
> - * But firstly, delete statistics since schema
> - * changes after DDL.
> - */
> - int record_reg = ++parse_context->nMem;
> - int space_id_reg = ++parse_context->nMem;
> - int index_id_reg = ++parse_context->nMem;
> - sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
> - sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
> - sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
> - sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
> + vdbe_emit_index_drop(parse_context, index_id, space->def->id);
> + /* Delete statistics since schema changes after DDL. */
Looks like obsolete comment.
> diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
> index 163f4309c..0cada5f09 100755
> --- a/test/sql/constraint.test.lua
> +++ b/test/sql/constraint.test.lua
> @@ -131,6 +131,43 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
> -- uniqueness, index names should be unique in a space.
> box.execute('CREATE UNIQUE INDEX d ON t2(i);')
>
> +--
> +-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
> +-- correctly for each type of constraint.
> +--
> +-- Drop PRIMARY KEY constraint.
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
> +box.space.T2.index.C ~= nil
But PK is not dropped. Please, add tests where PK will be successfully
dropped.
> +-- Drop UNIQUE constraint.
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +box.space.T2.index.E == nil
> +-- Drop CHECK constraint.
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
> +box.space.T2.ck_constraint.E ~= nil
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +box.space.T2.ck_constraint.E == nil
> +-- Drop FOREIGN KEY constraint.
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +-- Check the error message.
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +
> +--
> +-- Ensure that constraint name dropped from/added into the
> +-- internal space hash table during a transaction.
> +--
This patch is about extending SQL syntax, don't get how this check
is related to subj..
> +box.begin() \
> +box.execute('CREATE UNIQUE INDEX e ON t2(i);') \
> +-- Drop UNIQUE constraint. \
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
> +-- Drop CHECK constraint. \
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') \
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
> +-- Drop FOREIGN KEY constraint. \
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);') \
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
> +box.commit()
> +
> --
> -- Cleanup.
> --
> --
> 2.21.0 (Apple Git-122)
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] sql: support constraint drop
2020-01-13 17:00 ` Nikita Pettik
@ 2020-01-24 14:21 ` Roman Khabibov
2020-01-28 17:39 ` Nikita Pettik
0 siblings, 1 reply; 13+ messages in thread
From: Roman Khabibov @ 2020-01-24 14:21 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
Hi! Thanks for the review.
> On Jan 13, 2020, at 20:00, Nikita Pettik <korablev@tarantool.org> wrote:
>
> On 09 Jan 13:15, Roman Khabibov wrote:
>> diff --git a/src/box/errcode.h b/src/box/errcode.h
>> index 3065b1948..9ba42dfb4 100644
>> --- a/src/box/errcode.h
>> +++ b/src/box/errcode.h
>> @@ -221,7 +221,7 @@ struct errcode_record {
>> /*166 */_(ER_NO_SUCH_COLLATION, "Collation '%s' does not exist") \
>> /*167 */_(ER_CREATE_FK_CONSTRAINT, "Failed to create foreign key constraint '%s': %s") \
>> /*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \
>> - /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \
>> + /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint '%s' does not exist in space '%s'") \
>
> Why did you change error (in scope of this patch)?
In the previous patch about constraint_id, I did about the same with ER_CONSTRAINT_EXISTS.
Now I decided that changing the error also applies to this patch and it is too small to be
a separate patch.
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index bc50ecbfa..7c4b5760e 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -1469,6 +1469,29 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
>> sql_table_delete_from(parse, src_list, where);
>> }
>>
>> +/**
>> + * Generate VDBE program to remove entry with @a index_id and @a
>> + * space_id from _index space.
>> + */
>> +static void
>> +vdbe_emit_index_drop(struct Parse *parse_context, uint32_t index_id,
>> + uint32_t space_id)
>> +{
>
> Instead of passing values of space/index ids, I'd rather pass registers
> holding these values. In most cases it allows to avoid duplicating
> the same intstruction (which stores id in the given register).
Done. In the separate patch.
>> + struct Vdbe *v = sqlGetVdbe(parse_context);
>> + assert(v != NULL);
>> + int record_reg = ++parse_context->nMem;
>> + int space_id_reg = ++parse_context->nMem;
>> + int index_id_reg = ++parse_context->nMem;
>> + sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
>> + sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
>> + sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
>> + sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
>> + if (index_id == 0)
>> + VdbeComment((v, "Remove primary key", index_id));
>> + else
>> + VdbeComment((v, "Remove secondary index iid = %u", index_id));
>
> Simply without branching: "Remove index with iid = %u”
Done.
>> +}
>> +
>> /**
>> * Generate VDBE program to remove entry from _fk_constraint space.
>> *
>> @@ -1488,17 +1511,6 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
>> sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
>> P4_DYNAMIC);
>> sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1);
>> - const char *error_msg =
>> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
>> - constraint_name);
>> - if (vdbe_emit_halt_with_presence_test(parse_context,
>> - BOX_FK_CONSTRAINT_ID, 0,
>> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
>> - error_msg, false,
>> - OP_Found) != 0) {
>> - sqlDbFree(parse_context->db, constraint_name);
>> - return;
>> - }
>
> Why did you drop this check?
>
>> sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
>> sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
>> VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
>> @@ -1523,12 +1535,6 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
>> sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
>> sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
>> sqlDbStrDup(db, ck_name), P4_DYNAMIC);
>> - const char *error_msg =
>> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
>> - if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
>> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
>> - error_msg, false, OP_Found) != 0)
>> - return;
>
> Same question.
I dropped these checks, because I thought that the data are consistent. These two
functions are called when:
1) DROP TABLE. If the constraint exist in struct space, then the corresponding tuple
exists in _ck/_fk system space too. Therefore this error can not occur in box.
2) ALTER TABLE DROP CONSTRAINT. Analogically, I check the constraint for existence in
sql_drop_constraint() and throw error on parsing level.
>> sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2);
>> sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2);
>> VdbeComment((v, "Delete CK constraint %s", ck_name));
>> @@ -1617,7 +1623,6 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
>> */
>> int idx_rec_reg = ++parse_context->nMem;
>> int space_id_reg = ++parse_context->nMem;
>> - int index_id_reg = ++parse_context->nMem;
>> int space_id = space->def->id;
>> sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
>> sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
>> @@ -1680,23 +1685,12 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
>> * secondary exist.
>> */
>> for (uint32_t i = 1; i < index_count; ++i) {
>> - sqlVdbeAddOp2(v, OP_Integer,
>> - space->index[i]->def->iid,
>> - index_id_reg);
>> - sqlVdbeAddOp3(v, OP_MakeRecord,
>> - space_id_reg, 2, idx_rec_reg);
>> - sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID,
>> - idx_rec_reg);
>> - VdbeComment((v,
>> - "Remove secondary index iid = %u",
>> - space->index[i]->def->iid));
>> + vdbe_emit_index_drop(parse_context,
>> + space->index[i]->def->iid,
>> + space_id);
>
> I'd better move this refactoring to a separate patch.
Done.
>> }
>> }
>> - sqlVdbeAddOp2(v, OP_Integer, 0, index_id_reg);
>> - sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2,
>> - idx_rec_reg);
>> - sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, idx_rec_reg);
>> - VdbeComment((v, "Remove primary index"));
>> + vdbe_emit_index_drop(parse_context, 0, space_id);
>> }
>> /* Delete records about the space from the _truncate. */
>> sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, idx_rec_reg);
>> @@ -2050,28 +2044,61 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
>> is_deferred;
>> }
>>
>> +/**
>> + * Emit code to drop the entry from _index or _ck_contstraint or
>> + * _fk_constraint space corresponding with the constraint type.
>> + */
>> void
>> -sql_drop_foreign_key(struct Parse *parse_context)
>> +sql_drop_constraint(struct Parse *parse_context)
>> {
>> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
>> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
>> + struct drop_entity_def *drop_def =
>> + &parse_context->drop_constraint_def.base;
>> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
>> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
>> const char *table_name = drop_def->base.entity_name->a[0].zName;
>> assert(table_name != NULL);
>> - struct space *child = space_by_name(table_name);
>> - if (child == NULL) {
>> + struct space *space = space_by_name(table_name);
>> + if (space == NULL) {
>> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
>> parse_context->is_aborted = true;
>> return;
>> }
>> - char *constraint_name =
>> - sql_name_from_token(parse_context->db, &drop_def->name);
>> - if (constraint_name == NULL) {
>> + char *name = sql_name_from_token(parse_context->db, &drop_def->name);
>> + if (name == NULL) {
>> + parse_context->is_aborted = true;
>> + return;
>> + }
>> + struct constraint_id *id = space_find_constraint_id(space, name);
>> + if (id == NULL) {
>> + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
>> parse_context->is_aborted = true;
>> return;
>> }
>> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
>> - child->def->id);
>> + uint32_t space_id = space->def->id;
>> + assert(id->type == CONSTRAINT_TYPE_PK || id->type ==
>> + CONSTRAINT_TYPE_UNIQUE || id->type == CONSTRAINT_TYPE_FK ||
>> + id->type == CONSTRAINT_TYPE_CK);
>
> Instead of such huge checks iterating over all possible variants,
> enum can be extended with _MAX last element (see enum index_type for
> example) and check becomes quite simple: assert(id->type < contraint_type_MAX);
Ok, but I thought that this assert is needed to prevent the following situation.
For example, I have added new value to enum constraint_type, and I forgot to
extend this switch-case construction. Then I will get unreachable().
>> + switch (id->type) {
>> + case CONSTRAINT_TYPE_PK:
>> + case CONSTRAINT_TYPE_UNIQUE: {
>> + uint32_t index_id = box_index_id_by_name(space_id, name,
>> + strlen(name));
>> + /*
>> + * We have already sure, that this index exists,
>> + * so we don't check index_id for BOX_ID_NIL.
>> + */
>
> Then add assert(index_id != BOX_ID_NIL);
>> + vdbe_emit_index_drop(parse_context, index_id, space_id);
>> + break;
>> + }
>> + case CONSTRAINT_TYPE_FK:
>> + vdbe_emit_fk_constraint_drop(parse_context, name, space_id);
>> + break;
>> + case CONSTRAINT_TYPE_CK:
>> + vdbe_emit_ck_constraint_drop(parse_context, name, space_id);
>> + break;
>> + default:
>> + unreachable();
>> + }
>> /*
>> * We account changes to row count only if drop of
>> * foreign keys take place in a separate
+/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ */
void
-sql_drop_foreign_key(struct Parse *parse_context)
+sql_drop_constraint(struct Parse *parse)
{
- struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
- assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
+ struct drop_entity_def *drop_def = &parse->drop_constraint_def.base;
+ assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
const char *table_name = drop_def->base.entity_name->a[0].zName;
assert(table_name != NULL);
- struct space *child = space_by_name(table_name);
- if (child == NULL) {
+ struct space *space = space_by_name(table_name);
+ if (space == NULL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
- parse_context->is_aborted = true;
+ parse->is_aborted = true;
return;
}
- char *constraint_name =
- sql_name_from_token(parse_context->db, &drop_def->name);
- if (constraint_name == NULL) {
- parse_context->is_aborted = true;
+ char *name = sql_name_from_token(parse->db, &drop_def->name);
+ if (name == NULL) {
+ parse->is_aborted = true;
return;
}
- vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
- child->def->id);
+ struct constraint_id *id = space_find_constraint_id(space, name);
+ if (id == NULL) {
+ diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
+ parse->is_aborted = true;
+ return;
+ }
+ uint32_t space_id = space->def->id;
+ assert(id->type < constraint_type_MAX);
+ switch (id->type) {
+ case CONSTRAINT_TYPE_PK:
+ case CONSTRAINT_TYPE_UNIQUE: {
+ uint32_t index_id = box_index_id_by_name(space_id, name,
+ strlen(name));
+ /*
+ * We have already sure, that this index exists,
+ * so we don't check index_id for BOX_ID_NIL.
+ */
+ assert(index_id != BOX_ID_NIL);
+ int space_id_reg = ++parse->nMem;
+ int index_id_reg = ++parse->nMem;
+ struct Vdbe *v = sqlGetVdbe(parse);
+ assert(v != NULL);
+ sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
+ vdbe_emit_index_drop(parse, index_id, index_id_reg,
+ space_id_reg);
+ break;
+ }
+ case CONSTRAINT_TYPE_FK:
+ vdbe_emit_fk_constraint_drop(parse, name, space_id);
+ break;
+ case CONSTRAINT_TYPE_CK:
+ vdbe_emit_ck_constraint_drop(parse, name, space_id);
+ break;
+ default:
+ unreachable();
+ }
/*
* We account changes to row count only if drop of
- * foreign keys take place in a separate
- * ALTER TABLE DROP CONSTRAINT statement, since whole
- * DROP TABLE always returns 1 (one) as a row count.
+ * constraints take place in a separate ALTER TABLE DROP
+ * CONSTRAINT statement, since whole DROP TABLE always
+ * returns 1 (one) as a row count.
*/
- struct Vdbe *v = sqlGetVdbe(parse_context);
+ struct Vdbe *v = sqlGetVdbe(parse);
sqlVdbeCountChanges(v);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
}
>> @@ -2704,18 +2731,8 @@ sql_drop_index(struct Parse *parse_context)
>> goto exit_drop_index;
>> }
>>
>> - /*
>> - * Generate code to remove entry from _index space
>> - * But firstly, delete statistics since schema
>> - * changes after DDL.
>> - */
>> - int record_reg = ++parse_context->nMem;
>> - int space_id_reg = ++parse_context->nMem;
>> - int index_id_reg = ++parse_context->nMem;
>> - sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
>> - sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
>> - sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
>> - sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
>> + vdbe_emit_index_drop(parse_context, index_id, space->def->id);
>> + /* Delete statistics since schema changes after DDL. */
>
> Looks like obsolete comment.
Removed.
>> diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
>> index 163f4309c..0cada5f09 100755
>> --- a/test/sql/constraint.test.lua
>> +++ b/test/sql/constraint.test.lua
>> @@ -131,6 +131,43 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
>> -- uniqueness, index names should be unique in a space.
>> box.execute('CREATE UNIQUE INDEX d ON t2(i);')
>>
>> +--
>> +-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
>> +-- correctly for each type of constraint.
>> +--
>> +-- Drop PRIMARY KEY constraint.
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
>> +box.space.T2.index.C ~= nil
>
> But PK is not dropped. Please, add tests where PK will be successfully
> dropped.
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.index.E == nil
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX d ON t2;')
+box.space.T2.index.C ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+box.space.T2.index.C == nil
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+box.space.T2.ck_constraint.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.ck_constraint.E == nil
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+-- Check the error message.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+
>> +-- Drop UNIQUE constraint.
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
>> +box.space.T2.index.E == nil
>> +-- Drop CHECK constraint.
>> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
>> +box.space.T2.ck_constraint.E ~= nil
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
>> +box.space.T2.ck_constraint.E == nil
>> +-- Drop FOREIGN KEY constraint.
>> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
>> +-- Check the error message.
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
>> +
>> +--
>> +-- Ensure that constraint name dropped from/added into the
>> +-- internal space hash table during a transaction.
>> +--
>
> This patch is about extending SQL syntax, don't get how this check
> is related to subj..
I mean to check that this operation is transactional. Removed.
commit 2220d8f4e09421e88d05a9d5265f6aca0fd30916
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date: Thu Jan 2 20:13:36 2020 +0300
sql: support constraint drop
Extend <ALTER TABLE> statement to drop table constraints by their
names.
Closes #4120
@TarantoolBot document
Title: Drop table constraints in SQL
Now, it is possible to drop table constraints (PRIMARY KEY,
UNIQUE, FOREIGN KEY, CHECK) using
<ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
by their names.
For example:
tarantool> box.execute([[CREATE TABLE test (
a INTEGER PRIMARY KEY,
b INTEGER,
CONSTRAINT cnstr CHECK (a >= 0)
);]])
---
- row_count: 1
...
tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
---
- row_count: 1
...
The same for all the other constraints.
diff --git a/src/box/constraint_id.h b/src/box/constraint_id.h
index 21f067cdc..ed4f37426 100755
--- a/src/box/constraint_id.h
+++ b/src/box/constraint_id.h
@@ -40,6 +40,7 @@ enum constraint_type {
CONSTRAINT_TYPE_UNIQUE,
CONSTRAINT_TYPE_FK,
CONSTRAINT_TYPE_CK,
+ constraint_type_MAX,
};
extern const char *constraint_type_strs[];
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 6f6e28c6c..b7787d378 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -221,7 +221,7 @@ struct errcode_record {
/*166 */_(ER_NO_SUCH_COLLATION, "Collation '%s' does not exist") \
/*167 */_(ER_CREATE_FK_CONSTRAINT, "Failed to create foreign key constraint '%s': %s") \
/*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \
- /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \
+ /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint '%s' does not exist in space '%s'") \
/*170 */_(ER_CONSTRAINT_EXISTS, "%s constraint '%s' already exists in space '%s'") \
/*171 */_(ER_SQL_TYPE_MISMATCH, "Type mismatch: can not convert %s to %s") \
/*172 */_(ER_ROWID_OVERFLOW, "Rowid is overflowed: too many entries in ephemeral space") \
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 973b420cf..14f6c1a97 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -121,7 +121,7 @@ sql_alter_ck_constraint_enable(struct Parse *parse)
int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 2);
sqlVdbeAddOp4(v, OP_SetDiag, ER_NO_SUCH_CONSTRAINT, 0, 0,
sqlMPrintf(db, tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
- constraint_name), P4_DYNAMIC);
+ constraint_name, tbl_name), P4_DYNAMIC);
sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
sqlVdbeJumpHere(v, addr);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 58a6a8a6b..1a695efa4 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1508,17 +1508,6 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
P4_DYNAMIC);
sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1);
- const char *error_msg =
- tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
- constraint_name);
- if (vdbe_emit_halt_with_presence_test(parse_context,
- BOX_FK_CONSTRAINT_ID, 0,
- key_reg, 2, ER_NO_SUCH_CONSTRAINT,
- error_msg, false,
- OP_Found) != 0) {
- sqlDbFree(parse_context->db, constraint_name);
- return;
- }
sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
@@ -1543,12 +1532,6 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
sqlDbStrDup(db, ck_name), P4_DYNAMIC);
- const char *error_msg =
- tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
- if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
- key_reg, 2, ER_NO_SUCH_CONSTRAINT,
- error_msg, false, OP_Found) != 0)
- return;
sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2);
sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2);
VdbeComment((v, "Delete CK constraint %s", ck_name));
@@ -2061,35 +2044,72 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
is_deferred;
}
+/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ */
void
-sql_drop_foreign_key(struct Parse *parse_context)
+sql_drop_constraint(struct Parse *parse)
{
- struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
- assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
+ struct drop_entity_def *drop_def = &parse->drop_constraint_def.base;
+ assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
const char *table_name = drop_def->base.entity_name->a[0].zName;
assert(table_name != NULL);
- struct space *child = space_by_name(table_name);
- if (child == NULL) {
+ struct space *space = space_by_name(table_name);
+ if (space == NULL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
- parse_context->is_aborted = true;
+ parse->is_aborted = true;
return;
}
- char *constraint_name =
- sql_name_from_token(parse_context->db, &drop_def->name);
- if (constraint_name == NULL) {
- parse_context->is_aborted = true;
+ char *name = sql_name_from_token(parse->db, &drop_def->name);
+ if (name == NULL) {
+ parse->is_aborted = true;
return;
}
- vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
- child->def->id);
+ struct constraint_id *id = space_find_constraint_id(space, name);
+ if (id == NULL) {
+ diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
+ parse->is_aborted = true;
+ return;
+ }
+ uint32_t space_id = space->def->id;
+ assert(id->type < constraint_type_MAX);
+ switch (id->type) {
+ case CONSTRAINT_TYPE_PK:
+ case CONSTRAINT_TYPE_UNIQUE: {
+ uint32_t index_id = box_index_id_by_name(space_id, name,
+ strlen(name));
+ /*
+ * We have already sure, that this index exists,
+ * so we don't check index_id for BOX_ID_NIL.
+ */
+ assert(index_id != BOX_ID_NIL);
+ int space_id_reg = ++parse->nMem;
+ int index_id_reg = ++parse->nMem;
+ struct Vdbe *v = sqlGetVdbe(parse);
+ assert(v != NULL);
+ sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
+ vdbe_emit_index_drop(parse, index_id, index_id_reg,
+ space_id_reg);
+ break;
+ }
+ case CONSTRAINT_TYPE_FK:
+ vdbe_emit_fk_constraint_drop(parse, name, space_id);
+ break;
+ case CONSTRAINT_TYPE_CK:
+ vdbe_emit_ck_constraint_drop(parse, name, space_id);
+ break;
+ default:
+ unreachable();
+ }
/*
* We account changes to row count only if drop of
- * foreign keys take place in a separate
- * ALTER TABLE DROP CONSTRAINT statement, since whole
- * DROP TABLE always returns 1 (one) as a row count.
+ * constraints take place in a separate ALTER TABLE DROP
+ * CONSTRAINT statement, since whole DROP TABLE always
+ * returns 1 (one) as a row count.
*/
- struct Vdbe *v = sqlGetVdbe(parse_context);
+ struct Vdbe *v = sqlGetVdbe(parse);
sqlVdbeCountChanges(v);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index cfe1c0012..1a0e89703 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1763,9 +1763,9 @@ cmd ::= alter_table_start(A) RENAME TO nm(N). {
}
cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
- drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
+ drop_constraint_def_init(&pParse->drop_constraint_def, X, &Z, false);
pParse->initiateTTrans = true;
- sql_drop_foreign_key(pParse);
+ sql_drop_constraint(pParse);
}
cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). {
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 2f433e4c0..e6d45b256 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -159,10 +159,6 @@ enum entity_type {
ENTITY_TYPE_TRIGGER,
ENTITY_TYPE_CK,
ENTITY_TYPE_FK,
- /**
- * For assertion checks that constraint definition is
- * created before initialization of a term constraint.
- */
ENTITY_TYPE_CONSTRAINT,
};
@@ -265,7 +261,7 @@ struct drop_trigger_def {
struct drop_entity_def base;
};
-struct drop_fk_def {
+struct drop_constraint_def {
struct drop_entity_def base;
};
@@ -408,11 +404,12 @@ drop_trigger_def_init(struct drop_trigger_def *drop_trigger_def,
}
static inline void
-drop_fk_def_init(struct drop_fk_def *drop_fk_def, struct SrcList *parent_name,
- struct Token *name, bool if_exist)
+drop_constraint_def_init(struct drop_constraint_def *drop_constraint_def,
+ struct SrcList *parent_name, struct Token *name,
+ bool if_exist)
{
- drop_entity_def_init(&drop_fk_def->base, parent_name, name, if_exist,
- ENTITY_TYPE_FK);
+ drop_entity_def_init(&drop_constraint_def->base, parent_name, name,
+ if_exist, ENTITY_TYPE_CONSTRAINT);
}
static inline void
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d1fcf4761..b1d4b913a 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2237,7 +2237,7 @@ struct Parse {
struct create_trigger_def create_trigger_def;
struct create_view_def create_view_def;
struct rename_entity_def rename_entity_def;
- struct drop_fk_def drop_fk_def;
+ struct drop_constraint_def drop_constraint_def;
struct drop_index_def drop_index_def;
struct drop_table_def drop_table_def;
struct drop_trigger_def drop_trigger_def;
@@ -3743,11 +3743,9 @@ sql_create_foreign_key(struct Parse *parse_context);
/**
* Function called from parser to handle
* <ALTER TABLE table DROP CONSTRAINT constraint> SQL statement.
- *
- * @param parse_context Parsing context.
*/
void
-sql_drop_foreign_key(struct Parse *parse_context);
+sql_drop_constraint(struct Parse *parse);
/**
* Now our SQL implementation can't operate on spaces which
diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
index 3e21a5f40..759acc9c3 100755
--- a/test/sql-tap/alter2.test.lua
+++ b/test/sql-tap/alter2.test.lua
@@ -256,7 +256,7 @@ test:do_catchsql_test(
ALTER TABLE child DROP CONSTRAINT fake;
]], {
-- <alter2-5.2>
- 1, "Constraint FAKE does not exist"
+ 1, "Constraint 'FAKE' does not exist in space 'CHILD'"
-- </alter2-5.2>
})
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 4ae0b4c10..7b18e5d6b 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -801,7 +801,7 @@ box.execute('ALTER TABLE test ADD CONSTRAINT \"some_ck\" CHECK(a < 10);')
box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"falsch\"")
---
- null
-- Constraint falsch does not exist
+- Constraint 'falsch' does not exist in space 'TEST'
...
box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"some_ck\"")
---
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
index 1585c2327..e8b8f382c 100644
--- a/test/sql/constraint.result
+++ b/test/sql/constraint.result
@@ -291,6 +291,73 @@ box.execute('CREATE UNIQUE INDEX d ON t2(i);')
| - Index 'D' already exists in space 'T2'
| ...
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX d ON t2;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C == nil
+ | ---
+ | - true
+ | ...
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.E ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+-- Check the error message.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - null
+ | - Constraint 'E' does not exist in space 'T2'
+ | ...
+
--
-- Cleanup.
--
diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
index 163f4309c..ef2cdb8cc 100755
--- a/test/sql/constraint.test.lua
+++ b/test/sql/constraint.test.lua
@@ -131,6 +131,30 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
-- uniqueness, index names should be unique in a space.
box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.index.E == nil
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX d ON t2;')
+box.space.T2.index.C ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+box.space.T2.index.C == nil
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+box.space.T2.ck_constraint.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.ck_constraint.E == nil
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+-- Check the error message.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+
--
-- Cleanup.
--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] sql: support constraint drop
2020-01-24 14:21 ` [Tarantool-patches] [PATCH 2/2] " Roman Khabibov
@ 2020-01-28 17:39 ` Nikita Pettik
2020-02-01 17:36 ` Roman Khabibov
0 siblings, 1 reply; 13+ messages in thread
From: Nikita Pettik @ 2020-01-28 17:39 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches
On 24 Jan 17:21, Roman Khabibov wrote:
> Hi! Thanks for the review.
>
> > On Jan 13, 2020, at 20:00, Nikita Pettik <korablev@tarantool.org> wrote:
> >
> > On 09 Jan 13:15, Roman Khabibov wrote:
> >> diff --git a/src/box/errcode.h b/src/box/errcode.h
> >> index 3065b1948..9ba42dfb4 100644
> >> --- a/src/box/errcode.h
> >> +++ b/src/box/errcode.h
> >> @@ -221,7 +221,7 @@ struct errcode_record {
> >> /*166 */_(ER_NO_SUCH_COLLATION, "Collation '%s' does not exist") \
> >> /*167 */_(ER_CREATE_FK_CONSTRAINT, "Failed to create foreign key constraint '%s': %s") \
> >> /*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \
> >> - /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \
> >> + /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint '%s' does not exist in space '%s'") \
> >
> > Why did you change error (in scope of this patch)?
> In the previous patch about constraint_id, I did about the same with ER_CONSTRAINT_EXISTS.
> Now I decided that changing the error also applies to this patch and it is too small to be
> a separate patch.
Separation into patches shouldn't be done based on size estimation,
but rather depending on logical consistency. Please, move to a separate
patch and add justification for that.
> >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> >> index bc50ecbfa..7c4b5760e 100644
> >> --- a/src/box/sql/build.c
> >> +++ b/src/box/sql/build.c
> >> @@ -1469,6 +1469,29 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
> >> sql_table_delete_from(parse, src_list, where);
> >> @@ -1488,17 +1511,6 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
> >> sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
> >> P4_DYNAMIC);
> >> sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1);
> >> - const char *error_msg =
> >> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
> >> - constraint_name);
> >> - if (vdbe_emit_halt_with_presence_test(parse_context,
> >> - BOX_FK_CONSTRAINT_ID, 0,
> >> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
> >> - error_msg, false,
> >> - OP_Found) != 0) {
> >> - sqlDbFree(parse_context->db, constraint_name);
> >> - return;
> >> - }
> >
> > Why did you drop this check?
> >
> >> sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
> >> sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
> >> VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
> >> @@ -1523,12 +1535,6 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
> >> sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
> >> sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
> >> sqlDbStrDup(db, ck_name), P4_DYNAMIC);
> >> - const char *error_msg =
> >> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
> >> - if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
> >> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
> >> - error_msg, false, OP_Found) != 0)
> >> - return;
> >
> > Same question.
> I dropped these checks, because I thought that the data are consistent. These two
> functions are called when:
> 1) DROP TABLE. If the constraint exist in struct space, then the corresponding tuple
> exists in _ck/_fk system space too. Therefore this error can not occur in box.
>
> 2) ALTER TABLE DROP CONSTRAINT. Analogically, I check the constraint for existence in
> sql_drop_constraint() and throw error on parsing level.
Data can be consistent at the parsing stage, but be inconsistent during
execution. Especially taking into account 'prepare' mechanism having
been recently introduced. So please put these checks back.
> >> @@ -2050,28 +2044,61 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
> >> is_deferred;
> >> }
> >>
> >> +/**
> >> + * Emit code to drop the entry from _index or _ck_contstraint or
> >> + * _fk_constraint space corresponding with the constraint type.
> >> + */
> >> void
> >> -sql_drop_foreign_key(struct Parse *parse_context)
> >> +sql_drop_constraint(struct Parse *parse_context)
> >> {
> >> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
> >> - child->def->id);
> >> + uint32_t space_id = space->def->id;
> >> + assert(id->type == CONSTRAINT_TYPE_PK || id->type ==
> >> + CONSTRAINT_TYPE_UNIQUE || id->type == CONSTRAINT_TYPE_FK ||
> >> + id->type == CONSTRAINT_TYPE_CK);
> >
> > Instead of such huge checks iterating over all possible variants,
> > enum can be extended with _MAX last element (see enum index_type for
> > example) and check becomes quite simple: assert(id->type < contraint_type_MAX);
> Ok, but I thought that this assert is needed to prevent the following situation.
> For example, I have added new value to enum constraint_type, and I forgot to
> extend this switch-case construction. Then I will get unreachable().
Yep, and what's wrong with this assumption?
> >> + vdbe_emit_ck_constraint_drop(parse_context, name, space_id);
> >> + break;
> >> + default:
> >> + unreachable();
> >> + }
> >> /*
> >> * We account changes to row count only if drop of
> >> * foreign keys take place in a separate
> +/**
> + * Emit code to drop the entry from _index or _ck_contstraint or
> + * _fk_constraint space corresponding with the constraint type.
> + */
> void
> -sql_drop_foreign_key(struct Parse *parse_context)
> +sql_drop_constraint(struct Parse *parse)
> {
Did I request this renaming?
> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
> + struct drop_entity_def *drop_def = &parse->drop_constraint_def.base;
> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
> const char *table_name = drop_def->base.entity_name->a[0].zName;
> assert(table_name != NULL);
> - struct space *child = space_by_name(table_name);
> - if (child == NULL) {
> + struct space *space = space_by_name(table_name);
> + if (space == NULL) {
> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
> - parse_context->is_aborted = true;
> + parse->is_aborted = true;
> return;
> }
> - char *constraint_name =
> - sql_name_from_token(parse_context->db, &drop_def->name);
> - if (constraint_name == NULL) {
> - parse_context->is_aborted = true;
> + char *name = sql_name_from_token(parse->db, &drop_def->name);
> + if (name == NULL) {
> + parse->is_aborted = true;
> return;
> }
> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
> - child->def->id);
> + struct constraint_id *id = space_find_constraint_id(space, name);
> + if (id == NULL) {
> + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
> + parse->is_aborted = true;
> + return;
> + }
> + uint32_t space_id = space->def->id;
> + assert(id->type < constraint_type_MAX);
> + switch (id->type) {
> + case CONSTRAINT_TYPE_PK:
> + case CONSTRAINT_TYPE_UNIQUE: {
> + uint32_t index_id = box_index_id_by_name(space_id, name,
> + strlen(name));
> + /*
> + * We have already sure, that this index exists,
> + * so we don't check index_id for BOX_ID_NIL.
> + */
Nit: have already assured/verified (sure is an adjective/adverb).
> + assert(index_id != BOX_ID_NIL);
> + int space_id_reg = ++parse->nMem;
> + int index_id_reg = ++parse->nMem;
> + struct Vdbe *v = sqlGetVdbe(parse);
> + assert(v != NULL);
> + sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
> + vdbe_emit_index_drop(parse, index_id, index_id_reg,
> + space_id_reg);
> + break;
> + }
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] sql: support constraint drop
2020-01-28 17:39 ` Nikita Pettik
@ 2020-02-01 17:36 ` Roman Khabibov
2020-02-11 16:56 ` Nikita Pettik
0 siblings, 1 reply; 13+ messages in thread
From: Roman Khabibov @ 2020-02-01 17:36 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
Hi! Thanks for the review.
> On Jan 28, 2020, at 20:39, Nikita Pettik <korablev@tarantool.org> wrote:
>
> On 24 Jan 17:21, Roman Khabibov wrote:
>> Hi! Thanks for the review.
>>
>>> On Jan 13, 2020, at 20:00, Nikita Pettik <korablev@tarantool.org> wrote:
>>>
>>> On 09 Jan 13:15, Roman Khabibov wrote:
>>>> diff --git a/src/box/errcode.h b/src/box/errcode.h
>>>> index 3065b1948..9ba42dfb4 100644
>>>> --- a/src/box/errcode.h
>>>> +++ b/src/box/errcode.h
>>>> @@ -221,7 +221,7 @@ struct errcode_record {
>>>> /*166 */_(ER_NO_SUCH_COLLATION, "Collation '%s' does not exist") \
>>>> /*167 */_(ER_CREATE_FK_CONSTRAINT, "Failed to create foreign key constraint '%s': %s") \
>>>> /*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \
>>>> - /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \
>>>> + /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint '%s' does not exist in space '%s'") \
>>>
>>> Why did you change error (in scope of this patch)?
>> In the previous patch about constraint_id, I did about the same with ER_CONSTRAINT_EXISTS.
>> Now I decided that changing the error also applies to this patch and it is too small to be
>> a separate patch.
>
> Separation into patches shouldn't be done based on size estimation,
> but rather depending on logical consistency. Please, move to a separate
> patch and add justification for that.
Done.
>>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>>> index bc50ecbfa..7c4b5760e 100644
>>>> --- a/src/box/sql/build.c
>>>> +++ b/src/box/sql/build.c
>>>> @@ -1469,6 +1469,29 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
>>>> sql_table_delete_from(parse, src_list, where);
>>>> @@ -1488,17 +1511,6 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
>>>> sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
>>>> P4_DYNAMIC);
>>>> sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1);
>>>> - const char *error_msg =
>>>> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
>>>> - constraint_name);
>>>> - if (vdbe_emit_halt_with_presence_test(parse_context,
>>>> - BOX_FK_CONSTRAINT_ID, 0,
>>>> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
>>>> - error_msg, false,
>>>> - OP_Found) != 0) {
>>>> - sqlDbFree(parse_context->db, constraint_name);
>>>> - return;
>>>> - }
>>>
>>> Why did you drop this check?
>>>
>>>> sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
>>>> sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
>>>> VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
>>>> @@ -1523,12 +1535,6 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
>>>> sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
>>>> sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
>>>> sqlDbStrDup(db, ck_name), P4_DYNAMIC);
>>>> - const char *error_msg =
>>>> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
>>>> - if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
>>>> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
>>>> - error_msg, false, OP_Found) != 0)
>>>> - return;
>>>
>>> Same question.
>> I dropped these checks, because I thought that the data are consistent. These two
>> functions are called when:
>> 1) DROP TABLE. If the constraint exist in struct space, then the corresponding tuple
>> exists in _ck/_fk system space too. Therefore this error can not occur in box.
>>
>> 2) ALTER TABLE DROP CONSTRAINT. Analogically, I check the constraint for existence in
>> sql_drop_constraint() and throw error on parsing level.
>
> Data can be consistent at the parsing stage, but be inconsistent during
> execution. Especially taking into account 'prepare' mechanism having
> been recently introduced. So please put these checks back.
Done.
>>>> @@ -2050,28 +2044,61 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
>>>> is_deferred;
>>>> }
>>>>
>>>> +/**
>>>> + * Emit code to drop the entry from _index or _ck_contstraint or
>>>> + * _fk_constraint space corresponding with the constraint type.
>>>> + */
>>>> void
>>>> -sql_drop_foreign_key(struct Parse *parse_context)
>>>> +sql_drop_constraint(struct Parse *parse_context)
>>>> {
>>>> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
>>>> - child->def->id);
>>>> + uint32_t space_id = space->def->id;
>>>> + assert(id->type == CONSTRAINT_TYPE_PK || id->type ==
>>>> + CONSTRAINT_TYPE_UNIQUE || id->type == CONSTRAINT_TYPE_FK ||
>>>> + id->type == CONSTRAINT_TYPE_CK);
>>>
>>> Instead of such huge checks iterating over all possible variants,
>>> enum can be extended with _MAX last element (see enum index_type for
>>> example) and check becomes quite simple: assert(id->type < contraint_type_MAX);
>> Ok, but I thought that this assert is needed to prevent the following situation.
>> For example, I have added new value to enum constraint_type, and I forgot to
>> extend this switch-case construction. Then I will get unreachable().
>
> Yep, and what's wrong with this assumption?
>
>>>> + vdbe_emit_ck_constraint_drop(parse_context, name, space_id);
>>>> + break;
>>>> + default:
>>>> + unreachable();
>>>> + }
>>>> /*
>>>> * We account changes to row count only if drop of
>>>> * foreign keys take place in a separate
>> +/**
>> + * Emit code to drop the entry from _index or _ck_contstraint or
>> + * _fk_constraint space corresponding with the constraint type.
>> + */
>> void
>> -sql_drop_foreign_key(struct Parse *parse_context)
>> +sql_drop_constraint(struct Parse *parse)
>> {
>
> Did I request this renaming?
No, but it slightly reduced the code.
>> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
>> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
>> + struct drop_entity_def *drop_def = &parse->drop_constraint_def.base;
>> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
>> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
>> const char *table_name = drop_def->base.entity_name->a[0].zName;
>> assert(table_name != NULL);
>> - struct space *child = space_by_name(table_name);
>> - if (child == NULL) {
>> + struct space *space = space_by_name(table_name);
>> + if (space == NULL) {
>> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
>> - parse_context->is_aborted = true;
>> + parse->is_aborted = true;
>> return;
>> }
>> - char *constraint_name =
>> - sql_name_from_token(parse_context->db, &drop_def->name);
>> - if (constraint_name == NULL) {
>> - parse_context->is_aborted = true;
>> + char *name = sql_name_from_token(parse->db, &drop_def->name);
>> + if (name == NULL) {
>> + parse->is_aborted = true;
>> return;
>> }
>> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
>> - child->def->id);
>> + struct constraint_id *id = space_find_constraint_id(space, name);
>> + if (id == NULL) {
>> + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
>> + parse->is_aborted = true;
>> + return;
>> + }
>> + uint32_t space_id = space->def->id;
>> + assert(id->type < constraint_type_MAX);
>> + switch (id->type) {
>> + case CONSTRAINT_TYPE_PK:
>> + case CONSTRAINT_TYPE_UNIQUE: {
>> + uint32_t index_id = box_index_id_by_name(space_id, name,
>> + strlen(name));
>> + /*
>> + * We have already sure, that this index exists,
>> + * so we don't check index_id for BOX_ID_NIL.
>> + */
>
> Nit: have already assured/verified (sure is an adjective/adverb).
Done.
>> + assert(index_id != BOX_ID_NIL);
>> + int space_id_reg = ++parse->nMem;
>> + int index_id_reg = ++parse->nMem;
>> + struct Vdbe *v = sqlGetVdbe(parse);
>> + assert(v != NULL);
>> + sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
>> + vdbe_emit_index_drop(parse, index_id, index_id_reg,
>> + space_id_reg);
>> + break;
>> + }
commit 818af1cca284eb6c19b7367f20c00de26b8d2305
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date: Thu Jan 2 20:13:36 2020 +0300
sql: support constraint drop
Extend <ALTER TABLE> statement to drop table constraints by their
names.
Closes #4120
@TarantoolBot document
Title: Drop table constraints in SQL
Now, it is possible to drop table constraints (PRIMARY KEY,
UNIQUE, FOREIGN KEY, CHECK) using
<ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
by their names.
For example:
tarantool> box.execute([[CREATE TABLE test (
a INTEGER PRIMARY KEY,
b INTEGER,
CONSTRAINT cnstr CHECK (a >= 0)
);]])
---
- row_count: 1
...
tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
---
- row_count: 1
...
The same for all the other constraints.
diff --git a/src/box/constraint_id.h b/src/box/constraint_id.h
index 21f067cdc..ed4f37426 100755
--- a/src/box/constraint_id.h
+++ b/src/box/constraint_id.h
@@ -40,6 +40,7 @@ enum constraint_type {
CONSTRAINT_TYPE_UNIQUE,
CONSTRAINT_TYPE_FK,
CONSTRAINT_TYPE_CK,
+ constraint_type_MAX,
};
extern const char *constraint_type_strs[];
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index d9bf8de91..27963c455 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2052,35 +2052,74 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
is_deferred;
}
+/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ */
void
-sql_drop_foreign_key(struct Parse *parse_context)
+sql_drop_constraint(struct Parse *parse_context)
{
- struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
- assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
+ struct drop_entity_def *drop_def =
+ &parse_context->drop_constraint_def.base;
+ assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
const char *table_name = drop_def->base.entity_name->a[0].zName;
assert(table_name != NULL);
- struct space *child = space_by_name(table_name);
- if (child == NULL) {
+ struct space *space = space_by_name(table_name);
+ if (space == NULL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
parse_context->is_aborted = true;
return;
}
- char *constraint_name =
- sql_name_from_token(parse_context->db, &drop_def->name);
- if (constraint_name == NULL) {
+ char *name = sql_name_from_token(parse_context->db, &drop_def->name);
+ if (name == NULL) {
+ parse_context->is_aborted = true;
+ return;
+ }
+ struct constraint_id *id = space_find_constraint_id(space, name);
+ if (id == NULL) {
+ diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
parse_context->is_aborted = true;
return;
}
- vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
- child->def);
+ struct Vdbe *v = sqlGetVdbe(parse_context);
+ assert(v != NULL);
+ assert(id->type < constraint_type_MAX);
+ switch (id->type) {
+ case CONSTRAINT_TYPE_PK:
+ case CONSTRAINT_TYPE_UNIQUE: {
+ uint32_t index_id = box_index_id_by_name(space->def->id, name,
+ strlen(name));
+ /*
+ * We have already verified, that this index
+ * exists, so we don't check index_id for
+ * BOX_ID_NIL.
+ */
+ assert(index_id != BOX_ID_NIL);
+ int record_reg = ++parse_context->nMem;
+ int space_id_reg = ++parse_context->nMem;
+ int index_id_reg = ++parse_context->nMem;
+ sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
+ sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
+ sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
+ sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
+ break;
+ }
+ case CONSTRAINT_TYPE_FK:
+ vdbe_emit_fk_constraint_drop(parse_context, name, space->def);
+ break;
+ case CONSTRAINT_TYPE_CK:
+ vdbe_emit_ck_constraint_drop(parse_context, name, space->def);
+ break;
+ default:
+ unreachable();
+ }
/*
* We account changes to row count only if drop of
- * foreign keys take place in a separate
- * ALTER TABLE DROP CONSTRAINT statement, since whole
- * DROP TABLE always returns 1 (one) as a row count.
+ * constraints take place in a separate ALTER TABLE DROP
+ * CONSTRAINT statement, since whole DROP TABLE always
+ * returns 1 (one) as a row count.
*/
- struct Vdbe *v = sqlGetVdbe(parse_context);
sqlVdbeCountChanges(v);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index cfe1c0012..1a0e89703 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1763,9 +1763,9 @@ cmd ::= alter_table_start(A) RENAME TO nm(N). {
}
cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
- drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
+ drop_constraint_def_init(&pParse->drop_constraint_def, X, &Z, false);
pParse->initiateTTrans = true;
- sql_drop_foreign_key(pParse);
+ sql_drop_constraint(pParse);
}
cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). {
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 2f433e4c0..e6d45b256 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -159,10 +159,6 @@ enum entity_type {
ENTITY_TYPE_TRIGGER,
ENTITY_TYPE_CK,
ENTITY_TYPE_FK,
- /**
- * For assertion checks that constraint definition is
- * created before initialization of a term constraint.
- */
ENTITY_TYPE_CONSTRAINT,
};
@@ -265,7 +261,7 @@ struct drop_trigger_def {
struct drop_entity_def base;
};
-struct drop_fk_def {
+struct drop_constraint_def {
struct drop_entity_def base;
};
@@ -408,11 +404,12 @@ drop_trigger_def_init(struct drop_trigger_def *drop_trigger_def,
}
static inline void
-drop_fk_def_init(struct drop_fk_def *drop_fk_def, struct SrcList *parent_name,
- struct Token *name, bool if_exist)
+drop_constraint_def_init(struct drop_constraint_def *drop_constraint_def,
+ struct SrcList *parent_name, struct Token *name,
+ bool if_exist)
{
- drop_entity_def_init(&drop_fk_def->base, parent_name, name, if_exist,
- ENTITY_TYPE_FK);
+ drop_entity_def_init(&drop_constraint_def->base, parent_name, name,
+ if_exist, ENTITY_TYPE_CONSTRAINT);
}
static inline void
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d1fcf4761..2ea653c7d 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2237,7 +2237,7 @@ struct Parse {
struct create_trigger_def create_trigger_def;
struct create_view_def create_view_def;
struct rename_entity_def rename_entity_def;
- struct drop_fk_def drop_fk_def;
+ struct drop_constraint_def drop_constraint_def;
struct drop_index_def drop_index_def;
struct drop_table_def drop_table_def;
struct drop_trigger_def drop_trigger_def;
@@ -3747,7 +3747,7 @@ sql_create_foreign_key(struct Parse *parse_context);
* @param parse_context Parsing context.
*/
void
-sql_drop_foreign_key(struct Parse *parse_context);
+sql_drop_constraint(struct Parse *parse_context);
/**
* Now our SQL implementation can't operate on spaces which
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
index 1585c2327..2847fe8ef 100644
--- a/test/sql/constraint.result
+++ b/test/sql/constraint.result
@@ -291,6 +291,67 @@ box.execute('CREATE UNIQUE INDEX d ON t2(i);')
| - Index 'D' already exists in space 'T2'
| ...
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX d ON t2;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C == nil
+ | ---
+ | - true
+ | ...
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.E ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+
--
-- Cleanup.
--
diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
index 163f4309c..12f673dac 100755
--- a/test/sql/constraint.test.lua
+++ b/test/sql/constraint.test.lua
@@ -131,6 +131,28 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
-- uniqueness, index names should be unique in a space.
box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.index.E == nil
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX d ON t2;')
+box.space.T2.index.C ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+box.space.T2.index.C == nil
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+box.space.T2.ck_constraint.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.ck_constraint.E == nil
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+
--
-- Cleanup.
--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] sql: support constraint drop
2020-02-01 17:36 ` Roman Khabibov
@ 2020-02-11 16:56 ` Nikita Pettik
2020-02-16 10:24 ` Roman Khabibov
0 siblings, 1 reply; 13+ messages in thread
From: Nikita Pettik @ 2020-02-11 16:56 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches
On 01 Feb 20:36, Roman Khabibov wrote:
> Hi! Thanks for the review.
>
> >>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> >>>> index bc50ecbfa..7c4b5760e 100644
> >>>> --- a/src/box/sql/build.c
> >>>> +++ b/src/box/sql/build.c
> >>>> @@ -1469,6 +1469,29 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
> >>>> sql_table_delete_from(parse, src_list, where);
> >>>> @@ -1488,17 +1511,6 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
> >>>> sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
> >>>> P4_DYNAMIC);
> >>>> sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1);
> >>>> - const char *error_msg =
> >>>> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
> >>>> - constraint_name);
> >>>> - if (vdbe_emit_halt_with_presence_test(parse_context,
> >>>> - BOX_FK_CONSTRAINT_ID, 0,
> >>>> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
> >>>> - error_msg, false,
> >>>> - OP_Found) != 0) {
> >>>> - sqlDbFree(parse_context->db, constraint_name);
> >>>> - return;
> >>>> - }
> >>>
> >>> Why did you drop this check?
> >>>
> >>>> sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
> >>>> sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
> >>>> VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
> >>>> @@ -1523,12 +1535,6 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
> >>>> sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
> >>>> sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
> >>>> sqlDbStrDup(db, ck_name), P4_DYNAMIC);
> >>>> - const char *error_msg =
> >>>> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
> >>>> - if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
> >>>> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
> >>>> - error_msg, false, OP_Found) != 0)
> >>>> - return;
> >>>
> >>> Same question.
> >> I dropped these checks, because I thought that the data are consistent. These two
> >> functions are called when:
> >> 1) DROP TABLE. If the constraint exist in struct space, then the corresponding tuple
> >> exists in _ck/_fk system space too. Therefore this error can not occur in box.
> >>
> >> 2) ALTER TABLE DROP CONSTRAINT. Analogically, I check the constraint for existence in
> >> sql_drop_constraint() and throw error on parsing level.
> >
> > Data can be consistent at the parsing stage, but be inconsistent during
> > execution. Especially taking into account 'prepare' mechanism having
> > been recently introduced. So please put these checks back.
> Done.
In sql_drop_constraint() in index dropping branch there's still no
vdbe_emit_halt_with_presence_test() call. Please add it there as well.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index d9bf8de91..27963c455 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2052,35 +2052,74 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
> is_deferred;
> }
>
> +/**
> + * Emit code to drop the entry from _index or _ck_contstraint or
> + * _fk_constraint space corresponding with the constraint type.
> + */
As a rule we place comments near function declaration.
> void
> -sql_drop_foreign_key(struct Parse *parse_context)
> +sql_drop_constraint(struct Parse *parse_context)
> {
> }
> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
> - child->def);
> + struct Vdbe *v = sqlGetVdbe(parse_context);
> + assert(v != NULL);
> + assert(id->type < constraint_type_MAX);
> + switch (id->type) {
> + case CONSTRAINT_TYPE_PK:
> + case CONSTRAINT_TYPE_UNIQUE: {
> + uint32_t index_id = box_index_id_by_name(space->def->id, name,
> + strlen(name));
> + /*
> + * We have already verified, that this index
> + * exists, so we don't check index_id for
> + * BOX_ID_NIL.
> + */
> + assert(index_id != BOX_ID_NIL);
> + int record_reg = ++parse_context->nMem;
> + int space_id_reg = ++parse_context->nMem;
> + int index_id_reg = ++parse_context->nMem;
> + sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
> + sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
> + sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
> + sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
> + break;
> + }
> + case CONSTRAINT_TYPE_FK:
> + vdbe_emit_fk_constraint_drop(parse_context, name, space->def);
> + break;
> + case CONSTRAINT_TYPE_CK:
> + vdbe_emit_ck_constraint_drop(parse_context, name, space->def);
> + break;
> + default:
> + unreachable();
> + }
> /*
> * We account changes to row count only if drop of
> - * foreign keys take place in a separate
> - * ALTER TABLE DROP CONSTRAINT statement, since whole
> - * DROP TABLE always returns 1 (one) as a row count.
> + * constraints take place in a separate ALTER TABLE DROP
> + * CONSTRAINT statement, since whole DROP TABLE always
> + * returns 1 (one) as a row count.
> */
This comment doesn't look like related to this function:
sql_drop_constraint() is never called within drop table statement.
> - struct Vdbe *v = sqlGetVdbe(parse_context);
> sqlVdbeCountChanges(v);
> sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
> }
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index cfe1c0012..1a0e89703 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1763,9 +1763,9 @@ cmd ::= alter_table_start(A) RENAME TO nm(N). {
> }
>
> cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
> - drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
> + drop_constraint_def_init(&pParse->drop_constraint_def, X, &Z, false);
> pParse->initiateTTrans = true;
> - sql_drop_foreign_key(pParse);
> + sql_drop_constraint(pParse);
> }
>
> cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). {
> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
> index 2f433e4c0..e6d45b256 100644
> --- a/src/box/sql/parse_def.h
> +++ b/src/box/sql/parse_def.h
> @@ -159,10 +159,6 @@ enum entity_type {
> ENTITY_TYPE_TRIGGER,
> ENTITY_TYPE_CK,
> ENTITY_TYPE_FK,
> - /**
> - * For assertion checks that constraint definition is
> - * created before initialization of a term constraint.
> - */
Why did you drop this comment?
> ENTITY_TYPE_CONSTRAINT,
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] sql: support constraint drop
2020-02-11 16:56 ` Nikita Pettik
@ 2020-02-16 10:24 ` Roman Khabibov
2020-02-20 19:55 ` Nikita Pettik
0 siblings, 1 reply; 13+ messages in thread
From: Roman Khabibov @ 2020-02-16 10:24 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
Hi! Thanks for the review.
> On Feb 11, 2020, at 19:56, Nikita Pettik <korablev@tarantool.org> wrote:
>
> On 01 Feb 20:36, Roman Khabibov wrote:
>> Hi! Thanks for the review.
>>
>>>>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>>>>> index bc50ecbfa..7c4b5760e 100644
>>>>>> --- a/src/box/sql/build.c
>>>>>> +++ b/src/box/sql/build.c
>>>>>> @@ -1469,6 +1469,29 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
>>>>>> sql_table_delete_from(parse, src_list, where);
>>>>>> @@ -1488,17 +1511,6 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
>>>>>> sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
>>>>>> P4_DYNAMIC);
>>>>>> sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1);
>>>>>> - const char *error_msg =
>>>>>> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
>>>>>> - constraint_name);
>>>>>> - if (vdbe_emit_halt_with_presence_test(parse_context,
>>>>>> - BOX_FK_CONSTRAINT_ID, 0,
>>>>>> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
>>>>>> - error_msg, false,
>>>>>> - OP_Found) != 0) {
>>>>>> - sqlDbFree(parse_context->db, constraint_name);
>>>>>> - return;
>>>>>> - }
>>>>>
>>>>> Why did you drop this check?
>>>>>
>>>>>> sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
>>>>>> sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
>>>>>> VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
>>>>>> @@ -1523,12 +1535,6 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
>>>>>> sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
>>>>>> sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
>>>>>> sqlDbStrDup(db, ck_name), P4_DYNAMIC);
>>>>>> - const char *error_msg =
>>>>>> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
>>>>>> - if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
>>>>>> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
>>>>>> - error_msg, false, OP_Found) != 0)
>>>>>> - return;
>>>>>
>>>>> Same question.
>>>> I dropped these checks, because I thought that the data are consistent. These two
>>>> functions are called when:
>>>> 1) DROP TABLE. If the constraint exist in struct space, then the corresponding tuple
>>>> exists in _ck/_fk system space too. Therefore this error can not occur in box.
>>>>
>>>> 2) ALTER TABLE DROP CONSTRAINT. Analogically, I check the constraint for existence in
>>>> sql_drop_constraint() and throw error on parsing level.
>>>
>>> Data can be consistent at the parsing stage, but be inconsistent during
>>> execution. Especially taking into account 'prepare' mechanism having
>>> been recently introduced. So please put these checks back.
>> Done.
>
> In sql_drop_constraint() in index dropping branch there's still no
> vdbe_emit_halt_with_presence_test() call. Please add it there as well.
+ case CONSTRAINT_TYPE_PK:
+ case CONSTRAINT_TYPE_UNIQUE: {
+ uint32_t index_id = box_index_id_by_name(space->def->id, name,
+ strlen(name));
+ /*
+ * We have already verified, that this index
+ * exists, so we don't check index_id for
+ * BOX_ID_NIL.
+ */
+ assert(index_id != BOX_ID_NIL);
+ int record_reg = ++parse_context->nMem;
+ int space_id_reg = ++parse_context->nMem;
+ int index_id_reg = ++parse_context->nMem;
+ sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
+ sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
+ const char *error_msg =
+ tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
+ id->name, space->def->name);
+ if (vdbe_emit_halt_with_presence_test(parse_context,
+ BOX_INDEX_ID, 0,
+ space_id_reg, 2,
+ ER_NO_SUCH_CONSTRAINT,
+ error_msg, false,
+ OP_Found) != 0)
+ return;
+ sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
+ sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
+ break;
+ }
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index d9bf8de91..27963c455 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -2052,35 +2052,74 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
>> is_deferred;
>> }
>>
>> +/**
>> + * Emit code to drop the entry from _index or _ck_contstraint or
>> + * _fk_constraint space corresponding with the constraint type.
>> + */
>
> As a rule we place comments near function declaration.
/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ *
* Function called from parser to handle
* <ALTER TABLE table DROP CONSTRAINT constraint> SQL statement.
*
* @param parse_context Parsing context.
*/
void
-sql_drop_foreign_key(struct Parse *parse_context);
+sql_drop_constraint(struct Parse *parse_context);
>> void
>> -sql_drop_foreign_key(struct Parse *parse_context)
>> +sql_drop_constraint(struct Parse *parse_context)
>> {
>> }
>> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
>> - child->def);
>> + struct Vdbe *v = sqlGetVdbe(parse_context);
>> + assert(v != NULL);
>> + assert(id->type < constraint_type_MAX);
>> + switch (id->type) {
>> + case CONSTRAINT_TYPE_PK:
>> + case CONSTRAINT_TYPE_UNIQUE: {
>> + uint32_t index_id = box_index_id_by_name(space->def->id, name,
>> + strlen(name));
>> + /*
>> + * We have already verified, that this index
>> + * exists, so we don't check index_id for
>> + * BOX_ID_NIL.
>> + */
>> + assert(index_id != BOX_ID_NIL);
>> + int record_reg = ++parse_context->nMem;
>> + int space_id_reg = ++parse_context->nMem;
>> + int index_id_reg = ++parse_context->nMem;
>> + sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
>> + sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
>> + sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
>> + sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
>> + break;
>> + }
>> + case CONSTRAINT_TYPE_FK:
>> + vdbe_emit_fk_constraint_drop(parse_context, name, space->def);
>> + break;
>> + case CONSTRAINT_TYPE_CK:
>> + vdbe_emit_ck_constraint_drop(parse_context, name, space->def);
>> + break;
>> + default:
>> + unreachable();
>> + }
>> /*
>> * We account changes to row count only if drop of
>> - * foreign keys take place in a separate
>> - * ALTER TABLE DROP CONSTRAINT statement, since whole
>> - * DROP TABLE always returns 1 (one) as a row count.
>> + * constraints take place in a separate ALTER TABLE DROP
>> + * CONSTRAINT statement, since whole DROP TABLE always
>> + * returns 1 (one) as a row count.
>> */
>
> This comment doesn't look like related to this function:
> sql_drop_constraint() is never called within drop table statement.
Dropped.
>> - struct Vdbe *v = sqlGetVdbe(parse_context);
>> sqlVdbeCountChanges(v);
>> sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
>> }
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index cfe1c0012..1a0e89703 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -1763,9 +1763,9 @@ cmd ::= alter_table_start(A) RENAME TO nm(N). {
>> }
>>
>> cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
>> - drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
>> + drop_constraint_def_init(&pParse->drop_constraint_def, X, &Z, false);
>> pParse->initiateTTrans = true;
>> - sql_drop_foreign_key(pParse);
>> + sql_drop_constraint(pParse);
>> }
>>
>> cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). {
>> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
>> index 2f433e4c0..e6d45b256 100644
>> --- a/src/box/sql/parse_def.h
>> +++ b/src/box/sql/parse_def.h
>> @@ -159,10 +159,6 @@ enum entity_type {
>> ENTITY_TYPE_TRIGGER,
>> ENTITY_TYPE_CK,
>> ENTITY_TYPE_FK,
>> - /**
>> - * For assertion checks that constraint definition is
>> - * created before initialization of a term constraint.
>> - */
>
> Why did you drop this comment?
Returned. I mistakenly thought, that it isn’t only for asserts.
commit 490b34dacc19cb9ea7fc36f52dec023fb20d1b3b
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date: Thu Jan 2 20:13:36 2020 +0300
sql: support constraint drop
Extend <ALTER TABLE> statement to drop table constraints by their
names.
Closes #4120
@TarantoolBot document
Title: Drop table constraints in SQL
Now, it is possible to drop table constraints (PRIMARY KEY,
UNIQUE, FOREIGN KEY, CHECK) using
<ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
by their names.
For example:
tarantool> box.execute([[CREATE TABLE test (
a INTEGER PRIMARY KEY,
b INTEGER,
CONSTRAINT cnstr CHECK (a >= 0)
);]])
---
- row_count: 1
...
tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
---
- row_count: 1
...
The same for all the other constraints.
diff --git a/src/box/constraint_id.h b/src/box/constraint_id.h
index 21f067cdc..ed4f37426 100755
--- a/src/box/constraint_id.h
+++ b/src/box/constraint_id.h
@@ -40,6 +40,7 @@ enum constraint_type {
CONSTRAINT_TYPE_UNIQUE,
CONSTRAINT_TYPE_FK,
CONSTRAINT_TYPE_CK,
+ constraint_type_MAX,
};
extern const char *constraint_type_strs[];
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index d9bf8de91..76ad79350 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2052,35 +2052,78 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
is_deferred;
}
+/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ */
void
-sql_drop_foreign_key(struct Parse *parse_context)
+sql_drop_constraint(struct Parse *parse_context)
{
- struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
- assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
+ struct drop_entity_def *drop_def =
+ &parse_context->drop_constraint_def.base;
+ assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
const char *table_name = drop_def->base.entity_name->a[0].zName;
assert(table_name != NULL);
- struct space *child = space_by_name(table_name);
- if (child == NULL) {
+ struct space *space = space_by_name(table_name);
+ if (space == NULL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
parse_context->is_aborted = true;
return;
}
- char *constraint_name =
- sql_name_from_token(parse_context->db, &drop_def->name);
- if (constraint_name == NULL) {
+ char *name = sql_name_from_token(parse_context->db, &drop_def->name);
+ if (name == NULL) {
+ parse_context->is_aborted = true;
+ return;
+ }
+ struct constraint_id *id = space_find_constraint_id(space, name);
+ if (id == NULL) {
+ diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
parse_context->is_aborted = true;
return;
}
- vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
- child->def);
- /*
- * We account changes to row count only if drop of
- * foreign keys take place in a separate
- * ALTER TABLE DROP CONSTRAINT statement, since whole
- * DROP TABLE always returns 1 (one) as a row count.
- */
struct Vdbe *v = sqlGetVdbe(parse_context);
+ assert(v != NULL);
+ assert(id->type < constraint_type_MAX);
+ switch (id->type) {
+ case CONSTRAINT_TYPE_PK:
+ case CONSTRAINT_TYPE_UNIQUE: {
+ uint32_t index_id = box_index_id_by_name(space->def->id, name,
+ strlen(name));
+ /*
+ * We have already verified, that this index
+ * exists, so we don't check index_id for
+ * BOX_ID_NIL.
+ */
+ assert(index_id != BOX_ID_NIL);
+ int record_reg = ++parse_context->nMem;
+ int space_id_reg = ++parse_context->nMem;
+ int index_id_reg = ++parse_context->nMem;
+ sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
+ sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
+ const char *error_msg =
+ tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
+ id->name, space->def->name);
+ if (vdbe_emit_halt_with_presence_test(parse_context,
+ BOX_INDEX_ID, 0,
+ space_id_reg, 2,
+ ER_NO_SUCH_CONSTRAINT,
+ error_msg, false,
+ OP_Found) != 0)
+ return;
+ sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
+ sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
+ break;
+ }
+ case CONSTRAINT_TYPE_FK:
+ vdbe_emit_fk_constraint_drop(parse_context, name, space->def);
+ break;
+ case CONSTRAINT_TYPE_CK:
+ vdbe_emit_ck_constraint_drop(parse_context, name, space->def);
+ break;
+ default:
+ unreachable();
+ }
sqlVdbeCountChanges(v);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index cfe1c0012..1a0e89703 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1763,9 +1763,9 @@ cmd ::= alter_table_start(A) RENAME TO nm(N). {
}
cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
- drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
+ drop_constraint_def_init(&pParse->drop_constraint_def, X, &Z, false);
pParse->initiateTTrans = true;
- sql_drop_foreign_key(pParse);
+ sql_drop_constraint(pParse);
}
cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). {
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 2f433e4c0..cb0ecd2fc 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -265,7 +265,7 @@ struct drop_trigger_def {
struct drop_entity_def base;
};
-struct drop_fk_def {
+struct drop_constraint_def {
struct drop_entity_def base;
};
@@ -408,11 +408,12 @@ drop_trigger_def_init(struct drop_trigger_def *drop_trigger_def,
}
static inline void
-drop_fk_def_init(struct drop_fk_def *drop_fk_def, struct SrcList *parent_name,
- struct Token *name, bool if_exist)
+drop_constraint_def_init(struct drop_constraint_def *drop_constraint_def,
+ struct SrcList *parent_name, struct Token *name,
+ bool if_exist)
{
- drop_entity_def_init(&drop_fk_def->base, parent_name, name, if_exist,
- ENTITY_TYPE_FK);
+ drop_entity_def_init(&drop_constraint_def->base, parent_name, name,
+ if_exist, ENTITY_TYPE_CONSTRAINT);
}
static inline void
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d1fcf4761..1579cc92e 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2237,7 +2237,7 @@ struct Parse {
struct create_trigger_def create_trigger_def;
struct create_view_def create_view_def;
struct rename_entity_def rename_entity_def;
- struct drop_fk_def drop_fk_def;
+ struct drop_constraint_def drop_constraint_def;
struct drop_index_def drop_index_def;
struct drop_table_def drop_table_def;
struct drop_trigger_def drop_trigger_def;
@@ -3741,13 +3741,16 @@ void
sql_create_foreign_key(struct Parse *parse_context);
/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ *
* Function called from parser to handle
* <ALTER TABLE table DROP CONSTRAINT constraint> SQL statement.
*
* @param parse_context Parsing context.
*/
void
-sql_drop_foreign_key(struct Parse *parse_context);
+sql_drop_constraint(struct Parse *parse_context);
/**
* Now our SQL implementation can't operate on spaces which
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
index 1585c2327..2847fe8ef 100644
--- a/test/sql/constraint.result
+++ b/test/sql/constraint.result
@@ -291,6 +291,67 @@ box.execute('CREATE UNIQUE INDEX d ON t2(i);')
| - Index 'D' already exists in space 'T2'
| ...
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX d ON t2;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C == nil
+ | ---
+ | - true
+ | ...
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.E ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+
--
-- Cleanup.
--
diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
index 163f4309c..12f673dac 100755
--- a/test/sql/constraint.test.lua
+++ b/test/sql/constraint.test.lua
@@ -131,6 +131,28 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
-- uniqueness, index names should be unique in a space.
box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.index.E == nil
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX d ON t2;')
+box.space.T2.index.C ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+box.space.T2.index.C == nil
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+box.space.T2.ck_constraint.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.ck_constraint.E == nil
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+
--
-- Cleanup.
--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] sql: support constraint drop
2020-02-16 10:24 ` Roman Khabibov
@ 2020-02-20 19:55 ` Nikita Pettik
0 siblings, 0 replies; 13+ messages in thread
From: Nikita Pettik @ 2020-02-20 19:55 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches, v.shpilevoy
On 16 Feb 13:24, Roman Khabibov wrote:
> Hi! Thanks for the review.
>
> > On Feb 11, 2020, at 19:56, Nikita Pettik <korablev@tarantool.org> wrote:
> >
> > On 01 Feb 20:36, Roman Khabibov wrote:
> >> Hi! Thanks for the review.
Now LGTM. Vlad, could you provide secodn review (just in case)?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: support constraint drop
2020-01-09 10:15 [Tarantool-patches] [PATCH] sql: support constraint drop Roman Khabibov
2020-01-13 17:00 ` Nikita Pettik
@ 2020-02-20 23:09 ` Vladislav Shpilevoy
2020-02-20 23:36 ` Nikita Pettik
2020-02-29 12:47 ` [Tarantool-patches] [PATCH v2 3/3] " Roman Khabibov
1 sibling, 2 replies; 13+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-20 23:09 UTC (permalink / raw)
To: Roman Khabibov, tarantool-patches, n.pettik
Hi! Thanks for the patch!
> Extend <ALTER TABLE> statement to drop table constraints by their
> names.
>
> Closes #4120
>
> @TarantoolBot document
> Title: Drop table constraints in SQL
> Now, it is possible to drop table constraints (PRIMARY KEY,
> UNIQUE, FOREIGN KEY, CHECK) using
> <ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
> by their names.
>
> For example:
>
> tarantool> box.execute([[CREATE TABLE test (
> a INTEGER PRIMARY KEY,
> b INTEGER,
> CONSTRAINT cnstr CHECK (a >= 0)
> );]])
> ---
> - row_count: 1
> ...
>
> tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
> ---
> - row_count: 1
> ...
>
> The same for all the other constraints.
>
1. Please, add a @ChangeLog record. Not to the commit message, but
in a separate mail. I guess, Kirill will search for '@ChangeLog'
in the mail history.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index d9bf8de91..76ad79350 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2052,35 +2052,78 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
> is_deferred;
> }
>
> +/**
> + * Emit code to drop the entry from _index or _ck_contstraint or
> + * _fk_constraint space corresponding with the constraint type.
> + */
> void
> -sql_drop_foreign_key(struct Parse *parse_context)
> +sql_drop_constraint(struct Parse *parse_context)
> {
> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
> + struct drop_entity_def *drop_def =
> + &parse_context->drop_constraint_def.base;
> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
> const char *table_name = drop_def->base.entity_name->a[0].zName;
> assert(table_name != NULL);
> - struct space *child = space_by_name(table_name);
> - if (child == NULL) {
> + struct space *space = space_by_name(table_name);
> + if (space == NULL) {
> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
> parse_context->is_aborted = true;
> return;
> }
> - char *constraint_name =
> - sql_name_from_token(parse_context->db, &drop_def->name);
> - if (constraint_name == NULL) {
> + char *name = sql_name_from_token(parse_context->db, &drop_def->name);
> + if (name == NULL) {
> + parse_context->is_aborted = true;
> + return;
> + }
> + struct constraint_id *id = space_find_constraint_id(space, name);
> + if (id == NULL) {
2. We perhaps need to do that at runtime - search in fk, ck, and index
spaces, or somehow in this hash table. Because otherwise we rely on
having struct space object, which in theory won't always be the case.
However, not in scope of this patch maybe. Because anyway we have
struct space used in lots of other places. And I don't know a general
solution how to get rid of all of them. Yet.
> + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
> parse_context->is_aborted = true;
> return;
> }
> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
> - child->def);
> - /*
> - * We account changes to row count only if drop of
> - * foreign keys take place in a separate
> - * ALTER TABLE DROP CONSTRAINT statement, since whole
> - * DROP TABLE always returns 1 (one) as a row count.
> - */
> struct Vdbe *v = sqlGetVdbe(parse_context);
> + assert(v != NULL);
> + assert(id->type < constraint_type_MAX);
> + switch (id->type) {
> + case CONSTRAINT_TYPE_PK:
> + case CONSTRAINT_TYPE_UNIQUE: {
> + uint32_t index_id = box_index_id_by_name(space->def->id, name,
> + strlen(name));
3. This is definitely not ok. It is not just looking at space *, it is a
select during parsing. _index has a unique index by space id and name, so
you can emit a deletion opcode without learning an index id. It shouldn't
be hard.
> + /*
> + * We have already verified, that this index
> + * exists, so we don't check index_id for
> + * BOX_ID_NIL.
> + */
4. You are going to need to do that when you will emit deletion opcode
by name. You can't check whether the index exists during compilation.
> + assert(index_id != BOX_ID_NIL);
> + int record_reg = ++parse_context->nMem;
> + int space_id_reg = ++parse_context->nMem;
> + int index_id_reg = ++parse_context->nMem;
> + sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
> + sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
> + const char *error_msg =
> + tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
> + id->name, space->def->name);
> + if (vdbe_emit_halt_with_presence_test(parse_context,
> + BOX_INDEX_ID, 0,
> + space_id_reg, 2,
> + ER_NO_SUCH_CONSTRAINT,
> + error_msg, false,
> + OP_Found) != 0)
> + return;
> + sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
> + sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
> + break;
> + }> diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
> index 163f4309c..12f673dac 100755
> --- a/test/sql/constraint.test.lua
> +++ b/test/sql/constraint.test.lua
> @@ -131,6 +131,28 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
> -- uniqueness, index names should be unique in a space.
> box.execute('CREATE UNIQUE INDEX d ON t2(i);')
>
> +--
> +-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
> +-- correctly for each type of constraint.
> +--
> +-- Drop UNIQUE constraint.
> +box.space.T2.index.E ~= nil
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +box.space.T2.index.E == nil
> +-- Drop PRIMARY KEY constraint named "C".
> +box.execute('DROP INDEX d ON t2;')
> +box.space.T2.index.C ~= nil
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
> +box.space.T2.index.C == nil
> +-- Drop CHECK constraint.
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
> +box.space.T2.ck_constraint.E ~= nil
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +box.space.T2.ck_constraint.E == nil
> +-- Drop FOREIGN KEY constraint.
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +
5. Please, add tests for deletion of not existing
constraints; for deletion of non-constraint objects
(non-unique index).
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: support constraint drop
2020-02-20 23:09 ` [Tarantool-patches] [PATCH] " Vladislav Shpilevoy
@ 2020-02-20 23:36 ` Nikita Pettik
2020-02-29 12:47 ` [Tarantool-patches] [PATCH v2 3/3] " Roman Khabibov
1 sibling, 0 replies; 13+ messages in thread
From: Nikita Pettik @ 2020-02-20 23:36 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 21 Feb 00:09, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> > - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
> > - child->def);
> > - /*
> > - * We account changes to row count only if drop of
> > - * foreign keys take place in a separate
> > - * ALTER TABLE DROP CONSTRAINT statement, since whole
> > - * DROP TABLE always returns 1 (one) as a row count.
> > - */
> > struct Vdbe *v = sqlGetVdbe(parse_context);
> > + assert(v != NULL);
> > + assert(id->type < constraint_type_MAX);
> > + switch (id->type) {
> > + case CONSTRAINT_TYPE_PK:
> > + case CONSTRAINT_TYPE_UNIQUE: {
> > + uint32_t index_id = box_index_id_by_name(space->def->id, name,
> > + strlen(name));
>
> 3. This is definitely not ok. It is not just looking at space *, it is a
> select during parsing. _index has a unique index by space id and name, so
> you can emit a deletion opcode without learning an index id. It shouldn't
> be hard.
Fair note. Then additional request from me: let's remove
box_index_id_by_name() from sql_drop_index() and pragma_index_info()
as well (in a separate patch ofc).
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop
2020-02-20 23:09 ` [Tarantool-patches] [PATCH] " Vladislav Shpilevoy
2020-02-20 23:36 ` Nikita Pettik
@ 2020-02-29 12:47 ` Roman Khabibov
2020-02-29 15:32 ` Vladislav Shpilevoy
1 sibling, 1 reply; 13+ messages in thread
From: Roman Khabibov @ 2020-02-29 12:47 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hi! Thanks for the review.
> On Feb 21, 2020, at 02:09, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Hi! Thanks for the patch!
>
>> Extend <ALTER TABLE> statement to drop table constraints by their
>> names.
>>
>> Closes #4120
>>
>> @TarantoolBot document
>> Title: Drop table constraints in SQL
>> Now, it is possible to drop table constraints (PRIMARY KEY,
>> UNIQUE, FOREIGN KEY, CHECK) using
>> <ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
>> by their names.
>>
>> For example:
>>
>> tarantool> box.execute([[CREATE TABLE test (
>> a INTEGER PRIMARY KEY,
>> b INTEGER,
>> CONSTRAINT cnstr CHECK (a >= 0)
>> );]])
>> ---
>> - row_count: 1
>> ...
>>
>> tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
>> ---
>> - row_count: 1
>> ...
>>
>> The same for all the other constraints.
>>
>
> 1. Please, add a @ChangeLog record. Not to the commit message, but
> in a separate mail. I guess, Kirill will search for '@ChangeLog'
> in the mail history.
>
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index d9bf8de91..76ad79350 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -2052,35 +2052,78 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
>> is_deferred;
>> }
>>
>> +/**
>> + * Emit code to drop the entry from _index or _ck_contstraint or
>> + * _fk_constraint space corresponding with the constraint type.
>> + */
>> void
>> -sql_drop_foreign_key(struct Parse *parse_context)
>> +sql_drop_constraint(struct Parse *parse_context)
>> {
>> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
>> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
>> + struct drop_entity_def *drop_def =
>> + &parse_context->drop_constraint_def.base;
>> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
>> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
>> const char *table_name = drop_def->base.entity_name->a[0].zName;
>> assert(table_name != NULL);
>> - struct space *child = space_by_name(table_name);
>> - if (child == NULL) {
>> + struct space *space = space_by_name(table_name);
>> + if (space == NULL) {
>> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
>> parse_context->is_aborted = true;
>> return;
>> }
>> - char *constraint_name =
>> - sql_name_from_token(parse_context->db, &drop_def->name);
>> - if (constraint_name == NULL) {
>> + char *name = sql_name_from_token(parse_context->db, &drop_def->name);
>> + if (name == NULL) {
>> + parse_context->is_aborted = true;
>> + return;
>> + }
>> + struct constraint_id *id = space_find_constraint_id(space, name);
>> + if (id == NULL) {
>
> 2. We perhaps need to do that at runtime - search in fk, ck, and index
> spaces, or somehow in this hash table. Because otherwise we rely on
> having struct space object, which in theory won't always be the case.
> which in theory won't always be the case.
Why? Can you, please, explain?
> However, not in scope of this patch maybe. Because anyway we have
> struct space used in lots of other places. And I don't know a general
> solution how to get rid of all of them. Yet.
Do you mean to avoid struct space usage within build.c?
>> + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
>> parse_context->is_aborted = true;
>> return;
>> }
>> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
>> - child->def);
>> - /*
>> - * We account changes to row count only if drop of
>> - * foreign keys take place in a separate
>> - * ALTER TABLE DROP CONSTRAINT statement, since whole
>> - * DROP TABLE always returns 1 (one) as a row count.
>> - */
>> struct Vdbe *v = sqlGetVdbe(parse_context);
>> + assert(v != NULL);
>> + assert(id->type < constraint_type_MAX);
>> + switch (id->type) {
>> + case CONSTRAINT_TYPE_PK:
>> + case CONSTRAINT_TYPE_UNIQUE: {
>> + uint32_t index_id = box_index_id_by_name(space->def->id, name,
>> + strlen(name));
>
> 3. This is definitely not ok. It is not just looking at space *, it is a
> select during parsing. _index has a unique index by space id and name, so
> you can emit a deletion opcode without learning an index id. It shouldn't
> be hard.
>
>> + /*
>> + * We have already verified, that this index
>> + * exists, so we don't check index_id for
>> + * BOX_ID_NIL.
>> + */
>
> 4. You are going to need to do that when you will emit deletion opcode
> by name. You can't check whether the index exists during compilation.
See the patch in the patch set.
>> + assert(index_id != BOX_ID_NIL);
>> + int record_reg = ++parse_context->nMem;
>> + int space_id_reg = ++parse_context->nMem;
>> + int index_id_reg = ++parse_context->nMem;
>> + sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
>> + sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
>> + const char *error_msg =
>> + tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
>> + id->name, space->def->name);
>> + if (vdbe_emit_halt_with_presence_test(parse_context,
>> + BOX_INDEX_ID, 0,
>> + space_id_reg, 2,
>> + ER_NO_SUCH_CONSTRAINT,
>> + error_msg, false,
>> + OP_Found) != 0)
>> + return;
>> + sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
>> + sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
>> + break;
>> + }> diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
>> index 163f4309c..12f673dac 100755
>> --- a/test/sql/constraint.test.lua
>> +++ b/test/sql/constraint.test.lua
>> @@ -131,6 +131,28 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
>> -- uniqueness, index names should be unique in a space.
>> box.execute('CREATE UNIQUE INDEX d ON t2(i);')
>>
>> +--
>> +-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
>> +-- correctly for each type of constraint.
>> +--
>> +-- Drop UNIQUE constraint.
>> +box.space.T2.index.E ~= nil
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
>> +box.space.T2.index.E == nil
>> +-- Drop PRIMARY KEY constraint named "C".
>> +box.execute('DROP INDEX d ON t2;')
>> +box.space.T2.index.C ~= nil
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
>> +box.space.T2.index.C == nil
>> +-- Drop CHECK constraint.
>> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
>> +box.space.T2.ck_constraint.E ~= nil
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
>> +box.space.T2.ck_constraint.E == nil
>> +-- Drop FOREIGN KEY constraint.
>> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
>> +
>
> 5. Please, add tests for deletion of not existing
> constraints; for deletion of non-constraint objects
> (non-unique index).
diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
index 163f4309c..a9fa4ccc9 100755
--- a/test/sql/constraint.test.lua
+++ b/test/sql/constraint.test.lua
@@ -131,6 +131,34 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
-- uniqueness, index names should be unique in a space.
box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop non-constraint object (non-unique index).
+box.execute('CREATE INDEX non_constraint ON t2(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_constraint;')
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.index.E == nil
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX non_constraint ON t2;')
+box.execute('DROP INDEX d ON t2;')
+box.space.T2.index.C ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+box.space.T2.index.C == nil
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT ck_constraint CHECK(i > 0);')
+box.space.T2.ck_constraint.CK_CONSTRAINT ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT ck_constraint;')
+box.space.T2.ck_constraint.CK_CONSTRAINT == nil
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT fk FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT fk;')
+-- Drop non-existing constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_existing_constraint;’)
+
sql: support constraint drop
Extend <ALTER TABLE> statement to drop table constraints by their
names.
Closes #4120
@TarantoolBot document
Title: Drop table constraints in SQL
Now, it is possible to drop table constraints (PRIMARY KEY,
UNIQUE, FOREIGN KEY, CHECK) using
<ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
by their names.
For example:
tarantool> box.execute([[CREATE TABLE test (
a INTEGER PRIMARY KEY,
b INTEGER,
CONSTRAINT cnstr CHECK (a >= 0)
);]])
---
- row_count: 1
...
tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
---
- row_count: 1
...
The same for all the other constraints.
---
src/box/constraint_id.h | 1 +
src/box/sql/build.c | 63 ++++++++++++++++++----------
src/box/sql/parse.y | 4 +-
src/box/sql/parse_def.h | 11 ++---
src/box/sql/sqlInt.h | 7 +++-
test/sql/constraint.result | 81 ++++++++++++++++++++++++++++++++++++
test/sql/constraint.test.lua | 28 +++++++++++++
7 files changed, 165 insertions(+), 30 deletions(-)
diff --git a/src/box/constraint_id.h b/src/box/constraint_id.h
index 21f067cdc..ed4f37426 100755
--- a/src/box/constraint_id.h
+++ b/src/box/constraint_id.h
@@ -40,6 +40,7 @@ enum constraint_type {
CONSTRAINT_TYPE_UNIQUE,
CONSTRAINT_TYPE_FK,
CONSTRAINT_TYPE_CK,
+ constraint_type_MAX,
};
extern const char *constraint_type_strs[];
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 00877b7d8..17fd07f78 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1508,8 +1508,6 @@ vdbe_emit_index_drop(struct Parse *parse_context, const char *name,
*
* @param parse_context Parsing context.
* @param constraint_name Name of FK constraint to be dropped.
- * Must be allocated on head by sqlDbMalloc().
- * It will be freed in VDBE.
* @param child_def Def of table which constraint belongs to.
*/
static void
@@ -1519,7 +1517,8 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
struct Vdbe *vdbe = sqlGetVdbe(parse_context);
assert(vdbe != NULL);
int key_reg = sqlGetTempRange(parse_context, 3);
- sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
+ const char *name_copy = sqlDbStrDup(parse_context->db, constraint_name);
+ sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, name_copy,
P4_DYNAMIC);
sqlVdbeAddOp2(vdbe, OP_Integer, child_def->id, key_reg + 1);
const char *error_msg =
@@ -1529,10 +1528,8 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
BOX_FK_CONSTRAINT_ID, 0,
key_reg, 2, ER_NO_SUCH_CONSTRAINT,
error_msg, false,
- OP_Found) != 0) {
- sqlDbFree(parse_context->db, constraint_name);
+ OP_Found) != 0)
return;
- }
sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
@@ -1689,11 +1686,7 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
struct fk_constraint *child_fk;
rlist_foreach_entry(child_fk, &space->child_fk_constraint,
in_child_space) {
-
- char *fk_name_dup = sqlDbStrDup(v->db, child_fk->def->name);
- if (fk_name_dup == NULL)
- return;
- vdbe_emit_fk_constraint_drop(parse_context, fk_name_dup,
+ vdbe_emit_fk_constraint_drop(parse_context, child_fk->def->name,
space->def);
}
/* Delete all CK constraints. */
@@ -2086,28 +2079,38 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
is_deferred;
}
+/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ */
void
-sql_drop_foreign_key(struct Parse *parse_context)
+sql_drop_constraint(struct Parse *parse_context)
{
- struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
- assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
+ struct drop_entity_def *drop_def =
+ &parse_context->drop_constraint_def.base;
+ assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
const char *table_name = drop_def->base.entity_name->a[0].zName;
assert(table_name != NULL);
- struct space *child = space_by_name(table_name);
- if (child == NULL) {
+ struct space *space = space_by_name(table_name);
+ if (space == NULL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
parse_context->is_aborted = true;
return;
}
- char *constraint_name =
- sql_name_from_token(parse_context->db, &drop_def->name);
- if (constraint_name == NULL) {
+ char *name = sql_normalized_name_region_new(&parse_context->region,
+ drop_def->name.z,
+ drop_def->name.n);
+ if (name == NULL) {
+ parse_context->is_aborted = true;
+ return;
+ }
+ struct constraint_id *id = space_find_constraint_id(space, name);
+ if (id == NULL) {
+ diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
parse_context->is_aborted = true;
return;
}
- vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
- child->def);
/*
* We account changes to row count only if drop of
* foreign keys take place in a separate
@@ -2115,6 +2118,24 @@ sql_drop_foreign_key(struct Parse *parse_context)
* DROP TABLE always returns 1 (one) as a row count.
*/
struct Vdbe *v = sqlGetVdbe(parse_context);
+ assert(v != NULL);
+ assert(id->type < constraint_type_MAX);
+ switch (id->type) {
+ case CONSTRAINT_TYPE_PK:
+ case CONSTRAINT_TYPE_UNIQUE: {
+ vdbe_emit_index_drop(parse_context, name, space->def,
+ ER_NO_SUCH_CONSTRAINT, false);
+ break;
+ }
+ case CONSTRAINT_TYPE_FK:
+ vdbe_emit_fk_constraint_drop(parse_context, name, space->def);
+ break;
+ case CONSTRAINT_TYPE_CK:
+ vdbe_emit_ck_constraint_drop(parse_context, name, space->def);
+ break;
+ default:
+ unreachable();
+ }
sqlVdbeCountChanges(v);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index cfe1c0012..1a0e89703 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1763,9 +1763,9 @@ cmd ::= alter_table_start(A) RENAME TO nm(N). {
}
cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
- drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
+ drop_constraint_def_init(&pParse->drop_constraint_def, X, &Z, false);
pParse->initiateTTrans = true;
- sql_drop_foreign_key(pParse);
+ sql_drop_constraint(pParse);
}
cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). {
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 2f433e4c0..cb0ecd2fc 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -265,7 +265,7 @@ struct drop_trigger_def {
struct drop_entity_def base;
};
-struct drop_fk_def {
+struct drop_constraint_def {
struct drop_entity_def base;
};
@@ -408,11 +408,12 @@ drop_trigger_def_init(struct drop_trigger_def *drop_trigger_def,
}
static inline void
-drop_fk_def_init(struct drop_fk_def *drop_fk_def, struct SrcList *parent_name,
- struct Token *name, bool if_exist)
+drop_constraint_def_init(struct drop_constraint_def *drop_constraint_def,
+ struct SrcList *parent_name, struct Token *name,
+ bool if_exist)
{
- drop_entity_def_init(&drop_fk_def->base, parent_name, name, if_exist,
- ENTITY_TYPE_FK);
+ drop_entity_def_init(&drop_constraint_def->base, parent_name, name,
+ if_exist, ENTITY_TYPE_CONSTRAINT);
}
static inline void
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d1fcf4761..1579cc92e 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2237,7 +2237,7 @@ struct Parse {
struct create_trigger_def create_trigger_def;
struct create_view_def create_view_def;
struct rename_entity_def rename_entity_def;
- struct drop_fk_def drop_fk_def;
+ struct drop_constraint_def drop_constraint_def;
struct drop_index_def drop_index_def;
struct drop_table_def drop_table_def;
struct drop_trigger_def drop_trigger_def;
@@ -3741,13 +3741,16 @@ void
sql_create_foreign_key(struct Parse *parse_context);
/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ *
* Function called from parser to handle
* <ALTER TABLE table DROP CONSTRAINT constraint> SQL statement.
*
* @param parse_context Parsing context.
*/
void
-sql_drop_foreign_key(struct Parse *parse_context);
+sql_drop_constraint(struct Parse *parse_context);
/**
* Now our SQL implementation can't operate on spaces which
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
index 1585c2327..bcdc46a1c 100644
--- a/test/sql/constraint.result
+++ b/test/sql/constraint.result
@@ -291,6 +291,87 @@ box.execute('CREATE UNIQUE INDEX d ON t2(i);')
| - Index 'D' already exists in space 'T2'
| ...
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop non-constraint object (non-unique index).
+box.execute('CREATE INDEX non_constraint ON t2(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_constraint;')
+ | ---
+ | - null
+ | - Constraint 'NON_CONSTRAINT' does not exist in space 'T2'
+ | ...
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX non_constraint ON t2;')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('DROP INDEX d ON t2;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C == nil
+ | ---
+ | - true
+ | ...
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT ck_constraint CHECK(i > 0);')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.CK_CONSTRAINT ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT ck_constraint;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.CK_CONSTRAINT == nil
+ | ---
+ | - true
+ | ...
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT fk FOREIGN KEY(i) REFERENCES t1(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT fk;')
+ | ---
+ | - row_count: 1
+ | ...
+-- Drop non-existing constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_existing_constraint;')
+ | ---
+ | - null
+ | - Constraint 'NON_EXISTING_CONSTRAINT' does not exist in space 'T2'
+ | ...
+
--
-- Cleanup.
--
diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
index 163f4309c..a9fa4ccc9 100755
--- a/test/sql/constraint.test.lua
+++ b/test/sql/constraint.test.lua
@@ -131,6 +131,34 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
-- uniqueness, index names should be unique in a space.
box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop non-constraint object (non-unique index).
+box.execute('CREATE INDEX non_constraint ON t2(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_constraint;')
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.index.E == nil
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX non_constraint ON t2;')
+box.execute('DROP INDEX d ON t2;')
+box.space.T2.index.C ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+box.space.T2.index.C == nil
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT ck_constraint CHECK(i > 0);')
+box.space.T2.ck_constraint.CK_CONSTRAINT ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT ck_constraint;')
+box.space.T2.ck_constraint.CK_CONSTRAINT == nil
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT fk FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT fk;')
+-- Drop non-existing constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_existing_constraint;')
+
--
-- Cleanup.
--
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop
2020-02-29 12:47 ` [Tarantool-patches] [PATCH v2 3/3] " Roman Khabibov
@ 2020-02-29 15:32 ` Vladislav Shpilevoy
2020-03-03 10:13 ` Roman Khabibov
0 siblings, 1 reply; 13+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-29 15:32 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches
Thanks for the patch!
>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>> index d9bf8de91..76ad79350 100644
>>> --- a/src/box/sql/build.c
>>> +++ b/src/box/sql/build.c
>>> @@ -2052,35 +2052,78 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
>>> is_deferred;
>>> }
>>>
>>> +/**
>>> + * Emit code to drop the entry from _index or _ck_contstraint or
>>> + * _fk_constraint space corresponding with the constraint type.
>>> + */
>>> void
>>> -sql_drop_foreign_key(struct Parse *parse_context)
>>> +sql_drop_constraint(struct Parse *parse_context)
>>> {
>>> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
>>> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
>>> + struct drop_entity_def *drop_def =
>>> + &parse_context->drop_constraint_def.base;
>>> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
>>> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
>>> const char *table_name = drop_def->base.entity_name->a[0].zName;
>>> assert(table_name != NULL);
>>> - struct space *child = space_by_name(table_name);
>>> - if (child == NULL) {
>>> + struct space *space = space_by_name(table_name);
>>> + if (space == NULL) {
>>> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
>>> parse_context->is_aborted = true;
>>> return;
>>> }
>>> - char *constraint_name =
>>> - sql_name_from_token(parse_context->db, &drop_def->name);
>>> - if (constraint_name == NULL) {
>>> + char *name = sql_name_from_token(parse_context->db, &drop_def->name);
>>> + if (name == NULL) {
>>> + parse_context->is_aborted = true;
>>> + return;
>>> + }
>>> + struct constraint_id *id = space_find_constraint_id(space, name);
>>> + if (id == NULL) {
>>
>> 2. We perhaps need to do that at runtime - search in fk, ck, and index
>> spaces, or somehow in this hash table. Because otherwise we rely on
>> having struct space object, which in theory won't always be the case.
>> which in theory won't always be the case.
> Why? Can you, please, explain?
Remote client nodes do not have any schema objects. If a request
requires struct space *, it can't be compiled on the client. And
this is where we are going to.
>> However, not in scope of this patch maybe. Because anyway we have
>> struct space used in lots of other places. And I don't know a general
>> solution how to get rid of all of them. Yet.
> Do you mean to avoid struct space usage within build.c?
Yes, I want us to avoid any schema objects usage where possible.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 00877b7d8..17fd07f78 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1519,7 +1517,8 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
constraint_name can become 'const' now.
> struct Vdbe *vdbe = sqlGetVdbe(parse_context);
> assert(vdbe != NULL);
> int key_reg = sqlGetTempRange(parse_context, 3);
> - sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
> + const char *name_copy = sqlDbStrDup(parse_context->db, constraint_name);
> + sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, name_copy,
> P4_DYNAMIC);
> sqlVdbeAddOp2(vdbe, OP_Integer, child_def->id, key_reg + 1);
> const char *error_msg =
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop
2020-02-29 15:32 ` Vladislav Shpilevoy
@ 2020-03-03 10:13 ` Roman Khabibov
0 siblings, 0 replies; 13+ messages in thread
From: Roman Khabibov @ 2020-03-03 10:13 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hi! Thanks for the review.
> On Feb 29, 2020, at 18:32, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Thanks for the patch!
>
>>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>>> index d9bf8de91..76ad79350 100644
>>>> --- a/src/box/sql/build.c
>>>> +++ b/src/box/sql/build.c
>>>> @@ -2052,35 +2052,78 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
>>>> is_deferred;
>>>> }
>>>>
>>>> +/**
>>>> + * Emit code to drop the entry from _index or _ck_contstraint or
>>>> + * _fk_constraint space corresponding with the constraint type.
>>>> + */
>>>> void
>>>> -sql_drop_foreign_key(struct Parse *parse_context)
>>>> +sql_drop_constraint(struct Parse *parse_context)
>>>> {
>>>> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
>>>> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
>>>> + struct drop_entity_def *drop_def =
>>>> + &parse_context->drop_constraint_def.base;
>>>> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
>>>> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
>>>> const char *table_name = drop_def->base.entity_name->a[0].zName;
>>>> assert(table_name != NULL);
>>>> - struct space *child = space_by_name(table_name);
>>>> - if (child == NULL) {
>>>> + struct space *space = space_by_name(table_name);
>>>> + if (space == NULL) {
>>>> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
>>>> parse_context->is_aborted = true;
>>>> return;
>>>> }
>>>> - char *constraint_name =
>>>> - sql_name_from_token(parse_context->db, &drop_def->name);
>>>> - if (constraint_name == NULL) {
>>>> + char *name = sql_name_from_token(parse_context->db, &drop_def->name);
>>>> + if (name == NULL) {
>>>> + parse_context->is_aborted = true;
>>>> + return;
>>>> + }
>>>> + struct constraint_id *id = space_find_constraint_id(space, name);
>>>> + if (id == NULL) {
>>>
>>> 2. We perhaps need to do that at runtime - search in fk, ck, and index
>>> spaces, or somehow in this hash table. Because otherwise we rely on
>>> having struct space object, which in theory won't always be the case.
>>> which in theory won't always be the case.
>> Why? Can you, please, explain?
>
> Remote client nodes do not have any schema objects. If a request
> requires struct space *, it can't be compiled on the client. And
> this is where we are going to.
>
>>> However, not in scope of this patch maybe. Because anyway we have
>>> struct space used in lots of other places. And I don't know a general
>>> solution how to get rid of all of them. Yet.
>> Do you mean to avoid struct space usage within build.c?
>
> Yes, I want us to avoid any schema objects usage where possible.
>
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 00877b7d8..17fd07f78 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -1519,7 +1517,8 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
>
> constraint_name can become 'const' now.
Done.
>> struct Vdbe *vdbe = sqlGetVdbe(parse_context);
>> assert(vdbe != NULL);
>> int key_reg = sqlGetTempRange(parse_context, 3);
>> - sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
>> + const char *name_copy = sqlDbStrDup(parse_context->db, constraint_name);
>> + sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, name_copy,
>> P4_DYNAMIC);
>> sqlVdbeAddOp2(vdbe, OP_Integer, child_def->id, key_reg + 1);
>> const char *error_msg =
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-03-03 10:13 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-09 10:15 [Tarantool-patches] [PATCH] sql: support constraint drop Roman Khabibov
2020-01-13 17:00 ` Nikita Pettik
2020-01-24 14:21 ` [Tarantool-patches] [PATCH 2/2] " Roman Khabibov
2020-01-28 17:39 ` Nikita Pettik
2020-02-01 17:36 ` Roman Khabibov
2020-02-11 16:56 ` Nikita Pettik
2020-02-16 10:24 ` Roman Khabibov
2020-02-20 19:55 ` Nikita Pettik
2020-02-20 23:09 ` [Tarantool-patches] [PATCH] " Vladislav Shpilevoy
2020-02-20 23:36 ` Nikita Pettik
2020-02-29 12:47 ` [Tarantool-patches] [PATCH v2 3/3] " Roman Khabibov
2020-02-29 15:32 ` Vladislav Shpilevoy
2020-03-03 10:13 ` Roman Khabibov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox