[Tarantool-patches] [PATCH 2/2] sql: support constraint drop

Roman Khabibov roman.habibov at tarantool.org
Sat Feb 1 20:36:33 MSK 2020


Hi! Thanks for the review.

> On Jan 28, 2020, at 20:39, Nikita Pettik <korablev at 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 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.
> 
> 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 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/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.
 --



More information about the Tarantool-patches mailing list