[Tarantool-patches] [PATCH 2/2] sql: support constraint drop
Roman Khabibov
roman.habibov at tarantool.org
Fri Jan 24 17:21:36 MSK 2020
Hi! Thanks for the review.
> On Jan 13, 2020, at 20:00, Nikita Pettik <korablev at 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 at 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.
--
More information about the Tarantool-patches
mailing list