* [Tarantool-patches] [PATCH v2 1/2] sql: specify field types in ephemeral space format
2020-04-12 16:29 [Tarantool-patches] [PATCH v2 0/2] sql: fix order of inserted rows imeevma
@ 2020-04-12 16:29 ` imeevma
2020-04-13 22:47 ` Nikita Pettik
2020-04-12 16:29 ` [Tarantool-patches] [PATCH v2 2/2] sql: do not change order of inserted values imeevma
2020-04-13 22:35 ` [Tarantool-patches] [PATCH v2 0/2] sql: fix order of inserted rows Nikita Pettik
2 siblings, 1 reply; 6+ messages in thread
From: imeevma @ 2020-04-12 16:29 UTC (permalink / raw)
To: korablev, v.shpilevoy, tsafin, tarantool-patches
This patch allows to specify field types in ephemeral space format.
Prior to this patch, all fields had a SCALAR field type.
This patch allows us to not use the primary index to obtain field
types, since now the ephemeral space has field types in the
format. This allows us to change the structure of the primary
index, which helps to solve the issue #4256. In addition, since we
can now set the field types of the ephemeral space, we can use
this feature to set the field types according to the left value of
the IN operator. This will fix issue #4692.
Needed for #4256
Needed for #4692
Closes #3841
---
src/box/space_def.c | 5 ++---
src/box/space_def.h | 3 ++-
src/box/sql.c | 52 +++++++++++++++++++++++++++++++++++---------------
src/box/tuple_format.c | 22 ++++++++++-----------
4 files changed, 51 insertions(+), 31 deletions(-)
diff --git a/src/box/space_def.c b/src/box/space_def.c
index 6611312..5af0f38 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -233,7 +233,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
}
struct space_def*
-space_def_new_ephemeral(uint32_t field_count)
+space_def_new_ephemeral(uint32_t field_count, struct field_def *fields)
{
struct space_opts opts = space_opts_default;
opts.is_temporary = true;
@@ -242,8 +242,7 @@ space_def_new_ephemeral(uint32_t field_count)
"ephemeral",
strlen("ephemeral"),
"memtx", strlen("memtx"),
- &opts, &field_def_default,
- 0);
+ &opts, fields, field_count);
return space_def;
}
diff --git a/src/box/space_def.h b/src/box/space_def.h
index ac6d226..61bbdb4 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -173,11 +173,12 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
/**
* Create a new ephemeral space definition.
* @param field_count Number of fields in the space.
+ * @param fields Field definitions.
*
* @retval Space definition.
*/
struct space_def *
-space_def_new_ephemeral(uint32_t field_count);
+space_def_new_ephemeral(uint32_t field_count, struct field_def *fields);
/**
* Size of the space_def.
diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df8..e4434b2 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -330,13 +330,41 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
return NULL;
}
- struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc,
- sizeof(*ephemer_key_parts) * field_count);
- if (ephemer_key_parts == NULL) {
- diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count,
- "region", "key parts");
- return NULL;
+ struct region *region = &fiber()->gc;
+ /*
+ * Name of the fields will be "_COLUMN_1", "_COLUMN_2"
+ * and so on. Since number of columns no more than 2000,
+ * length of each name is no more than strlen("_COLUMN_")
+ * + 5.
+ */
+ assert(SQL_MAX_COLUMN <= 2000);
+ uint32_t name_len = strlen("_COLUMN_") + 5;
+ uint32_t size = field_count * (sizeof(struct field_def) + name_len +
+ sizeof(struct key_part_def));
+ struct field_def *fields = region_alloc(region, size);
+ struct key_part_def *ephemer_key_parts = (void *)fields +
+ field_count * sizeof(struct field_def);
+ char *names = (char *)ephemer_key_parts +
+ field_count * sizeof(struct key_part_def);
+ for (uint32_t i = 0; i < field_count; ++i) {
+ struct field_def *field = &fields[i];
+ field->name = names;
+ names += name_len;
+ sprintf(field->name, "_COLUMN_%d", i);
+ field->is_nullable = true;
+ field->nullable_action = ON_CONFLICT_ACTION_NONE;
+ field->default_value = NULL;
+ field->default_value_expr = NULL;
+ if (def != NULL && i < def->part_count) {
+ assert(def->parts[i].type < field_type_MAX);
+ field->type = def->parts[i].type;
+ field->coll_id = def->parts[i].coll_id;
+ } else {
+ field->coll_id = COLL_NONE;
+ field->type = FIELD_TYPE_SCALAR;
+ }
}
+
for (uint32_t i = 0; i < field_count; ++i) {
struct key_part_def *part = &ephemer_key_parts[i];
part->fieldno = i;
@@ -344,14 +372,8 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
part->is_nullable = true;
part->sort_order = SORT_ORDER_ASC;
part->path = NULL;
- if (def != NULL && i < def->part_count) {
- assert(def->parts[i].type < field_type_MAX);
- part->type = def->parts[i].type;
- part->coll_id = def->parts[i].coll_id;
- } else {
- part->coll_id = COLL_NONE;
- part->type = FIELD_TYPE_SCALAR;
- }
+ part->type = fields[i].type;
+ part->coll_id = fields[i].coll_id;
}
struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
field_count, false);
@@ -370,7 +392,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
rlist_add_entry(&key_list, ephemer_index_def, link);
struct space_def *ephemer_space_def =
- space_def_new_ephemeral(field_count);
+ space_def_new_ephemeral(field_count, fields);
if (ephemer_space_def == NULL) {
index_def_delete(ephemer_index_def);
return NULL;
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 312c966..beaeb0f 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format)
* dictionary will never be altered. If it can, then alter can
* affect another space, which shares a format with one which is
* altered.
+ *
+ * The only way to change the format of the space is to recreate
+ * space with the new format inside of BOX. Since there is no
+ * mechanism for recreating the ephemeral space, we need not worry
+ * about changing the format of the ephemeral space.
+ *
* @param p_format Double pointer to format. It is updated with
* hashed value, if corresponding format was found
* in hash table
@@ -709,13 +715,7 @@ static bool
tuple_format_reuse(struct tuple_format **p_format)
{
struct tuple_format *format = *p_format;
- if (!format->is_ephemeral)
- return false;
- /*
- * These fields do not participate in hashing.
- * Make sure they're unset.
- */
- assert(format->dict->name_count == 0);
+ assert(format->is_ephemeral);
assert(format->is_temporary);
mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format,
NULL);
@@ -739,9 +739,7 @@ tuple_format_reuse(struct tuple_format **p_format)
static int
tuple_format_add_to_hash(struct tuple_format *format)
{
- if(!format->is_ephemeral)
- return 0;
- assert(format->dict->name_count == 0);
+ assert(format->is_ephemeral);
assert(format->is_temporary);
mh_int_t key = mh_tuple_format_put(tuple_formats_hash,
(const struct tuple_format **)&format,
@@ -795,11 +793,11 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine,
if (tuple_format_create(format, keys, key_count, space_fields,
space_field_count) < 0)
goto err;
- if (tuple_format_reuse(&format))
+ if (is_ephemeral && tuple_format_reuse(&format))
return format;
if (tuple_format_register(format) < 0)
goto err;
- if (tuple_format_add_to_hash(format) < 0) {
+ if (is_ephemeral && tuple_format_add_to_hash(format) < 0) {
tuple_format_deregister(format);
goto err;
}
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Tarantool-patches] [PATCH v2 1/2] sql: specify field types in ephemeral space format
2020-04-12 16:29 ` [Tarantool-patches] [PATCH v2 1/2] sql: specify field types in ephemeral space format imeevma
@ 2020-04-13 22:47 ` Nikita Pettik
0 siblings, 0 replies; 6+ messages in thread
From: Nikita Pettik @ 2020-04-13 22:47 UTC (permalink / raw)
To: imeevma; +Cc: tarantool-patches, v.shpilevoy
On 12 Apr 19:29, imeevma@tarantool.org wrote:
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 1256df8..e4434b2 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -330,13 +330,41 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
> return NULL;
> }
>
> - struct key_part_def *ephemer_key_parts = region_alloc(&fiber()->gc,
> - sizeof(*ephemer_key_parts) * field_count);
> - if (ephemer_key_parts == NULL) {
> - diag_set(OutOfMemory, sizeof(*ephemer_key_parts) * field_count,
> - "region", "key parts");
> - return NULL;
> + struct region *region = &fiber()->gc;
> + /*
> + * Name of the fields will be "_COLUMN_1", "_COLUMN_2"
> + * and so on. Since number of columns no more than 2000,
> + * length of each name is no more than strlen("_COLUMN_")
> + * + 5.
> + */
> + assert(SQL_MAX_COLUMN <= 2000);
Ephemeral space is capable of storing more columns than casual table.
For instance eph. table holding result of join operations features
number of columns which equals to sum of all table's columns
participating in join.
> + uint32_t name_len = strlen("_COLUMN_") + 5;
> + uint32_t size = field_count * (sizeof(struct field_def) + name_len +
> + sizeof(struct key_part_def));
> + struct field_def *fields = region_alloc(region, size);
NULL check?
> + struct key_part_def *ephemer_key_parts = (void *)fields +
> + field_count * sizeof(struct field_def);
Strange indentation.
> + char *names = (char *)ephemer_key_parts +
> + field_count * sizeof(struct key_part_def);
> + for (uint32_t i = 0; i < field_count; ++i) {
> + struct field_def *field = &fields[i];
> + field->name = names;
> + names += name_len;
> + sprintf(field->name, "_COLUMN_%d", i);
> + field->is_nullable = true;
> + field->nullable_action = ON_CONFLICT_ACTION_NONE;
> + field->default_value = NULL;
> + field->default_value_expr = NULL;
> + if (def != NULL && i < def->part_count) {
> + assert(def->parts[i].type < field_type_MAX);
> + field->type = def->parts[i].type;
> + field->coll_id = def->parts[i].coll_id;
> + } else {
> + field->coll_id = COLL_NONE;
> + field->type = FIELD_TYPE_SCALAR;
> + }
> }
> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 312c966..beaeb0f 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -698,6 +698,12 @@ tuple_format_destroy(struct tuple_format *format)
> * dictionary will never be altered. If it can, then alter can
> * affect another space, which shares a format with one which is
> * altered.
> + *
> + * The only way to change the format of the space is to recreate
> + * space with the new format inside of BOX. Since there is no
> + * mechanism for recreating the ephemeral space, we need not worry
> + * about changing the format of the ephemeral space.
> + *
> * @param p_format Double pointer to format. It is updated with
> * hashed value, if corresponding format was found
> * in hash table
> @@ -709,13 +715,7 @@ static bool
> tuple_format_reuse(struct tuple_format **p_format)
> {
> struct tuple_format *format = *p_format;
> - if (!format->is_ephemeral)
> - return false;
> - /*
> - * These fields do not participate in hashing.
> - * Make sure they're unset.
> - */
> - assert(format->dict->name_count == 0);
> + assert(format->is_ephemeral);
> assert(format->is_temporary);
> mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format,
> NULL);
Personally I'd split this patch into two: one which affects box
component, and another one which introduces format of ephemeral
spaces in SQL.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH v2 2/2] sql: do not change order of inserted values
2020-04-12 16:29 [Tarantool-patches] [PATCH v2 0/2] sql: fix order of inserted rows imeevma
2020-04-12 16:29 ` [Tarantool-patches] [PATCH v2 1/2] sql: specify field types in ephemeral space format imeevma
@ 2020-04-12 16:29 ` imeevma
2020-04-13 22:59 ` Nikita Pettik
2020-04-13 22:35 ` [Tarantool-patches] [PATCH v2 0/2] sql: fix order of inserted rows Nikita Pettik
2 siblings, 1 reply; 6+ messages in thread
From: imeevma @ 2020-04-12 16:29 UTC (permalink / raw)
To: korablev, v.shpilevoy, tsafin, tarantool-patches
Before this patch, if an ephemeral space was used during INSERT or
REPLACE, the inserted values were sorted by the first column,
since this was the first part of the index. This can lead to an
error when using the AUTOINCREMENT feature, since changing the
order of the inserted value can change the value inserted instead
of NULL. To avoid this, the patch makes the rowid of the inserted
row in the ephemeral space the only part of the ephemeral space
index.
Closes #4256
---
src/box/sql.c | 26 +++++++----
src/box/sql/insert.c | 19 +++++++-
src/box/sql/select.c | 2 +
src/box/sql/sqlInt.h | 2 +
src/box/sql/vdbe.c | 19 ++------
...256-do-not-change-order-during-insertion.result | 50 ++++++++++++++++++++++
...6-do-not-change-order-during-insertion.test.lua | 15 +++++++
7 files changed, 107 insertions(+), 26 deletions(-)
create mode 100644 test/sql/gh-4256-do-not-change-order-during-insertion.result
create mode 100644 test/sql/gh-4256-do-not-change-order-during-insertion.test.lua
diff --git a/src/box/sql.c b/src/box/sql.c
index e4434b2..86ad195 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -324,10 +324,17 @@ struct space *
sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
{
struct key_def *def = NULL;
+ uint32_t part_count = field_count;
if (key_info != NULL) {
def = sql_key_info_to_key_def(key_info);
if (def == NULL)
return NULL;
+ /*
+ * In case is_pk_rowid is true we can use rowid
+ * as the only part of the key.
+ */
+ if (key_info->is_pk_rowid)
+ part_count = 1;
}
struct region *region = &fiber()->gc;
@@ -339,13 +346,13 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
*/
assert(SQL_MAX_COLUMN <= 2000);
uint32_t name_len = strlen("_COLUMN_") + 5;
- uint32_t size = field_count * (sizeof(struct field_def) + name_len +
- sizeof(struct key_part_def));
+ uint32_t size = field_count * (sizeof(struct field_def) + name_len) +
+ part_count * sizeof(struct key_part_def);
struct field_def *fields = region_alloc(region, size);
struct key_part_def *ephemer_key_parts = (void *)fields +
field_count * sizeof(struct field_def);
char *names = (char *)ephemer_key_parts +
- field_count * sizeof(struct key_part_def);
+ part_count * sizeof(struct key_part_def);
for (uint32_t i = 0; i < field_count; ++i) {
struct field_def *field = &fields[i];
field->name = names;
@@ -365,18 +372,21 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
}
}
- for (uint32_t i = 0; i < field_count; ++i) {
+ for (uint32_t i = 0; i < part_count; ++i) {
struct key_part_def *part = &ephemer_key_parts[i];
- part->fieldno = i;
+ uint32_t j = i;
+ if (key_info != NULL && key_info->is_pk_rowid)
+ j = field_count - 1;
+ part->fieldno = j;
part->nullable_action = ON_CONFLICT_ACTION_NONE;
part->is_nullable = true;
part->sort_order = SORT_ORDER_ASC;
part->path = NULL;
- part->type = fields[i].type;
- part->coll_id = fields[i].coll_id;
+ part->type = fields[j].type;
+ part->coll_id = fields[j].coll_id;
}
struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
- field_count, false);
+ part_count, false);
if (ephemer_key_def == NULL)
return NULL;
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 43a0de5..52a948d 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -455,8 +455,23 @@ sqlInsert(Parse * pParse, /* Parser context */
reg_eph = ++pParse->nMem;
regRec = sqlGetTempReg(pParse);
regCopy = sqlGetTempRange(pParse, nColumn + 1);
- sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
- nColumn + 1);
+ /*
+ * This key_info is used to show that
+ * rowid should be the first part of PK.
+ * This way we will save initial order of
+ * the inserted values. The order is
+ * important if we use the AUTOINCREMENT
+ * feature, since changing the order can
+ * change the number inserted instead of
+ * NULL.
+ */
+ struct sql_key_info *key_info =
+ sql_key_info_new(pParse->db, nColumn + 1);
+ key_info->parts[nColumn].type = FIELD_TYPE_UNSIGNED;
+ key_info->is_pk_rowid = true;
+ sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph,
+ nColumn + 1, 0, (char *)key_info,
+ P4_KEYINFO);
addrL = sqlVdbeAddOp1(v, OP_Yield, dest.iSDParm);
VdbeCoverage(v);
sqlVdbeAddOp2(v, OP_NextIdEphemeral, reg_eph,
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 65e41f2..f394840 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1422,6 +1422,7 @@ sql_key_info_new(sql *db, uint32_t part_count)
key_info->key_def = NULL;
key_info->refs = 1;
key_info->part_count = part_count;
+ key_info->is_pk_rowid = false;
for (uint32_t i = 0; i < part_count; i++) {
struct key_part_def *part = &key_info->parts[i];
part->fieldno = i;
@@ -1448,6 +1449,7 @@ sql_key_info_new_from_key_def(sql *db, const struct key_def *key_def)
key_info->key_def = NULL;
key_info->refs = 1;
key_info->part_count = key_def->part_count;
+ key_info->is_pk_rowid = false;
key_def_dump_parts(key_def, key_info->parts, NULL);
return key_info;
}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2d6bad4..f07dc6a 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4118,6 +4118,8 @@ struct sql_key_info {
struct key_def *key_def;
/** Reference counter. */
uint32_t refs;
+ /** Rowid should be the only part of PK, if true. */
+ bool is_pk_rowid;
/** Number of parts in the key. */
uint32_t part_count;
/** Definition of the key parts. */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e8a029a..56c9d82 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2694,23 +2694,10 @@ case OP_Column: {
pC->cacheStatus = p->cacheCtr;
}
enum field_type field_type = field_type_MAX;
- if (pC->eCurType == CURTYPE_TARANTOOL) {
- /*
- * Ephemeral spaces feature only one index
- * covering all fields, but ephemeral spaces
- * lack format. So, we can fetch type from
- * key parts.
- */
- if (pC->uc.pCursor->curFlags & BTCF_TEphemCursor) {
- field_type = pC->uc.pCursor->index->def->
- key_def->parts[p2].type;
- } else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) {
- field_type = pC->uc.pCursor->space->def->
- fields[p2].type;
- }
- } else if (pC->eCurType == CURTYPE_SORTER) {
+ if (pC->eCurType == CURTYPE_TARANTOOL)
+ field_type = pC->uc.pCursor->space->def->fields[p2].type;
+ else if (pC->eCurType == CURTYPE_SORTER)
field_type = vdbe_sorter_get_field_type(pC->uc.pSorter, p2);
- }
struct Mem *default_val_mem =
pOp->p4type == P4_MEM ? pOp->p4.pMem : NULL;
if (vdbe_field_ref_fetch(&pC->field_ref, p2, pDest) != 0)
diff --git a/test/sql/gh-4256-do-not-change-order-during-insertion.result b/test/sql/gh-4256-do-not-change-order-during-insertion.result
new file mode 100644
index 0000000..56e28b5
--- /dev/null
+++ b/test/sql/gh-4256-do-not-change-order-during-insertion.result
@@ -0,0 +1,50 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+engine = test_run:get_cfg('engine')
+ | ---
+ | ...
+--
+-- Make sure that when inserting, values are inserted in the given
+-- order when ephemeral space is used.
+--
+box.execute('CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT);')
+ | ---
+ | - row_count: 1
+ | ...
+--
+-- In order for this INSERT to use the ephemeral space, we created
+-- this trigger.
+--
+box.execute('CREATE TRIGGER r AFTER INSERT ON t FOR EACH ROW BEGIN SELECT 1; END')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('INSERT INTO t VALUES (1), (NULL), (10), (NULL), (NULL), (3), (NULL);')
+ | ---
+ | - autoincrement_ids:
+ | - 2
+ | - 11
+ | - 12
+ | - 13
+ | row_count: 7
+ | ...
+box.execute('SELECT * FROM t;')
+ | ---
+ | - metadata:
+ | - name: I
+ | type: integer
+ | rows:
+ | - [1]
+ | - [2]
+ | - [3]
+ | - [10]
+ | - [11]
+ | - [12]
+ | - [13]
+ | ...
+box.execute('DROP TABLE t;')
+ | ---
+ | - row_count: 1
+ | ...
diff --git a/test/sql/gh-4256-do-not-change-order-during-insertion.test.lua b/test/sql/gh-4256-do-not-change-order-during-insertion.test.lua
new file mode 100644
index 0000000..4d8367c
--- /dev/null
+++ b/test/sql/gh-4256-do-not-change-order-during-insertion.test.lua
@@ -0,0 +1,15 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+--
+-- Make sure that when inserting, values are inserted in the given
+-- order when ephemeral space is used.
+--
+box.execute('CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT);')
+--
+-- In order for this INSERT to use the ephemeral space, we created
+-- this trigger.
+--
+box.execute('CREATE TRIGGER r AFTER INSERT ON t FOR EACH ROW BEGIN SELECT 1; END')
+box.execute('INSERT INTO t VALUES (1), (NULL), (10), (NULL), (NULL), (3), (NULL);')
+box.execute('SELECT * FROM t;')
+box.execute('DROP TABLE t;')
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Tarantool-patches] [PATCH v2 2/2] sql: do not change order of inserted values
2020-04-12 16:29 ` [Tarantool-patches] [PATCH v2 2/2] sql: do not change order of inserted values imeevma
@ 2020-04-13 22:59 ` Nikita Pettik
0 siblings, 0 replies; 6+ messages in thread
From: Nikita Pettik @ 2020-04-13 22:59 UTC (permalink / raw)
To: imeevma; +Cc: tarantool-patches, v.shpilevoy
On 12 Apr 19:29, imeevma@tarantool.org wrote:
> @@ -365,18 +372,21 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
> }
> }
>
> - for (uint32_t i = 0; i < field_count; ++i) {
> + for (uint32_t i = 0; i < part_count; ++i) {
> struct key_part_def *part = &ephemer_key_parts[i];
> - part->fieldno = i;
I'd place comment explaining value of j and position of 'rowid'.
> + uint32_t j = i;
> + if (key_info != NULL && key_info->is_pk_rowid)
> + j = field_count - 1;
> + part->fieldno = j;
> part->nullable_action = ON_CONFLICT_ACTION_NONE;
> part->is_nullable = true;
> part->sort_order = SORT_ORDER_ASC;
> part->path = NULL;
> - part->type = fields[i].type;
> - part->coll_id = fields[i].coll_id;
> + part->type = fields[j].type;
> + part->coll_id = fields[j].coll_id;
> }
> struct key_def *ephemer_key_def = key_def_new(ephemer_key_parts,
> - field_count, false);
> + part_count, false);
> if (ephemer_key_def == NULL)
> return NULL;
>
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 43a0de5..52a948d 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -455,8 +455,23 @@ sqlInsert(Parse * pParse, /* Parser context */
> reg_eph = ++pParse->nMem;
> regRec = sqlGetTempReg(pParse);
> regCopy = sqlGetTempRange(pParse, nColumn + 1);
> - sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
> - nColumn + 1);
> + /*
> + * This key_info is used to show that
> + * rowid should be the first part of PK.
> + * This way we will save initial order of
> + * the inserted values. The order is
> + * important if we use the AUTOINCREMENT
> + * feature, since changing the order can
> + * change the number inserted instead of
> + * NULL.
> + */
So why don't you use this trick only if space with autoincrement
is affected? So that proceed with old way for all other spaces.
> + struct sql_key_info *key_info =
> + sql_key_info_new(pParse->db, nColumn + 1);
> + key_info->parts[nColumn].type = FIELD_TYPE_UNSIGNED;
> + key_info->is_pk_rowid = true;
> + sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph,
> + nColumn + 1, 0, (char *)key_info,
> + P4_KEYINFO);
> addrL = sqlVdbeAddOp1(v, OP_Yield, dest.iSDParm);
> VdbeCoverage(v);
> sqlVdbeAddOp2(v, OP_NextIdEphemeral, reg_eph,
> diff --git a/test/sql/gh-4256-do-not-change-order-during-insertion.test.lua b/test/sql/gh-4256-do-not-change-order-during-insertion.test.lua
> new file mode 100644
> index 0000000..4d8367c
> --- /dev/null
> +++ b/test/sql/gh-4256-do-not-change-order-during-insertion.test.lua
> @@ -0,0 +1,15 @@
Btw, why don't you use SQL format of tests?
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')
> +--
> +-- Make sure that when inserting, values are inserted in the given
> +-- order when ephemeral space is used.
> +--
> +box.execute('CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT);')
> +--
> +-- In order for this INSERT to use the ephemeral space, we created
> +-- this trigger.
> +--
> +box.execute('CREATE TRIGGER r AFTER INSERT ON t FOR EACH ROW BEGIN SELECT 1; END')
> +box.execute('INSERT INTO t VALUES (1), (NULL), (10), (NULL), (NULL), (3), (NULL);')
> +box.execute('SELECT * FROM t;')
> +box.execute('DROP TABLE t;')
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 0/2] sql: fix order of inserted rows
2020-04-12 16:29 [Tarantool-patches] [PATCH v2 0/2] sql: fix order of inserted rows imeevma
2020-04-12 16:29 ` [Tarantool-patches] [PATCH v2 1/2] sql: specify field types in ephemeral space format imeevma
2020-04-12 16:29 ` [Tarantool-patches] [PATCH v2 2/2] sql: do not change order of inserted values imeevma
@ 2020-04-13 22:35 ` Nikita Pettik
2 siblings, 0 replies; 6+ messages in thread
From: Nikita Pettik @ 2020-04-13 22:35 UTC (permalink / raw)
To: imeevma; +Cc: tarantool-patches, v.shpilevoy
On 12 Apr 19:29, imeevma@tarantool.org wrote:
> This patch-set fixes order of inserted rows in case ephemeral
> space is used. The order now is the same in which these values
> were inserted originally. Wrong order could lead to an error in
> case AUTOINCREMENT feature was used.
This is second version of patch, so it would be nice to see
changelog describing differences between versions.
> https://github.com/tarantool/tarantool/issues/4256
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4256-fix-order-during-insertion
>
> #ChangeLog
> - The inserted values will now always be inserted in the order in
> which they were given (gh-4256).
>
> Mergen Imeev (2):
> sql: specify field types in ephemeral space format
> sql: do not change order of inserted values
>
> src/box/space_def.c | 5 +-
> src/box/space_def.h | 3 +-
> src/box/sql.c | 68 ++++++++++++++++------
> src/box/sql/insert.c | 19 +++++-
> src/box/sql/select.c | 2 +
> src/box/sql/sqlInt.h | 2 +
> src/box/sql/vdbe.c | 19 +-----
> src/box/tuple_format.c | 22 ++++---
> ...256-do-not-change-order-during-insertion.result | 50 ++++++++++++++++
> ...6-do-not-change-order-during-insertion.test.lua | 15 +++++
> 10 files changed, 153 insertions(+), 52 deletions(-)
> create mode 100644 test/sql/gh-4256-do-not-change-order-during-insertion.result
> create mode 100644 test/sql/gh-4256-do-not-change-order-during-insertion.test.lua
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread