[tarantool-patches] Re: [PATCH 5/6] sql: don't add string of 'CREATE INDEX ...' to index opts

[n.pettik]

> On 10 Dec 2018, at 17:18, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> 
> Thanks for the patch! See 2 comments and review fixes below.
> 
> On 10/12/2018 00:30, Nikita Pettik wrote:
>> Part of #2647
>> ---
>>  src/box/sql.c       | 22 +++-------------------
>>  src/box/sql/build.c | 35 +++--------------------------------
>>  2 files changed, 6 insertions(+), 51 deletions(-)
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index e79265823..45c539ec7 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -1214,7 +1196,9 @@ sql_encode_index_opts(struct region *region, const struct index_opts *opts,
>>  	bool is_error = false;
>>  	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
>>  		      set_encode_error, &is_error);
>> -	mpstream_encode_index_opts(&stream, opts);
>> +	mpstream_encode_map(&stream, 1);
>> +	mpstream_encode_str(&stream, "unique");
>> +	mpstream_encode_bool(&stream, opts->is_unique);;
>>  	mpstream_flush(&stream);
> 
> 1. We do not need mpstream for this tiny piece of code. I
> refactored it to simple region_alloc.

Ok, I don’t mind.

>>  	if (is_error) {
>>  		diag_set(OutOfMemory, stream.pos - stream.buf, "mpstream_flush",
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index fd1666c6d..ab2a56420 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -2546,6 +2527,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
>>  		sqlite3VdbeAddOp1(vdbe, OP_Close, cursor);
>>  		vdbe_emit_create_index(parse, def, index->def,
>>  				       def->id, index_id);
>> +		sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
> 
> 2. Are you sure that you should increment change counter here?
> As I know, it should be incremented once on a DDL, but this
> increment is under 'if (tbl_name != NULL)' which means that it
> is not a separate index, but part of CREATE TABLE, it is not?
> CREATE TABLE increments change counter on insertion into _space
> only.

tbl_name != NULL means that currently we are processing
CREATE INDEX statement, not CREATE TABLE. So, its ok.

> 
>>  		sqlite3VdbeAddOp0(vdbe, OP_Expire);
>>  	}
>>  
> 
> My review fixes here and on the branch:

Applied.