Re: [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing

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

> On Feb 29, 2020, at 18:31, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Thanks for the patch!
> 
> See 2 comments below.
> 
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index d9bf8de91..00877b7d8 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -47,7 +47,6 @@
>> #include "sqlInt.h"
>> #include "vdbeInt.h"
>> #include "tarantoolInt.h"
>> -#include "box/box.h"
>> #include "box/ck_constraint.h"
>> #include "box/fk_constraint.h"
>> #include "box/sequence.h"
>> @@ -1469,6 +1468,41 @@ 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 from _index space.
>> + *
>> + * @param parse_context Parsing context.
>> + * @param name Index name.
>> + *        It will be freed in VDBE.
> 
> 1. This easily fits into one line. Also I don't understand what does
> it mean 'freed in VDBE'. From what I see, this function has nothing
> to do with freeing 'name' argument, as well as VDBE. It is freed
> by the caller in the end of sql_drop_index().
Oops. This line turned out to be a case here.

>> + * @param space_def Def of table which index belongs to.
>> + * @param errcode Type of printing error: "no such index" or
>> +		  "no such constraint".
>> + * @param if_exist True if there was <IF EXISTS> in the query.
>> + */
>> +static void
>> +vdbe_emit_index_drop(struct Parse *parse_context, const char *name,
>> +		     struct space_def *space_def, int errcode, bool if_exist)
>> +{
>> +	assert(errcode == ER_NO_SUCH_INDEX_NAME ||
>> +	       errcode == ER_NO_SUCH_CONSTRAINT);
>> +	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
>> +	assert(vdbe != NULL);
>> +	assert(parse_context->db != NULL);
>> +	int key_reg = sqlGetTempRange(parse_context, 3);
>> +	sqlVdbeAddOp2(vdbe, OP_Integer, space_def->id, key_reg);
>> +	sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg + 1, 0,
>> +		      sqlDbStrDup(parse_context->db, name), P4_DYNAMIC);
>> +	const char *error_msg =
>> +		tt_sprintf(tnt_errcode_desc(errcode), name, space_def->name);
>> +	if (vdbe_emit_halt_with_presence_test(parse_context, BOX_INDEX_ID, 2,
>> +					      key_reg, 2, errcode, error_msg,
>> +					      if_exist, OP_Found) != 0)
>> +		return;
>> +	sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
>> +	sqlVdbeAddOp3(vdbe, OP_SDelete, BOX_INDEX_ID, key_reg + 2, 2);
>> +	sqlReleaseTempRange(parse_context, key_reg, 3);
>> +}
>> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
>> index a77bf4b16..c15f2e0d1 100644
>> --- a/src/box/sql/pragma.c
>> +++ b/src/box/sql/pragma.c
>> @@ -192,12 +191,9 @@ sql_pragma_index_info(struct Parse *parse,
>> 	struct space *space = space_by_name(tbl_name);
>> 	if (space == NULL)
>> 		return;
>> -	uint32_t iid = box_index_id_by_name(space->def->id, idx_name,
>> -					    strlen(idx_name));
>> -	if (iid == BOX_ID_NIL)
>> +	struct index *idx = space_index_by_name(space, idx_name);
>> +	if (idx == NULL)
>> 		return;
>> -	struct index *idx = space_index(space, iid);
>> -	assert(idx != NULL);
> 
> 2. Yeah, ok, good enough. The final goal should be to stop using
> even struct space pointer here. Sometime later.
> 
>> 	parse->nMem = 6;
>> 	struct Vdbe *v = sqlGetVdbe(parse);
>> 	assert(v != NULL);