[tarantool-patches] Re: [PATCH v2 2/2] sql: remove struct Enc

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

Thanks for the patch! See 7 comments below and a
commit on the branch.

On 27/08/2018 08:11, Kirill Shcherbatov wrote:
> We shouldn't use legacy Enc structure in SQL as we have
> featured class mpstream in tarantool core. With this path
> all allocations temporally commited with mpstream initialized
> on region and then duplicated in dynamic persistent memory via
> sqlite3DbMallocRaw.
> 
> Closes #3545.
> ---
>   src/box/sql.c              | 473 +++++++++++++++++++++++----------------------
>   src/box/sql/build.c        | 156 ++++++++-------
>   src/box/sql/tarantoolInt.h |  80 +++++---
>   src/box/sql/trigger.c      |  62 +++---
>   4 files changed, 416 insertions(+), 355 deletions(-)
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index b98593b..6bf832b 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -56,6 +56,8 @@
>   #include "xrow.h"
>   #include "iproto_constants.h"
>   #include "fkey.h"
> +#include "mpstream.h"
> +#include "small/region.h"

1. Double include of region.h.

>   
>   static sqlite3 *db = NULL;
>   
> @@ -840,6 +842,32 @@ rename_fail:
>   	return SQL_TARANTOOL_ERROR;
>   }
>   
> +/** Callback to forward and error from mpstream methods. */
> +static void
> +set_encode_error(void *error_ctx)
> +{
> +	*(bool *)error_ctx = true;
> +}
> +
> +void
> +mpstream_encode_index_opts(struct mpstream *stream, struct index_opts *opts)

2. Static.

3. No comment.

