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

[Kirill Yukhin <kyukhin@tarantool.org>]

Hi Vlad!
Thanks for uyour inputs.

My answers inlined, patch in the bottom.

On 17 мая 19:47, Vladislav Shpilevoy wrote:
> Thanks for review fixes! See 4 comments below.
> 
> On 17/05/2018 18:49, Kirill Yukhin wrote:
> > Hi Vlad,
> > 
> > On 16 мая 18:24, Kirill Yukhin wrote:
> > > There're many cases in SQL FE, where exact key def passed to
> > > doesn't ephemeral table matter. It is used to store data only.
> 
> 1. Can't read. Maybe 'where exact key def passed to ephemeral table doesn't matter' ?
> 
> > > Only field count makes sense in such a case. Hoewever it is
> 
> 2. However.
> 
> > > passed separately (in P2). So, allow P4 field to be NULL for
> > > OP_OpenTEphemeral. Update delte from routine from sql/delete.c
> 
> 3. delete.
> 
> > > accordingly.
> > > 
> > > Part of #3235
> > Pasting updated patch from 1st round of review of 1/2 patch.
> > 
> > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> > index 22130ef..1ad7469 100644
> > --- a/src/box/sql/insert.c
> > +++ b/src/box/sql/insert.c
> > @@ -566,13 +566,8 @@ 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);
> > +					  0, (char*)NULL, P4_KEYDEF);
> 
> 4. In the OP_OpenTEphemeral code you check for P4_KEYDEF absence, so you can use
> AddOp2 here. And in other places.
Fixed around the code.

--
Regards, Kirill Yukhin

commit 9e5ed7bb5e0d01ff5b8f9d5c6af82e915135fcb0
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..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);
 		} 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;
 }