Re: [Tarantool-patches] [PATCH] sql: support constraint drop

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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).