[tarantool-patches] [PATCH 0/2] Re-implement rowid generation for ephemeral spaces

[tarantool-patches] [PATCH 0/2] Re-implement rowid generation for ephemeral spaces

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/gh-3297-ephemeral-rowid
Issue: https://github.com/tarantool/tarantool/issues/3297

This patch-set fixes incorrect rowid generation for ephemeral spaces.
To achieve this, we introduce separate method in space's vtab,
which currently available only for memtx engine (as well as ephemeral
spaces). It simply increments built-in counter and returs new rowid.
It allows us to get rid of calling index_max() which obviously was
wrong way at calcucaling next id (since rowid field is taken to be
last in stored tuples).

Nikita Pettik (2):
  space: add method to fetch next rowid
  sql: use vtab::rowid_next() instead of index_max()

 src/box/blackhole.c                           |  1 +
 src/box/errcode.h                             |  2 ++
 src/box/memtx_space.c                         | 17 ++++++++++++
 src/box/memtx_space.h                         |  7 +++++
 src/box/space.c                               |  9 +++++++
 src/box/space.h                               |  3 +++
 src/box/sql.c                                 | 28 +------------------
 src/box/sql/insert.c                          | 17 +++---------
 src/box/sql/select.c                          | 12 ++++-----
 src/box/sql/tarantoolInt.h                    | 13 ---------
 src/box/sql/vdbe.c                            | 39 +++++++++++++++++----------
 src/box/sysview.c                             |  1 +
 src/box/vinyl.c                               |  1 +
 src/errinj.h                                  |  1 +
 test/box/errinj.result                        |  2 ++
 test/box/misc.result                          |  1 +
 test/sql-tap/gh-3297-ephemeral-rowid.test.lua | 30 +++++++++++++++++++++
 test/sql/errinj.result                        | 15 +++++++++++
 test/sql/errinj.test.lua                      |  7 +++++
 19 files changed, 132 insertions(+), 74 deletions(-)
 create mode 100755 test/sql-tap/gh-3297-ephemeral-rowid.test.lua

-- 
2.15.1

[tarantool-patches] [PATCH 1/2] space: add method to fetch next rowid

[Nikita Pettik]

Ephemeral space are extensively used in SQL to store intermediate
results of query processing. To keep things simple, they feature only
one unique index (primary) which covers all fields. However, ephemeral
space can be used to store non-unique entries. In this case, one
additional field added to the end if stored data:

[field1, ... fieldn, rowid]

Note that it can't be added to the beginning of tuple since data in
ephemeral space may be kept as sorted. Previously, to generate proper
rowid index_max() was used. However, it is obviously wrong way to do it.
Hence, lets add simple integer counter to memtx space (ephemeral spaces
are valid only for memtx engine) and introduce method in vtab to fetch
next rowid value.

Needed for #3297
---
 src/box/blackhole.c    |  1 +
 src/box/errcode.h      |  2 ++
 src/box/memtx_space.c  | 17 +++++++++++++++++
 src/box/memtx_space.h  |  7 +++++++
 src/box/space.c        |  9 +++++++++
 src/box/space.h        |  3 +++
 src/box/sysview.c      |  1 +
 src/box/vinyl.c        |  1 +
 src/errinj.h           |  1 +
 test/box/errinj.result |  2 ++
 test/box/misc.result   |  1 +
 11 files changed, 45 insertions(+)