> +{
> +	mpstream_encode_map(stream, 2);
> +	/* Mark as unique pk and unique indexes */

4. Finish a sentence with the dot. Btw the comment
is not related to the code. Below you do not whether
the index is unique or not.

> +	mpstream_encode_str(stream, "unique");
> +	/* If user didn't defined ON CONFLICT OPTIONS, all uniqueness checks
> +	 * will be made by Tarantool. However, Tarantool doesn't have ON
> +	 * CONFLIT option, so in that case (except ON CONFLICT ABORT, which is
> +	 * default behavior) uniqueness will be checked by SQL.
> +	 * INSERT OR REPLACE/IGNORE uniqueness checks will be also done by
> +	 * Tarantool.
> +	 */

5. "Did't defined" - corrupted phrase. Rather "did't define".
But the whole comment looks strange as well as the previous,
and out of its place. Just remove it.

> +	mpstream_encode_bool(stream, opts->is_unique);
> +	mpstream_encode_str(stream, "sql");
> +	mpstream_encode_strn(stream, opts->sql,
> +			     opts->sql != NULL ? strlen(opts->sql) : 0);> +}
> +
>   int
>   sql_index_update_table_name(struct index_def *def, const char *new_tbl_name,
>   			    char **sql_stmt)
> @@ -851,33 +879,43 @@ sql_index_update_table_name(struct index_def *def, const char *new_tbl_name,
>   	if (*sql_stmt == NULL)
>   		return -1;
>   
> -	uint32_t key_len = mp_sizeof_uint(def->space_id) +
> -			   mp_sizeof_uint(def->iid) + mp_sizeof_array(2);
> +	struct region *region = &fiber()->gc;
> +	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);
> +
> +	/* Encode key. */
> +	mpstream_encode_array(&stream, 2);
> +	mpstream_encode_uint(&stream, def->space_id);
> +	mpstream_encode_uint(&stream, def->iid);
> +
> +	/* Encode op. */
> +	uint32_t op_offset = stream.pos - stream.buf;
> +	mpstream_encode_array(&stream, 1);
> +	mpstream_encode_array(&stream, 3);
> +	mpstream_encode_str(&stream, "=");
> +	mpstream_encode_uint(&stream, BOX_INDEX_FIELD_OPTS);
> +
> +	/* Encode index opts. */
>   	struct index_opts opts = def->opts;
>   	opts.sql = *sql_stmt;
> -	uint32_t new_opts_sz = sql_encode_index_opts(&opts, NULL);
> -	uint32_t op_sz = mp_sizeof_array(1) + mp_sizeof_array(3) +
> -			 mp_sizeof_str(1) + mp_sizeof_uint(4) + new_opts_sz;
> +	mpstream_encode_index_opts(&stream, &opts);
>   
> -	char *key_begin = (char*) region_alloc(&fiber()->gc, key_len + op_sz);
> -	if (key_begin == NULL) {
> -		diag_set(OutOfMemory, key_len + op_sz, "region_alloc",
> -			 "key_begin");
> +	mpstream_flush(&stream);
> +	if (is_error) {
> +		diag_set(OutOfMemory, stream.pos - stream.buf,
> +			 "mpstream_flush", "stream");
>   		return -1;
>   	}
> -	char *key = mp_encode_array(key_begin, 2);
> -	key = mp_encode_uint(key, def->space_id);
> -	key = mp_encode_uint(key, def->iid);
> -
> -	char *op_begin = key;
> -	char *op = mp_encode_array(op_begin, 1);
> -	op = mp_encode_array(op, 3);
> -	op = mp_encode_str(op, "=", 1);
> -	op = mp_encode_uint(op, BOX_INDEX_FIELD_OPTS);
> -	op += sql_encode_index_opts(&opts, op);
> -
> -	if (box_update(BOX_INDEX_ID, 0, key_begin, key, op_begin, op,
> -		       0, NULL) != 0)
> +	size_t sz = region_used(region) - used;
> +	char *raw = region_join(region, sz);
> +	if (raw == NULL)
> +		diag_set(OutOfMemory, sz, "region_join", "raw");
> +
> +	if (box_update(BOX_INDEX_ID, 0, raw, raw + op_offset, raw + op_offset,

6. If 'raw == NULL' you still try to use it in box_update.

> +			raw + sz, 0, NULL) != 0)
>   		return -1;
>   
>   	return 0;

7. Here https://github.com/tarantool/tarantool/commit/0d8b0c5641ec4c27547b1d0121bb1e0a641b149b
I see changes not relevant to this patch and not presented in the
email. I paste them bellow:

> diff --git a/src/mpstream.c b/src/mpstream.c
> index 23d27f9e5..44c2d9213 100644
> --- a/src/mpstream.c
> +++ b/src/mpstream.c
> @@ -176,12 +176,6 @@ mpstream_encode_strn(struct mpstream *stream, const char *str, uint32_t len)
>      mpstream_advance(stream, pos - data);
>  }
>  
> -void
> -mpstream_encode_str(struct mpstream *stream, const char *str)
> -{
> -    mpstream_encode_strn(stream, str, strlen(str));
> -}
> -
>  void
>  mpstream_encode_nil(struct mpstream *stream)
>  {
> diff --git a/src/mpstream.h b/src/mpstream.h
> index 789bd741d..6f9462d4c 100644
> --- a/src/mpstream.h
> +++ b/src/mpstream.h
> @@ -108,8 +108,11 @@ mpstream_encode_double(struct mpstream *stream, double num);
>  void
>  mpstream_encode_strn(struct mpstream *stream, const char *str, uint32_t len);
>  
> -void
> -mpstream_encode_str(struct mpstream *stream, const char *str);
> +static inline void
> +mpstream_encode_str(struct mpstream *stream, const char *str)
> +{
> +	mpstream_encode_strn(stream, str, strlen(str));
> +}
>  
>  void
>  mpstream_encode_nil(struct mpstream *stream);

Obviously, it should not be part of this comment, but
of the previous one.


Below and on the branch you can find my fixes:

diff --git a/src/box/sql.c b/src/box/sql.c
index 6bf832bd7..d1b935c14 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -57,7 +57,6 @@
  #include "iproto_constants.h"
  #include "fkey.h"
  #include "mpstream.h"
-#include "small/region.h"
  
  static sqlite3 *db = NULL;
  
@@ -911,14 +910,12 @@ sql_index_update_table_name(struct index_def *def, const char *new_tbl_name,
  	}
  	size_t sz = region_used(region) - used;
  	char *raw = region_join(region, sz);
-	if (raw == NULL)
+	if (raw == NULL) {
  		diag_set(OutOfMemory, sz, "region_join", "raw");
-
-	if (box_update(BOX_INDEX_ID, 0, raw, raw + op_offset, raw + op_offset,
-			raw + sz, 0, NULL) != 0)
  		return -1;
-
-	return 0;
+	}
+	return box_update(BOX_INDEX_ID, 0, raw, raw + op_offset,
+			  raw + op_offset, raw + sz, 0, NULL);
  }
  
  int