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

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

Thanks for the patch!

>>> 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.
>> which in theory won't always be the case.
> Why? Can you, please, explain?

Remote client nodes do not have any schema objects. If a request
requires struct space *, it can't be compiled on the client. And
this is where we are going to.

>> 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.
> Do you mean to avoid struct space usage within build.c?

Yes, I want us to avoid any schema objects usage where possible.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 00877b7d8..17fd07f78 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1519,7 +1517,8 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,

constraint_name can become 'const' now.

>  	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
>  	assert(vdbe != NULL);
>  	int key_reg = sqlGetTempRange(parse_context, 3);
> -	sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
> +	const char *name_copy = sqlDbStrDup(parse_context->db, constraint_name);
> +	sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, name_copy,
>  			  P4_DYNAMIC);
>  	sqlVdbeAddOp2(vdbe, OP_Integer, child_def->id, key_reg + 1);
>  	const char *error_msg =