diff --git a/src/box/blackhole.c b/src/box/blackhole.c
index aafbfbf65..e34a2dc4a 100644
--- a/src/box/blackhole.c
+++ b/src/box/blackhole.c
@@ -118,6 +118,7 @@ static const struct space_vtab blackhole_space_vtab = {
 	/* .execute_upsert = */ blackhole_space_execute_upsert,
 	/* .ephemeral_replace = */ generic_space_ephemeral_replace,
 	/* .ephemeral_delete = */ generic_space_ephemeral_delete,
+	/* .ephemeral_rowid_next = */ generic_space_ephemeral_rowid_next,
 	/* .init_system_space = */ generic_init_system_space,
 	/* .init_ephemeral_space = */ generic_init_ephemeral_space,
 	/* .check_index_def = */ generic_space_check_index_def,
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 04f4f34ee..fab8b6617 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -223,6 +223,8 @@ struct errcode_record {
 	/*168 */_(ER_DROP_FK_CONSTRAINT,	"Failed to drop foreign key constraint '%s': %s") \
 	/*169 */_(ER_NO_SUCH_CONSTRAINT,	"Constraint %s does not exist") \
 	/*170 */_(ER_CONSTRAINT_EXISTS,		"Constraint %s already exists") \
+	/*171 */_(ER_ROWID_OVERFLOW,		"Rowid is overflowed: too many entries in ephemeral space") \
+
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 8cf2e6b94..075b86f9d 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -601,6 +601,21 @@ memtx_space_ephemeral_delete(struct space *space, const char *key)
 	return 0;
 }
 
+static int
+memtx_space_ephemeral_rowid_next(struct space *space, uint64_t *rowid)
+{
+	assert(rowid != NULL);
+	struct memtx_space *memtx_space = (struct memtx_space *)space;
+	ERROR_INJECT(ERRINJ_ROWID_OVERFLOW,
+		     { memtx_space->rowid = UINT64_MAX; });
+	if (unlikely(memtx_space->rowid == UINT64_MAX)) {
+		diag_set(ClientError, ER_ROWID_OVERFLOW);
+		return -1;
+	}
+	*rowid = memtx_space->rowid++;
+	return 0;
+}
+
 /* }}} DML */
 
 /* {{{ DDL */
@@ -941,6 +956,7 @@ static const struct space_vtab memtx_space_vtab = {
 	/* .execute_upsert = */ memtx_space_execute_upsert,
 	/* .ephemeral_replace = */ memtx_space_ephemeral_replace,
 	/* .ephemeral_delete = */ memtx_space_ephemeral_delete,
+	/* .ephemeral_rowid_next = */ memtx_space_ephemeral_rowid_next,
 	/* .init_system_space = */ memtx_init_system_space,
 	/* .init_ephemeral_space = */ memtx_init_ephemeral_space,
 	/* .check_index_def = */ memtx_space_check_index_def,
@@ -1002,6 +1018,7 @@ memtx_space_new(struct memtx_engine *memtx,
 	tuple_format_unref(format);
 
 	memtx_space->bsize = 0;
+	memtx_space->rowid = 0;
 	memtx_space->replace = memtx_space_replace_no_keys;
 	return (struct space *)memtx_space;
 }
diff --git a/src/box/memtx_space.h b/src/box/memtx_space.h
index 7dc341079..532538312 100644
--- a/src/box/memtx_space.h
+++ b/src/box/memtx_space.h
@@ -42,6 +42,13 @@ struct memtx_space {
 	struct space base;
 	/* Number of bytes used in memory by tuples in the space. */
 	size_t bsize;
+	/**
+	 * This counter is used to generate unique ids for
+	 * ephemeral spaces. Mostly used by SQL: values of this
+	 * var are stored as separate field to hold non-unique
+	 * tuples within one unique primary key.
+	 */
+	uint64_t rowid;
 	/**
 	 * A pointer to replace function, set to different values
 	 * at different stages of recovery.
diff --git a/src/box/space.c b/src/box/space.c
index 548f66787..4d174f7ec 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -568,6 +568,15 @@ generic_space_ephemeral_delete(struct space *space, const char *key)
 	return -1;
 }
 
+int
+generic_space_ephemeral_rowid_next(struct space *space, uint64_t *rowid)
+{
+	(void)space;
+	(void)rowid;
+	unreachable();
+	return 0;
+}
+
 void
 generic_init_system_space(struct space *space)
 {
diff --git a/src/box/space.h b/src/box/space.h
index f3e9e1e21..7eb7ae292 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -71,6 +71,8 @@ struct space_vtab {
 
 	int (*ephemeral_delete)(struct space *, const char *);
 
+	int (*ephemeral_rowid_next)(struct space *, uint64_t *);
+
 	void (*init_system_space)(struct space *);
 	/**
 	 * Initialize an ephemeral space instance.
@@ -457,6 +459,7 @@ size_t generic_space_bsize(struct space *);
 int generic_space_apply_initial_join_row(struct space *, struct request *);
 int generic_space_ephemeral_replace(struct space *, const char *, const char *);
 int generic_space_ephemeral_delete(struct space *, const char *);
+int generic_space_ephemeral_rowid_next(struct space *, uint64_t *);
 void generic_init_system_space(struct space *);
 void generic_init_ephemeral_space(struct space *);
 int generic_space_check_index_def(struct space *, struct index_def *);
diff --git a/src/box/sysview.c b/src/box/sysview.c
index ed5bca38e..29de43093 100644
--- a/src/box/sysview.c
+++ b/src/box/sysview.c
@@ -484,6 +484,7 @@ static const struct space_vtab sysview_space_vtab = {
 	/* .execute_upsert = */ sysview_space_execute_upsert,
 	/* .ephemeral_replace = */ generic_space_ephemeral_replace,
 	/* .ephemeral_delete = */ generic_space_ephemeral_delete,
+	/* .ephemeral_rowid_next = */ generic_space_ephemeral_rowid_next,
 	/* .init_system_space = */ generic_init_system_space,
 	/* .init_ephemeral_space = */ generic_init_ephemeral_space,
 	/* .check_index_def = */ generic_space_check_index_def,
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 09b665598..8ff52b466 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -4470,6 +4470,7 @@ static const struct space_vtab vinyl_space_vtab = {
 	/* .execute_upsert = */ vinyl_space_execute_upsert,
 	/* .ephemeral_replace = */ generic_space_ephemeral_replace,
 	/* .ephemeral_delete = */ generic_space_ephemeral_delete,
+	/* .ephemeral_rowid_next = */ generic_space_ephemeral_rowid_next,
 	/* .init_system_space = */ generic_init_system_space,
 	/* .init_ephemeral_space = */ generic_init_ephemeral_space,
 	/* .check_index_def = */ vinyl_space_check_index_def,
diff --git a/src/errinj.h b/src/errinj.h
index 84a1fbb5e..58aa5864a 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -121,6 +121,7 @@ struct errinj {
 	_(ERRINJ_RELAY_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \
 	_(ERRINJ_WAL_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \
 	_(ERRINJ_VY_COMPACTION_DELAY, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_ROWID_OVERFLOW, ERRINJ_BOOL, {.bparam = false}) \
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index c4a1326cd..191e16911 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -54,6 +54,8 @@ errinj.info()
     state: false
   ERRINJ_RELAY_REPORT_INTERVAL:
     state: 0
+  ERRINJ_ROWID_OVERFLOW:
+    state: false
   ERRINJ_VY_READ_PAGE_TIMEOUT:
     state: 0
   ERRINJ_XLOG_META:
diff --git a/test/box/misc.result b/test/box/misc.result
index 3d7317caf..e3d3d3c3b 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -497,6 +497,7 @@ t;
   168: box.error.DROP_FK_CONSTRAINT
   169: box.error.NO_SUCH_CONSTRAINT
   170: box.error.CONSTRAINT_EXISTS
+  171: box.error.ROWID_OVERFLOW
 ...
 test_run:cmd("setopt delimiter ''");
 ---
-- 
2.15.1

[tarantool-patches] [PATCH 2/2] sql: use vtab::rowid_next() instead of index_max()

[Nikita Pettik]

After introducing separate method in space's vtab to fetch next rowid
value, lets use it in SQL internals. This allows us to fix incorrect
results of queries involving storing equal tuples in ephemeral spaces.

Closes #3297
---
 src/box/sql.c                                 | 28 +------------------
 src/box/sql/insert.c                          | 17 +++---------
 src/box/sql/select.c                          | 12 ++++-----
 src/box/sql/tarantoolInt.h                    | 13 ---------
 src/box/sql/vdbe.c                            | 39 +++++++++++++++++----------
 test/sql-tap/gh-3297-ephemeral-rowid.test.lua | 30 +++++++++++++++++++++
 test/sql/errinj.result                        | 15 +++++++++++
 test/sql/errinj.test.lua                      |  7 +++++
 8 files changed, 87 insertions(+), 74 deletions(-)
 create mode 100755 test/sql-tap/gh-3297-ephemeral-rowid.test.lua

diff --git a/src/box/sql.c b/src/box/sql.c
index c7b87e57a..c4fc4b49c 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -41,6 +41,7 @@
 #include "box.h"
 #include "txn.h"
 #include "space.h"
+#include "memtx_space.h"
 #include "space_def.h"
 #include "index_def.h"
 #include "tuple.h"
@@ -1434,33 +1435,6 @@ sql_debug_info(struct info_handler *h)
 	info_end(h);
 }
 
-/**
- * Extract maximum integer value from ephemeral space.
- * If index is empty - return 0 in max_id and success status.
- *
- * @param space Pointer to ephemeral space.
- * @param fieldno Number of field from fetching tuple.
- * @param[out] max_id Fetched max value.
- *
- * @retval 0 on success, -1 otherwise.
- */
-int tarantoolSqlite3EphemeralGetMaxId(struct space *space, uint32_t fieldno,
-				      uint64_t *max_id)
-{
-	struct index *primary_index = *space->index;
-	struct tuple *tuple;
-	if (index_max(primary_index, NULL, 0, &tuple) != 0)
-		return -1;
-	if (tuple == NULL) {
-		*max_id = 0;
-		return 0;
-	}
-	if (tuple_field_u64(tuple, fieldno, max_id) == -1)
-		return -1;
-
-	return 0;
-}
-
 int
 tarantoolSqlNextSeqId(uint64_t *max_id)
 {
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 5862917f0..fc8b007bc 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -461,29 +461,19 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 			 *      M: ...
 			 */
 			int regRec;	/* Register to hold packed record */
-			int regTempId;	/* Register to hold temp table ID */
 			int regCopy;    /* Register to keep copy of registers from select */
 			int addrL;	/* Label "L" */
-			int64_t initial_pk = 0;
 
 			srcTab = pParse->nTab++;
 			reg_eph = ++pParse->nMem;
 			regRec = sqlite3GetTempReg(pParse);
-			regCopy = sqlite3GetTempRange(pParse, nColumn);
-			regTempId = sqlite3GetTempReg(pParse);
+			regCopy = sqlite3GetTempRange(pParse, nColumn + 1);
 			sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
 					  nColumn + 1);
-
-			/* Create counter for rowid */
-			sqlite3VdbeAddOp4Dup8(v, OP_Int64,
-					      0 /* unused */,
-					      regTempId,
-					      0 /* unused */,
-					      (const unsigned char*) &initial_pk,
-					      P4_INT64);
 			addrL = sqlite3VdbeAddOp1(v, OP_Yield, dest.iSDParm);
 			VdbeCoverage(v);
-			sqlite3VdbeAddOp2(v, OP_AddImm, regTempId, 1);
+			sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, reg_eph,
+					  regCopy + nColumn);
 			sqlite3VdbeAddOp3(v, OP_Copy, regFromSelect, regCopy, nColumn-1);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy,
 					  nColumn + 1, regRec);
@@ -494,7 +484,6 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 			sqlite3VdbeGoto(v, addrL);
 			sqlite3VdbeJumpHere(v, addrL);
 			sqlite3ReleaseTempReg(pParse, regRec);
-			sqlite3ReleaseTempReg(pParse, regTempId);
 			sqlite3ReleaseTempRange(pParse, regCopy, nColumn);
 		}
 	} else {
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 77e0c5d66..b191f318b 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1154,8 +1154,8 @@ selectInnerLoop(Parse * pParse,		/* The parser context */
 				int regRec = sqlite3GetTempReg(pParse);
 				/* Last column is required for ID. */
 				int regCopy = sqlite3GetTempRange(pParse, nResultCol + 1);
-				sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, pDest->reg_eph,
-						  nResultCol, regCopy + nResultCol);
+				sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, pDest->reg_eph,
+						  regCopy + nResultCol);
 				/* Positioning ID column to be last in inserted tuple.
 				 * NextId -> regCopy + n + 1
 				 * Copy [regResult, regResult + n] -> [regCopy, regCopy + n]
@@ -1601,8 +1601,8 @@ generateSortTail(Parse * pParse,	/* Parsing context */
 	case SRT_Table:
 	case SRT_EphemTab: {
 			int regCopy = sqlite3GetTempRange(pParse,  nColumn);
-			sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, pDest->reg_eph,
-					  nColumn, regTupleid);
+			sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, pDest->reg_eph,
+					  regTupleid);
 			sqlite3VdbeAddOp3(v, OP_Copy, regRow, regCopy, nSortData - 1);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy, nColumn + 1, regRow);
 			sqlite3VdbeAddOp2(v, OP_IdxInsert, regRow, pDest->reg_eph);
