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

[Nikita Pettik <korablev@tarantool.org>]

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