[tarantool-patches] Re: [PATCH 1/2] sql: allow key_def to be NULL for ephemeral create

[Kirill Yukhin <kyukhin@tarantool.org>]

On 18 мая 13:33, Vladislav Shpilevoy wrote:
> Hello. Thanks for fixes!
> 
> > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> > index 3455f52..2c1ce44 100644
> > --- a/src/box/sql/delete.c
> > +++ b/src/box/sql/delete.c
> > @@ -386,14 +386,9 @@ sqlite3DeleteFrom(Parse * pParse,	/* The parser context */
> >   			iPk = pParse->nMem + 1;
> >   			pParse->nMem += nPk;
> >   			iEphCur = pParse->nTab++;
> > -			struct key_def *def = key_def_new(nPk);
> > -			if (def == NULL) {
> > -				sqlite3OomFault(db);
> > -				goto delete_from_cleanup;
> > -			}
> >   			addrEphOpen =
> >   				sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iEphCur,
> > -						  nPk, 0, (char*)def, P4_KEYDEF);
> > +						  nPk);
> 
> Compilation error.
Whoops, fixed.

--
Regards, Kirill Yukhin

commit 8640d51f1fb60022b4b61b84fd5c6111c5dbe916
Author: Kirill Yukhin <kyukhin@tarantool.org>
Date:   Wed May 16 09:47:02 2018 +0300

    sql: allow key_def to be NULL for ephemeral create
    
    There're many cases in SQL FE, where exact key def passed to
    ephemeral table doesn't matter. It is used to store data only.
    Only field count makes sense in such a case. However it is
    passed separately (in P2). So, allow P4 field to be NULL for
    OP_OpenTEphemeral. Update delete from routine from sql/delete.c
    accordingly.
    
    Part of #3235

diff --git a/src/box/sql.c b/src/box/sql.c
index 195fdfb..6d2e0b9 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -370,8 +370,9 @@ int tarantoolSqlite3Count(BtCursor *pCur, i64 *pnEntry)
  *
  * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
  */
-int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
-				    struct key_def *def)
+int
+tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
+				struct key_def *def)
 {
 	assert(pCur);
 	assert(pCur->curFlags & BTCF_TEphemCursor);
@@ -381,7 +382,7 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
 		return SQL_TARANTOOL_ERROR;
 	for (uint32_t part = 0; part < field_count; ++part) {
 		struct coll *coll;
-		if (part < def->part_count)
+		if (def != NULL && part < def->part_count)
 			coll = def->parts[part].coll;
 		else
 			coll = NULL;
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 3455f52..1a2f3d0 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -386,14 +386,9 @@ sqlite3DeleteFrom(Parse * pParse,	/* The parser context */
 			iPk = pParse->nMem + 1;
 			pParse->nMem += nPk;
 			iEphCur = pParse->nTab++;
-			struct key_def *def = key_def_new(nPk);
-			if (def == NULL) {
-				sqlite3OomFault(db);
-				goto delete_from_cleanup;
-			}
 			addrEphOpen =
-				sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iEphCur,
-						  nPk, 0, (char*)def, P4_KEYDEF);
+				sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEphCur,
+						  nPk);
 		} else {
 			pPk = sqlite3PrimaryKeyIndex(pTab);
 			assert(pPk != 0);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 5ada87e..54a7e4a 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -566,13 +566,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 			regRec = sqlite3GetTempReg(pParse);
 			regCopy = sqlite3GetTempRange(pParse, nColumn);
 			regTempId = sqlite3GetTempReg(pParse);
-			struct key_def *def = key_def_new(nColumn + 1);
-			if (def == NULL) {
-				sqlite3OomFault(db);
-				goto insert_cleanup;
-			}
-			sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, srcTab, nColumn+1,
-					  0, (char*)def, P4_KEYDEF);
+			sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, srcTab, nColumn+1);
 			/* Create counter for rowid */
 			sqlite3VdbeAddOp4Dup8(v, OP_Int64,
 					      0 /* unused */,
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index bd895bc..de111d3 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2215,13 +2215,7 @@ generateWithRecursiveQuery(Parse * pParse,	/* Parsing context */
 		VdbeComment((v, "Orderby table"));
 		destQueue.pOrderBy = pOrderBy;
 	} else {
-		struct key_def *def = key_def_new(nCol + 1);
-		if (def == NULL) {
-			sqlite3OomFault(pParse->db);
-			goto end_of_recursive_query;
-		}
-		sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iQueue, nCol + 1, 0,
-				  (char*)def, P4_KEYDEF);
+		sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iQueue, nCol + 1);
 		VdbeComment((v, "Queue table"));
 	}
 	if (iDistinct) {
@@ -2422,13 +2416,7 @@ multiSelect(Parse * pParse,	/* Parsing context */
 	if (dest.eDest == SRT_EphemTab) {
 		assert(p->pEList);
 		int nCols = p->pEList->nExpr;
-		struct key_def *def = key_def_new(nCols + 1);
-		if (def == NULL) {
-			sqlite3OomFault(db);
-			goto multi_select_end;
-		}
-		sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, dest.iSDParm, nCols + 1,
-				  0, (char*)def, P4_KEYDEF);
+		sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, dest.iSDParm, nCols + 1);
 		VdbeComment((v, "Destination temp"));
 		dest.eDest = SRT_Table;
 	}