@@ -3127,8 +3127,8 @@ generateOutputSubroutine(struct Parse *parse, struct Select *p,
 	case SRT_EphemTab:{
 			int regRec = sqlite3GetTempReg(parse);
 			int regCopy = sqlite3GetTempRange(parse, in->nSdst + 1);
-			sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, dest->reg_eph,
-					  in->nSdst, regCopy + in->nSdst);
+			sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, dest->reg_eph,
+					  regCopy + in->nSdst);
 			sqlite3VdbeAddOp3(v, OP_Copy, in->iSdst, regCopy,
 					  in->nSdst - 1);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy,
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index daab3c84b..7d63b6fd6 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -130,19 +130,6 @@ int tarantoolSqlite3EphemeralCount(BtCursor * pCur, i64 * pnEntry);
 int tarantoolSqlite3EphemeralDrop(BtCursor * pCur);
 int tarantoolSqlite3EphemeralClearTable(BtCursor * pCur);
 
-/**
- * Extract maximum integer value from ephemeral space.
- * If index is empty - return 0 in max_id and success status.
- *
- * @param space Pointer to ephemeral space.
- * @param fieldno Number of field from fetching tuple.
- * @param[out] max_id Fetched max value.
- *
- * @retval 0 on success, -1 otherwise.
- */
-int tarantoolSqlite3EphemeralGetMaxId(struct space *space, uint32_t fieldno,
-				       uint64_t * max_id);
-
 /**
  * Performs exactly as extract_key + sqlite3VdbeCompareMsgpack,
  * only faster.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015cf9..c730c70cb 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3685,25 +3685,36 @@ case OP_NextSequenceId: {
 	break;
 }
 
-/* Opcode: NextIdEphemeral P1 P2 P3 * *
- * Synopsis: r[P3]=get_max(space_index[P1]{Column[P2]})
+/* Opcode: NextIdEphemeral P1 P2 * * *
+ * Synopsis: r[P2]=get_next_rowid(space[P1])
  *
- * This opcode works in the same way as OP_NextId does, except it
- * is only applied for ephemeral tables. The difference is in the
- * fact that all ephemeral tables don't have space_id (to be more
- * precise it equals to zero). This opcode uses register P1 to
- * fetch pointer to epehemeral space.
+ * This opcode stores next `rowid` for the ephemeral space to
+ * P2 register. `rowid` is required, because inserted to
+ * ephemeral space tuples may be not unique. Meanwhile,
+ * Tarantool`s ephemeral spaces can contain only unique tuples
+ * due to only one index (which is PK over all columns in space).
  */
 case OP_NextIdEphemeral: {
 	struct space *space = (struct space*)p->aMem[pOp->p1].u.p;
-	int p2 = pOp->p2;
-	assert(space != NULL);
-	pOut = &aMem[pOp->p3];
-	rc = tarantoolSqlite3EphemeralGetMaxId(space, p2,
-					       (uint64_t *) &pOut->u.i);
-	if (rc != 0)
+	assert(space->def->id == 0);
+	uint64_t rowid;
+	if (space->vtab->ephemeral_rowid_next(space, &rowid) != 0) {
+		rc = SQL_TARANTOOL_ERROR;
 		goto abort_due_to_error;
-	pOut->u.i += 1;
+	}
+	/*
+	 * FIXME: since memory cell can comprise only 32-bit
+	 * integer, make sure it can fit in. This check should
+	 * be removed when memory cell is extended with unsigned
+	 * 64-bit integer.
+	 */
+	if (rowid > INT32_MAX) {
+		diag_set(ClientError, ER_ROWID_OVERFLOW);
+		rc = SQL_TARANTOOL_ERROR;
+		goto abort_due_to_error;
+	}
+	pOut = &aMem[pOp->p2];
+	pOut->u.i = rowid;
 	pOut->flags = MEM_Int;
 	break;
 }
