[tarantool-patches] Re: [PATCH] Fix ephemeral table next rowid generation

["n.pettik" <korablev@tarantool.org>]

Lets follow SOP conventions: put link to the branch and link to the issue after ‘—‘.

Moreover, add ‘sql’ prefix to commit subject.

As for patch itself, I don’t like the idea to store rowid for ephemeral space
in cursor: if insertion in such space occurred not via SQL facilities, then
rowid would be wrong…Well, we know that rowid is always in
the last field, so why just don’t fetch value from last field and increment it?

> On 15 May 2018, at 22:49, AKhatskevich <avkhatskevich@tarantool.org> wrote:
> 
> branch: kh/gh-3297-fix_ephem_id-2
> notes: Kiril asked me to store rowid in BtCursor (implemented in this patch)
>       however I prefer to create a single static variable for the whole
>       tarantool (rowid just need to be unique in a space) as implemented
>       here: kh/gh-3297-fix_ephem_id-1
> 
> ------ commit message ------------
> 
> Next `rowid` value is stored in BtCursor, and is incremented after each
> insert to the ephemeral space.

It would be better if you explained provided changes. Without explanation
it is quite complicated to understand aim of the patch.
Also, in this patch, for instance, you have removed field from BtCursor,
so lets mention this fact.

> 
> Closes #3297
> ---
> src/box/sql.c                                 | 33 +--------------------------
> src/box/sql/cursor.h                          |  2 +-
> src/box/sql/insert.c                          | 16 +++----------
> src/box/sql/select.c                          | 10 ++++----
> src/box/sql/tarantoolInt.h                    |  2 --
> src/box/sql/vdbe.c                            | 25 ++++++++------------
> test/sql-tap/gh-3297_ephemeral_rowid.test.lua | 30 ++++++++++++++++++++++++
> 7 files changed, 49 insertions(+), 69 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 6104a6d0f..cd7ac8a50 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -423,6 +423,7 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
> 	}
> 	pCur->space = ephemer_new_space;
> 	pCur->index = *ephemer_new_space->index;
> +	pCur->ephemeral_rowid = 0;
> 
> 	int unused;
> 	return tarantoolSqlite3First(pCur, &unused);
> @@ -1080,7 +1081,6 @@ key_alloc(BtCursor *cur, size_t key_size)
> 		}
> 		cur->key = new_key;
> 	}
> -	cur->nKey = key_size;
> 	return 0;
> }
> 
> @@ -1621,37 +1621,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 pCur Cursor pointing 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(BtCursor *pCur, uint32_t fieldno,
> -				       uint64_t *max_id)
> -{
> -	struct space *ephem_space = pCur->space;
> -	assert(ephem_space);
> -	struct index *primary_index = *ephem_space->index;
> -
> -	struct tuple *tuple;
> -	if (index_max(primary_index, NULL, 0, &tuple) != 0) {
> -		return SQL_TARANTOOL_ERROR;
> -	}
> -	if (tuple == NULL) {
> -		*max_id = 0;
> -		return SQLITE_OK;
> -	}
> -	if (tuple_field_u64(tuple, fieldno, max_id) == -1)
> -		return SQL_TARANTOOL_ERROR;
> -
> -	return SQLITE_OK;
> -}
> -
> int
> tarantoolSqlNextSeqId(uint64_t *max_id)
> {
> diff --git a/src/box/sql/cursor.h b/src/box/sql/cursor.h
> index c55941784..6958a9f23 100644
> --- a/src/box/sql/cursor.h
> +++ b/src/box/sql/cursor.h
> @@ -57,7 +57,7 @@ typedef struct BtCursor BtCursor;
>  * for ordinary space, or to TEphemCursor for ephemeral space.
>  */
> struct BtCursor {
> -	i64 nKey;		/* Size of pKey, or last integer key */
> +	u64 ephemeral_rowid;	/* Next unique id for Ephemeral space */
> 	u8 curFlags;		/* zero or more BTCF_* flags defined below */
> 	u8 eState;		/* One of the CURSOR_XXX constants (see below) */
> 	u8 hints;		/* As configured by CursorSetHints() */
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 0b9e75a93..bddfdc50c 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -557,15 +557,12 @@ 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++;
> 			regRec = sqlite3GetTempReg(pParse);
> -			regCopy = sqlite3GetTempRange(pParse, nColumn);
> -			regTempId = sqlite3GetTempReg(pParse);
> +			regCopy = sqlite3GetTempRange(pParse, nColumn + 1);
> 			struct key_def *def = key_def_new(nColumn + 1);
> 			if (def == NULL) {
> 				sqlite3OomFault(db);
> @@ -573,16 +570,10 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
> 			}
> 			sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, srcTab, nColumn+1,
> 					  0, (char*)def, P4_KEYDEF);
> -			/* 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, srcTab,
> +					  regCopy + nColumn);
> 			sqlite3VdbeAddOp3(v, OP_Copy, regFromSelect, regCopy, nColumn-1);
> 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy,
> 					  nColumn + 1, regRec);
> @@ -593,7 +584,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 1f27efdb5..5cd289ccc 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1047,8 +1047,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, iParm,
> -						  nResultCol, regCopy + nResultCol);
> +				sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, iParm,
> +						  regCopy + nResultCol);
> 				/* Positioning ID column to be last in inserted tuple.
> 				 * NextId -> regCopy + n + 1
> 				 * Copy [regResult, regResult + n] -> [regCopy, regCopy + n]
> @@ -1418,7 +1418,7 @@ generateSortTail(Parse * pParse,	/* Parsing context */
> 	case SRT_Table:
> 	case SRT_EphemTab: {
> 			int regCopy = sqlite3GetTempRange(pParse,  nColumn);
> -			sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, iParm, nColumn, regTupleid);
> +			sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, iParm, regTupleid);
> 			sqlite3VdbeAddOp3(v, OP_Copy, regRow, regCopy, nSortData - 1);
> 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy, nColumn + 1, regRow);
> 			sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, regRow);
> @@ -2898,8 +2898,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->iSDParm,
> -					  in->nSdst, regCopy + in->nSdst);
> +			sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, dest->iSDParm,
> +					  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 94f94cb44..7da1f423b 100644
> --- a/src/box/sql/tarantoolInt.h
> +++ b/src/box/sql/tarantoolInt.h
> @@ -116,8 +116,6 @@ int tarantoolSqlite3EphemeralDelete(BtCursor * pCur);
> int tarantoolSqlite3EphemeralCount(BtCursor * pCur, i64 * pnEntry);
> int tarantoolSqlite3EphemeralDrop(BtCursor * pCur);
> int tarantoolSqlite3EphemeralClearTable(BtCursor * pCur);
> -int tarantoolSqlite3EphemeralGetMaxId(BtCursor * pCur, uint32_t fieldno,
> -				       uint64_t * max_id);
> 
> /**
>  * Performs exactly as extract_key + sqlite3VdbeCompareMsgpack,
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 89c35d218..743d6e8c5 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -3790,27 +3790,20 @@ 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_index[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 stores next `rowid` for the ephemeral space to P2 register.
> + * `rowid` is necessary, because inserted to ephemeral space tuples may be not
> + * unique, while Tarantool`s spaces can contain only unique tuples.
>  */
> case OP_NextIdEphemeral: {
> 	VdbeCursor *pC;
> -	int p2;
> 	pC = p->apCsr[pOp->p1];
> -	p2 = pOp->p2;
> -	pOut = &aMem[pOp->p3];
> -
> -	assert(pC->uc.pCursor->curFlags & BTCF_TEphemCursor);
> -
> -	rc = tarantoolSqlite3EphemeralGetMaxId(pC->uc.pCursor, p2,
> -					       (uint64_t *) &pOut->u.i);
> -	if (rc) goto abort_due_to_error;
> -
> -	pOut->u.i += 1;
> +	pOut = &aMem[pOp->p2];
> +	struct BtCursor * bt_cur = pC->uc.pCursor;
> +	assert(bt_cur->curFlags & BTCF_TEphemCursor);
> +	pOut->u.i = bt_cur->ephemeral_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..ed596e7ad
> --- /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 primary key);
> +    CREATE TABLE t2(a primary key, b);
> +    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()
> -- 
> 2.14.1
> 
>