[tarantool-patches] Re: [PATCH v2 2/2] sql: remove struct Enc
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Aug 28 04:43:00 MSK 2018
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
More information about the Tarantool-patches
mailing list