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

[Nikita Pettik <korablev@tarantool.org>]

On 01 Feb 20:36, Roman Khabibov wrote:
> Hi! Thanks for the review.
> 
> >>>> 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.

In sql_drop_constraint() in index dropping branch there's still no
vdbe_emit_halt_with_presence_test() call. Please add it there as well.
 
> 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.
> + */

As a rule we place comments near function declaration.

>  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);
> +	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.
>  	 */

This comment doesn't look like related to this function:
sql_drop_constraint() is never called within drop table statement.

> -	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.
> -	 */

Why did you drop this comment?

>  	ENTITY_TYPE_CONSTRAINT,