diff --git a/test/sql-tap/gh-3297-ephemeral-rowid.test.lua b/test/sql-tap/gh-3297-ephemeral-rowid.test.lua
new file mode 100755
index 000000000..dd2b07cc5
--- /dev/null
+++ b/test/sql-tap/gh-3297-ephemeral-rowid.test.lua
@@ -0,0 +1,30 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(1)
+
+-- Check that OP_NextIdEphemeral generates unique ids.
+--
+test:execsql [[
+    CREATE TABLE T1(A INT PRIMARY KEY);
+    CREATE TABLE T2(A INT PRIMARY KEY, B INT);
+    INSERT INTO T1 VALUES(12);
+    INSERT INTO T2 VALUES(1, 5);
+    INSERT INTO T2 VALUES(2, 2);
+    INSERT INTO T2 VALUES(3, 2);
+    INSERT INTO T2 VALUES(4, 2);
+]]
+
+test:do_execsql_test(
+    "gh-3297-1",
+    [[
+        SELECT * FROM ( SELECT A FROM T1 LIMIT 1), (SELECT B FROM T2 LIMIT 10);
+    ]],
+    {
+        12, 2,
+        12, 2,
+        12, 2,
+        12, 5
+    }
+)
+
+test:finish_test()
diff --git a/test/sql/errinj.result b/test/sql/errinj.result
index a0ba60f45..60f776c3c 100644
--- a/test/sql/errinj.result
+++ b/test/sql/errinj.result
@@ -279,3 +279,18 @@ errinj.set("ERRINJ_WAL_IO", false)
 box.sql.execute("DROP TABLE t3;")
 ---
 ...
+-- Make sure that overflow of rowid used for ephemeral spaces
+-- is hadnled properly.
+--
+box.error.injection.set("ERRINJ_ROWID_OVERFLOW", true)
+---
+- ok
+...
+box.sql.execute("WITH RECURSIVE tmp AS (SELECT 1 UNION ALL SELECT * FROM tmp LIMIT 2) SELECT * FROM tmp;")
+---
+- error: 'Rowid is overflowed: too many entries in ephemeral space'
+...
+box.error.injection.set("ERRINJ_ROWID_OVERFLOW", false)
+---
+- ok
+...
diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
index 25d73f0c2..034a43d4e 100644
--- a/test/sql/errinj.test.lua
+++ b/test/sql/errinj.test.lua
@@ -97,3 +97,10 @@ box.sql.execute("ALTER TABLE t3 DROP CONSTRAINT fk1;")
 box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
 errinj.set("ERRINJ_WAL_IO", false)
 box.sql.execute("DROP TABLE t3;")
+
+-- Make sure that overflow of rowid used for ephemeral spaces
+-- is hadnled properly.
+--
+box.error.injection.set("ERRINJ_ROWID_OVERFLOW", true)
+box.sql.execute("WITH RECURSIVE tmp AS (SELECT 1 UNION ALL SELECT * FROM tmp LIMIT 2) SELECT * FROM tmp;")
+box.error.injection.set("ERRINJ_ROWID_OVERFLOW", false)
-- 
2.15.1

Re: [tarantool-patches] [PATCH 1/2] space: add method to fetch next rowid

