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

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

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.

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

>   		sqlite3VdbeAddOp0(vdbe, OP_Expire);
>   	}
>   

My review fixes here and on the branch:

=================================================================

commit 65020d36bb3a9c8058789c90ed95665bd847a732
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Mon Dec 10 17:11:26 2018 +0300

     Review fixes

diff --git a/src/box/sql.c b/src/box/sql.c
index d270529ae..dd7a7ca4b 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1188,25 +1188,18 @@ char *
  sql_encode_index_opts(struct region *region, const struct index_opts *opts,
  		      uint32_t *size)
  {
-	size_t used = region_used(region);
-	struct mpstream stream;
-	bool is_error = false;
-	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
-		      set_encode_error, &is_error);
-	mpstream_encode_map(&stream, 1);
-	mpstream_encode_str(&stream, "unique");
-	mpstream_encode_bool(&stream, opts->is_unique);;
-	mpstream_flush(&stream);
-	if (is_error) {
-		diag_set(OutOfMemory, stream.pos - stream.buf, "mpstream_flush",
-			"stream");
+	int unique_len = strlen("unique");
+	*size = mp_sizeof_map(1) + mp_sizeof_str(unique_len) +
+		mp_sizeof_bool(opts->is_unique);
+	char *mem = (char *) region_alloc(region, *size);
+	if (mem == NULL) {
+		diag_set(OutOfMemory, *size, "region_alloc", "mem");
  		return NULL;
  	}
-	*size = region_used(region) - used;
-	char *raw = region_join(region, *size);
-	if (raw == NULL)
-		diag_set(OutOfMemory, *size, "region_join", "raw");
-	return raw;
+	char *pos = mp_encode_map(mem, 1);
+	pos = mp_encode_str(pos, "unique", unique_len);
+	pos = mp_encode_bool(pos, opts->is_unique);
+	return mem;
  }
  
  void
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index ab2a56420..50227232c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2125,7 +2125,6 @@ table_add_index(struct space *space, struct index *index)
   * @param idx_type Index type: non-unique index, unique index,
   *                 index implementing UNIQUE constraint or
   *                 index implementing PK constraint.
- * @param sql_stmt SQL statement, which creates the index.
   * @retval 0 on success, -1 on error.
   */
  static int