@@ -5608,11 +5596,8 @@ sqlite3Select(Parse * pParse,		/* The parser context */
 	/* If the output is destined for a temporary table, open that table.
 	 */
 	if (pDest->eDest == SRT_EphemTab) {
-		struct key_def *def = key_def_new(pEList->nExpr + 1);
-		if (def == NULL)
-			sqlite3OomFault(db);
-		sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, pDest->iSDParm,
-				  pEList->nExpr + 1, 0, (char*)def, P4_KEYDEF);
+		sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, pDest->iSDParm,
+				  pEList->nExpr + 1);
 
 		VdbeComment((v, "Output table"));
 	}
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index f3db92c..204adc2 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -358,13 +358,8 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	sqlite3VdbeAddOp2(v, OP_Null, 0, iPk);
 
 	if (isView) {
-		struct key_def *def = key_def_new(nKey);
-		if (def == NULL) {
-			sqlite3OomFault(db);
-			goto update_cleanup;
-		}
-		addrOpen = sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, iEph,
-					     nKey, 0, (char*)def, P4_KEYDEF);
+		addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph,
+					     nKey);
 	} else {
 		addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, nPk);
 		sql_vdbe_set_p4_key_def(pParse, pPk);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e27fe2f..455198d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3225,9 +3225,9 @@ open_cursor_set_hints:
 /**
  * Opcode: OpenTEphemeral P1 P2 * P4 *
  * Synopsis:
- * @param P1 index of new cursor to be created
- * @param P2 number of columns in a new table
- * @param P4 key def for new table
+ * @param P1 index of new cursor to be created.
+ * @param P2 number of columns in a new table.
+ * @param P4 key def for new table, NULL is allowed.
  *
  * This opcode creates Tarantool's ephemeral table and sets cursor P1 to it.
  */
@@ -3236,21 +3236,20 @@ case OP_OpenTEphemeral: {
 	BtCursor *pBtCur;
 	assert(pOp->p1 >= 0);
 	assert(pOp->p2 > 0);
-	assert(pOp->p4.key_def != NULL);
-	assert(pOp->p4type == P4_KEYDEF);
+	assert(pOp->p4type != P4_KEYDEF || pOp->p4.key_def != NULL);
 
 	pCx = allocateCursor(p, pOp->p1, pOp->p2, CURTYPE_TARANTOOL);
 	if (pCx == 0) goto no_mem;
 	pCx->nullRow = 1;
 
-	pCx->key_def  = pOp->p4.key_def;
 	pBtCur = pCx->uc.pCursor;
 	/* Ephemeral spaces don't have space_id */
 	pBtCur->eState = CURSOR_INVALID;
 	pBtCur->curFlags = BTCF_TEphemCursor;
 
 	rc = tarantoolSqlite3EphemeralCreate(pCx->uc.pCursor, pOp->p2,
-					     pCx->key_def);
+					     pOp->p4.key_def);
+	pCx->key_def = pCx->uc.pCursor->index->def->key_def;
 	if (rc) goto abort_due_to_error;
 	break;
 }