[Vladimir Davydov]

On Mon, Oct 29, 2018 at 10:02:05PM +0300, Nikita Pettik wrote:
> Ephemeral space are extensively used in SQL to store intermediate
> results of query processing. To keep things simple, they feature only
> one unique index (primary) which covers all fields. However, ephemeral
> space can be used to store non-unique entries. In this case, one
> additional field added to the end if stored data:
> 
> [field1, ... fieldn, rowid]
> 
> Note that it can't be added to the beginning of tuple since data in
> ephemeral space may be kept as sorted. Previously, to generate proper
> rowid index_max() was used. However, it is obviously wrong way to do it.
> Hence, lets add simple integer counter to memtx space (ephemeral spaces
> are valid only for memtx engine) and introduce method in vtab to fetch
> next rowid value.
> 
> Needed for #3297
> ---
>  src/box/blackhole.c    |  1 +
>  src/box/errcode.h      |  2 ++
>  src/box/memtx_space.c  | 17 +++++++++++++++++
>  src/box/memtx_space.h  |  7 +++++++
>  src/box/space.c        |  9 +++++++++
>  src/box/space.h        |  3 +++
>  src/box/sysview.c      |  1 +
>  src/box/vinyl.c        |  1 +
>  src/errinj.h           |  1 +
>  test/box/errinj.result |  2 ++
>  test/box/misc.result   |  1 +
>  11 files changed, 45 insertions(+)
> 
> diff --git a/src/box/blackhole.c b/src/box/blackhole.c
> index aafbfbf65..e34a2dc4a 100644
> --- a/src/box/blackhole.c
> +++ b/src/box/blackhole.c
> @@ -118,6 +118,7 @@ static const struct space_vtab blackhole_space_vtab = {
>  	/* .execute_upsert = */ blackhole_space_execute_upsert,
>  	/* .ephemeral_replace = */ generic_space_ephemeral_replace,
>  	/* .ephemeral_delete = */ generic_space_ephemeral_delete,
> +	/* .ephemeral_rowid_next = */ generic_space_ephemeral_rowid_next,

Urgh, I'm starting to think that after all we should tear ephemeral
spaces from the generic space class, because they clutter the memtx
implementation and never going to work with vinyl anyway, because vinyl
has very poor read performance.

I think that we should reimplement ephemeral spaces as a completely
separate entity. Start with in-memory only design, and when there's a
demand make them swappable. May be, we should even consider putting back
the original SQLite implementation.

Re: [tarantool-patches] [PATCH 1/2] space: add method to fetch next rowid

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 375 bytes --]



> On 30 Oct 2018, at 11:45, Vladimir Davydov <vdavydov.dev@gmail.com> wrote:
> 
> May be, we should even consider putting back
> the original SQLite implementation.

Unfortunately, this variant is unlikely to be possible at all.
We’ve already deleted and refactored far too much SQL code
since original ephemeral tables were substituted with Tarantool
ones.


