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

[Nikita Pettik <korablev@tarantool.org>]

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;
> +	}
>