[tarantool-patches] Re: [PATCH 5/6] sql: don't add string of 'CREATE INDEX ...' to index opts
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Dec 10 17:18:53 MSK 2018
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 at 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
More information about the Tarantool-patches
mailing list