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

[Roman Khabibov <roman.habibov@tarantool.org>]

Hi! Thanks for the review.

> On Feb 29, 2020, at 18:32, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> 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.
Done.
>> 	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 =