[-- Attachment #2: Type: text/html, Size: 1941 bytes --]

[tarantool-patches] Re: [PATCH 2/2] sql: use vtab::rowid_next() instead of index_max()

[Vladislav Shpilevoy]

Thanks for the patch!

I moved the new error code there, and removed errinj test. My review fixes
here and on the branch:

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

diff --git a/src/box/errcode.h b/src/box/errcode.h
index 04f4f34ee..d64b6f3ba 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -223,6 +223,7 @@ struct errcode_record {
  	/*168 */_(ER_DROP_FK_CONSTRAINT,	"Failed to drop foreign key constraint '%s': %s") \
  	/*169 */_(ER_NO_SUCH_CONSTRAINT,	"Constraint %s does not exist") \
  	/*170 */_(ER_CONSTRAINT_EXISTS,		"Constraint %s already exists") \
+	/*171 */_(ER_ROWID_OVERFLOW,            "Rowid is overflowed: too many entries in ephemeral space") \
  
  /*
   * !IMPORTANT! Please follow instructions at start of the file
diff --git a/test/sql/errinj.result b/test/sql/errinj.result
index 60f776c3c..a0ba60f45 100644
--- a/test/sql/errinj.result
+++ b/test/sql/errinj.result
@@ -279,18 +279,3 @@ errinj.set("ERRINJ_WAL_IO", false)
  box.sql.execute("DROP TABLE t3;")
  ---
  ...
--- Make sure that overflow of rowid used for ephemeral spaces
--- is hadnled properly.
---
-box.error.injection.set("ERRINJ_ROWID_OVERFLOW", true)
----
-- ok
-...
-box.sql.execute("WITH RECURSIVE tmp AS (SELECT 1 UNION ALL SELECT * FROM tmp LIMIT 2) SELECT * FROM tmp;")
----
-- error: 'Rowid is overflowed: too many entries in ephemeral space'
-...
-box.error.injection.set("ERRINJ_ROWID_OVERFLOW", false)
----
-- ok
-...
diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
index 034a43d4e..25d73f0c2 100644
--- a/test/sql/errinj.test.lua
+++ b/test/sql/errinj.test.lua
@@ -97,10 +97,3 @@ box.sql.execute("ALTER TABLE t3 DROP CONSTRAINT fk1;")
  box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
  errinj.set("ERRINJ_WAL_IO", false)
  box.sql.execute("DROP TABLE t3;")
-
--- Make sure that overflow of rowid used for ephemeral spaces
--- is hadnled properly.
---
-box.error.injection.set("ERRINJ_ROWID_OVERFLOW", true)
-box.sql.execute("WITH RECURSIVE tmp AS (SELECT 1 UNION ALL SELECT * FROM tmp LIMIT 2) SELECT * FROM tmp;")
-box.error.injection.set("ERRINJ_ROWID_OVERFLOW", false)

[tarantool-patches] Re: [PATCH 1/2] space: add method to fetch next rowid

[Vladislav Shpilevoy]

Hi! Thanks for the patch! I understand, that Vova said
that it should not be pushed, but Kirill asked me, on
the contrary, to review it. So I do.

On 29/10/2018 22:02, Nikita Pettik wrote:
> Ephemeral space are extensively used in SQL to store intermediate
> results of query processing. To keep things simple, they feature only
> one unique index (primary) which covers all fields. However, ephemeral
> space can be used to store non-unique entries. In this case, one
> additional field added to the end if stored data:
> 
> [field1, ... fieldn, rowid]
> 
> Note that it can't be added to the beginning of tuple since data in
> ephemeral space may be kept as sorted. Previously, to generate proper
> rowid index_max() was used. However, it is obviously wrong way to do it.
> Hence, lets add simple integer counter to memtx space (ephemeral spaces
> are valid only for memtx engine) and introduce method in vtab to fetch
> next rowid value.
> 
> Needed for #3297
> ---
>   src/box/blackhole.c    |  1 +
>   src/box/errcode.h      |  2 ++
>   src/box/memtx_space.c  | 17 +++++++++++++++++
>   src/box/memtx_space.h  |  7 +++++++
>   src/box/space.c        |  9 +++++++++
>   src/box/space.h        |  3 +++
>   src/box/sysview.c      |  1 +
>   src/box/vinyl.c        |  1 +
>   src/errinj.h           |  1 +
>   test/box/errinj.result |  2 ++
>   test/box/misc.result   |  1 +
>   11 files changed, 45 insertions(+)
> 
> index 04f4f34ee..fab8b6617 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -223,6 +223,8 @@ struct errcode_record {
>   	/*168 */_(ER_DROP_FK_CONSTRAINT,	"Failed to drop foreign key constraint '%s': %s") \
>   	/*169 */_(ER_NO_SUCH_CONSTRAINT,	"Constraint %s does not exist") \
>   	/*170 */_(ER_CONSTRAINT_EXISTS,		"Constraint %s already exists") \
> +	/*171 */_(ER_ROWID_OVERFLOW,		"Rowid is overflowed: too many entries in ephemeral space") \
> +

This error message as well as check on uint64_max are
not necessary, IMHO. I can not imagine how many hundreds of
years a one should insert into one ephemeral table to
reach this limit.

Also, one extra empty line.

 From this patch I removed errinj and moved rowid_overflow to
the next patch since it stores ids in 32 bits and here
the overflow is much more feasible.

Review fixes here and on the branch:

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

diff --git a/src/box/errcode.h b/src/box/errcode.h
index fab8b6617..04f4f34ee 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -223,8 +223,6 @@ struct errcode_record {
  	/*168 */_(ER_DROP_FK_CONSTRAINT,	"Failed to drop foreign key constraint '%s': %s") \
  	/*169 */_(ER_NO_SUCH_CONSTRAINT,	"Constraint %s does not exist") \
  	/*170 */_(ER_CONSTRAINT_EXISTS,		"Constraint %s already exists") \
-	/*171 */_(ER_ROWID_OVERFLOW,		"Rowid is overflowed: too many entries in ephemeral space") \
-
  
  /*
   * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 075b86f9d..e810011c8 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -606,12 +606,6 @@ memtx_space_ephemeral_rowid_next(struct space *space, uint64_t *rowid)
  {
  	assert(rowid != NULL);
  	struct memtx_space *memtx_space = (struct memtx_space *)space;
-	ERROR_INJECT(ERRINJ_ROWID_OVERFLOW,
-		     { memtx_space->rowid = UINT64_MAX; });
-	if (unlikely(memtx_space->rowid == UINT64_MAX)) {
-		diag_set(ClientError, ER_ROWID_OVERFLOW);
-		return -1;
-	}
  	*rowid = memtx_space->rowid++;
  	return 0;
  }
diff --git a/src/errinj.h b/src/errinj.h
index 58aa5864a..84a1fbb5e 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -121,7 +121,6 @@ struct errinj {
  	_(ERRINJ_RELAY_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \
  	_(ERRINJ_WAL_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \
  	_(ERRINJ_VY_COMPACTION_DELAY, ERRINJ_BOOL, {.bparam = false}) \
-	_(ERRINJ_ROWID_OVERFLOW, ERRINJ_BOOL, {.bparam = false}) \
  
  ENUM0(errinj_id, ERRINJ_LIST);
  extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 191e16911..c4a1326cd 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -54,8 +54,6 @@ errinj.info()
      state: false
    ERRINJ_RELAY_REPORT_INTERVAL:
      state: 0
-  ERRINJ_ROWID_OVERFLOW:
-    state: false
    ERRINJ_VY_READ_PAGE_TIMEOUT:
      state: 0
    ERRINJ_XLOG_META:

[tarantool-patches] Re: [PATCH 1/2] space: add method to fetch next rowid

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 3313 bytes --]



> On 9 Nov 2018, at 12:25, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the patch! I understand, that Vova said
> that it should not be pushed, but Kirill asked me, on
> the contrary, to review it. So I do.

Vladimir didn’t suggest better solutions except for complete reworking
them. Now it is definitely bug which leads to wrong results of
SELECT queries (which is a terrible thing taking into account the fact that
SQL is supposed to be used mostly for DQL). So lets take this patch as
a workaround and rework ephemeral tables when we will have enough time
and resources (surely if Kirill and Vladimir don’t mind).

With this bug it seems to be unacceptable to release beta version.

> On 29/10/2018 22:02, Nikita Pettik wrote:
>> Ephemeral space are extensively used in SQL to store intermediate
>> results of query processing. To keep things simple, they feature only
>> one unique index (primary) which covers all fields. However, ephemeral
>> space can be used to store non-unique entries. In this case, one
>> additional field added to the end if stored data:
>> [field1, ... fieldn, rowid]
>> Note that it can't be added to the beginning of tuple since data in
>> ephemeral space may be kept as sorted. Previously, to generate proper
>> rowid index_max() was used. However, it is obviously wrong way to do it.
>> Hence, lets add simple integer counter to memtx space (ephemeral spaces
>> are valid only for memtx engine) and introduce method in vtab to fetch
>> next rowid value.
>> Needed for #3297
>> ---
>>  src/box/blackhole.c    |  1 +
>>  src/box/errcode.h      |  2 ++
>>  src/box/memtx_space.c  | 17 +++++++++++++++++
>>  src/box/memtx_space.h  |  7 +++++++
>>  src/box/space.c        |  9 +++++++++
>>  src/box/space.h        |  3 +++
>>  src/box/sysview.c      |  1 +
>>  src/box/vinyl.c        |  1 +
>>  src/errinj.h           |  1 +
>>  test/box/errinj.result |  2 ++
>>  test/box/misc.result   |  1 +
>>  11 files changed, 45 insertions(+)
>> index 04f4f34ee..fab8b6617 100644
>> --- a/src/box/errcode.h
>> +++ b/src/box/errcode.h
>> @@ -223,6 +223,8 @@ struct errcode_record {
>>  	/*168 */_(ER_DROP_FK_CONSTRAINT,	"Failed to drop foreign key constraint '%s': %s") \
>>  	/*169 */_(ER_NO_SUCH_CONSTRAINT,	"Constraint %s does not exist") \
>>  	/*170 */_(ER_CONSTRAINT_EXISTS,		"Constraint %s already exists") \
>> +	/*171 */_(ER_ROWID_OVERFLOW,		"Rowid is overflowed: too many entries in ephemeral space") \
>> +
> 
> This error message as well as check on uint64_max are
> not necessary, IMHO. I can not imagine how many hundreds of
> years a one should insert into one ephemeral table to
> reach this limit.

It is true that 2^64 is likely to be quite huge number of tuples,
but for instance JOIN uses nested-loop algorithm, so it requires
n^2 memory for ephemeral table to comprise results.
In this regard, to reach the limit we need 4-way join where each
table contains 2^16 entries, which in turn doesn’t seem to be giant.

*It is only thoughts tho, I haven’t tested it since I suppose very likely
 my pc would simply get stuck.*

I wanted to create long test as the easiest solution, but Alexander warned
me that Travis may not survive such test due to lack of memory.



[-- Attachment #2: Type: text/html, Size: 11822 bytes --]

[tarantool-patches] Re: [PATCH 1/2] space: add method to fetch next rowid

[Vladislav Shpilevoy]

On 12/11/2018 02:16, n.pettik wrote:
> 
> 
>> On 9 Nov 2018, at 12:25, Vladislav Shpilevoy <v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org>> wrote:
>>
>> Hi! Thanks for the patch! I understand, that Vova said
>> that it should not be pushed, but Kirill asked me, on
>> the contrary, to review it. So I do.
> 
> Vladimir didn’t suggest better solutions except for complete reworking
> them. Now it is definitely bug which leads to wrong results of
> SELECT queries (which is a terrible thing taking into account the fact that
> SQL is supposed to be used mostly for DQL). So lets take this patch as
> a workaround and rework ephemeral tables when we will have enough time
> and resources (surely if Kirill and Vladimir don’t mind).
> 
> With this bug it seems to be unacceptable to release beta version.
> 
>> On 29/10/2018 22:02, Nikita Pettik wrote:
>>> Ephemeral space are extensively used in SQL to store intermediate
>>> results of query processing. To keep things simple, they feature only
>>> one unique index (primary) which covers all fields. However, ephemeral
>>> space can be used to store non-unique entries. In this case, one
>>> additional field added to the end if stored data:
>>> [field1, ... fieldn, rowid]
>>> Note that it can't be added to the beginning of tuple since data in
>>> ephemeral space may be kept as sorted. Previously, to generate proper
>>> rowid index_max() was used. However, it is obviously wrong way to do it.
>>> Hence, lets add simple integer counter to memtx space (ephemeral spaces
>>> are valid only for memtx engine) and introduce method in vtab to fetch
>>> next rowid value.
>>> Needed for #3297
>>> ---
>>>  src/box/blackhole.c    |  1 +
>>>  src/box/errcode.h      |  2 ++
>>>  src/box/memtx_space.c  | 17 +++++++++++++++++
>>>  src/box/memtx_space.h  |  7 +++++++
>>>  src/box/space.c        |  9 +++++++++
>>>  src/box/space.h        |  3 +++
>>>  src/box/sysview.c      |  1 +
>>>  src/box/vinyl.c        |  1 +
>>>  src/errinj.h           |  1 +
>>>  test/box/errinj.result |  2 ++
>>>  test/box/misc.result   |  1 +
>>>  11 files changed, 45 insertions(+)
>>> index 04f4f34ee..fab8b6617 100644
>>> --- a/src/box/errcode.h
>>> +++ b/src/box/errcode.h
>>> @@ -223,6 +223,8 @@ struct errcode_record {
>>> /*168 */_(ER_DROP_FK_CONSTRAINT,"Failed to drop foreign key constraint '%s': %s") \
>>> /*169 */_(ER_NO_SUCH_CONSTRAINT,"Constraint %s does not exist") \
>>> /*170 */_(ER_CONSTRAINT_EXISTS,"Constraint %s already exists") \
>>> +/*171 */_(ER_ROWID_OVERFLOW,"Rowid is overflowed: too many entries in ephemeral space") \
>>> +
>>
>> This error message as well as check on uint64_max are
>> not necessary, IMHO. I can not imagine how many hundreds of
>> years a one should insert into one ephemeral table to
>> reach this limit.
> 
> It is true that 2^64 is likely to be quite huge number of tuples,
> but for instance JOIN uses nested-loop algorithm, so it requires
> n^2 memory for ephemeral table to comprise results.
> In this regard, to reach the limit we need 4-way join where each
> table contains 2^16 entries, which in turn doesn’t seem to be giant.
> 
> *It is only thoughts tho, I haven’t tested it since I suppose very likely
>   my pc would simply get stuck.*
> 
> I wanted to create long test as the easiest solution, but Alexander warned
> me that Travis may not survive such test due to lack of memory.
> 
> 

I do not mind, if you drop my fixes. It is just nitpicking. The
patchset is generally ok already.

[tarantool-patches] Re: [PATCH 1/2] space: add method to fetch next rowid

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 3816 bytes --]



> On 12 Nov 2018, at 02:22, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> On 12/11/2018 02:16, n.pettik wrote:
>>> On 9 Nov 2018, at 12:25, Vladislav Shpilevoy <v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org>> wrote:
>>> 
>>> Hi! Thanks for the patch! I understand, that Vova said
>>> that it should not be pushed, but Kirill asked me, on
>>> the contrary, to review it. So I do.
>> Vladimir didn’t suggest better solutions except for complete reworking
>> them. Now it is definitely bug which leads to wrong results of
>> SELECT queries (which is a terrible thing taking into account the fact that
>> SQL is supposed to be used mostly for DQL). So lets take this patch as
>> a workaround and rework ephemeral tables when we will have enough time
>> and resources (surely if Kirill and Vladimir don’t mind).
>> With this bug it seems to be unacceptable to release beta version.
>>> On 29/10/2018 22:02, Nikita Pettik wrote:
>>>> Ephemeral space are extensively used in SQL to store intermediate
>>>> results of query processing. To keep things simple, they feature only
>>>> one unique index (primary) which covers all fields. However, ephemeral
>>>> space can be used to store non-unique entries. In this case, one
>>>> additional field added to the end if stored data:
>>>> [field1, ... fieldn, rowid]
>>>> Note that it can't be added to the beginning of tuple since data in
>>>> ephemeral space may be kept as sorted. Previously, to generate proper
>>>> rowid index_max() was used. However, it is obviously wrong way to do it.
>>>> Hence, lets add simple integer counter to memtx space (ephemeral spaces
>>>> are valid only for memtx engine) and introduce method in vtab to fetch
>>>> next rowid value.
>>>> Needed for #3297
>>>> ---
>>>>  src/box/blackhole.c    |  1 +
>>>>  src/box/errcode.h      |  2 ++
>>>>  src/box/memtx_space.c  | 17 +++++++++++++++++
>>>>  src/box/memtx_space.h  |  7 +++++++
>>>>  src/box/space.c        |  9 +++++++++
>>>>  src/box/space.h        |  3 +++
>>>>  src/box/sysview.c      |  1 +
>>>>  src/box/vinyl.c        |  1 +
>>>>  src/errinj.h           |  1 +
>>>>  test/box/errinj.result |  2 ++
>>>>  test/box/misc.result   |  1 +
>>>>  11 files changed, 45 insertions(+)
>>>> index 04f4f34ee..fab8b6617 100644
>>>> --- a/src/box/errcode.h
>>>> +++ b/src/box/errcode.h
>>>> @@ -223,6 +223,8 @@ struct errcode_record {
>>>> /*168 */_(ER_DROP_FK_CONSTRAINT,"Failed to drop foreign key constraint '%s': %s") \
>>>> /*169 */_(ER_NO_SUCH_CONSTRAINT,"Constraint %s does not exist") \
>>>> /*170 */_(ER_CONSTRAINT_EXISTS,"Constraint %s already exists") \
>>>> +/*171 */_(ER_ROWID_OVERFLOW,"Rowid is overflowed: too many entries in ephemeral space") \
>>>> +
>>> 
>>> This error message as well as check on uint64_max are
>>> not necessary, IMHO. I can not imagine how many hundreds of
>>> years a one should insert into one ephemeral table to
>>> reach this limit.
>> It is true that 2^64 is likely to be quite huge number of tuples,
>> but for instance JOIN uses nested-loop algorithm, so it requires
>> n^2 memory for ephemeral table to comprise results.
>> In this regard, to reach the limit we need 4-way join where each
>> table contains 2^16 entries, which in turn doesn’t seem to be giant.
>> *It is only thoughts tho, I haven’t tested it since I suppose very likely
>>  my pc would simply get stuck.*
>> I wanted to create long test as the easiest solution, but Alexander warned
>> me that Travis may not survive such test due to lack of memory.
> 
> I do not mind, if you drop my fixes. It is just nitpicking. The
> patchset is generally ok already.

Actually, I don’t mind your fixes as well, so I am going to apply them.
Also, I’ve rebased patch-set on fresh 2.1.


[-- Attachment #2: Type: text/html, Size: 7814 bytes --]

[tarantool-patches] Re: [PATCH 0/2] Re-implement rowid generation for ephemeral spaces

[Kirill Yukhin]

Hello,
On 29 Oct 22:02, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-3297-ephemeral-rowid
> Issue: https://github.com/tarantool/tarantool/issues/3297
> 
> This patch-set fixes incorrect rowid generation for ephemeral spaces.
> To achieve this, we introduce separate method in space's vtab,
> which currently available only for memtx engine (as well as ephemeral
> spaces). It simply increments built-in counter and returs new rowid.
> It allows us to get rid of calling index_max() which obviously was
> wrong way at calcucaling next id (since rowid field is taken to be
> last in stored tuples).

I've checked in the patch set into 2.1 branch.

--
Regards, Kirill Yukhin

[tarantool-patches] Re: [PATCH 1/2] space: add method to fetch next rowid

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [18/11/12 16:12]:
> 
> It is true that 2^64 is likely to be quite huge number of tuples,
> but for instance JOIN uses nested-loop algorithm, so it requires
> n^2 memory for ephemeral table to comprise results.
> In this regard, to reach the limit we need 4-way join where each
> table contains 2^16 entries, which in turn doesn’t seem to be giant.
> 
> *It is only thoughts tho, I haven’t tested it since I suppose very likely
>  my pc would simply get stuck.*

intel 64 bit architecture can not address more than 48 bits.

Apart from the fact that you can multiply 4 numbers in a cross
join there is a question how much time this nested loop is going
to run. Imagine it runs 2^48 processor ticks. It's several weeks
by a conservative estimate.
> 
> I wanted to create long test as the easiest solution, but Alexander warned
> me that Travis may not survive such test due to lack of memory.
> 
> 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov