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

[Kirill Yukhin <kyukhin@tarantool.org>]

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.
> Only field count makes sense in such a case. Hoewever it is
> passed separately (in P2). So, allow P4 field to be NULL for
> OP_OpenTEphemeral. Update delte from routine from sql/delete.c
> accordingly.
> 
> Part of #3235
Pasting updated patch from 1st round of review of 1/2 patch.

diff --git a/src/box/sql.c b/src/box/sql.c
index 6104a6d..e502f22 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/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);
 			/* 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..da4d816 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2215,13 +2215,8 @@ 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);
+				  (char*)NULL, P4_KEYDEF);
 		VdbeComment((v, "Queue table"));
 	}
 	if (iDistinct) {
@@ -2422,13 +2417,8 @@ 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);
+				  0, (char*)NULL, P4_KEYDEF);
 		VdbeComment((v, "Destination temp"));
 		dest.eDest = SRT_Table;
 	}
@@ -5608,11 +5598,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);
+				  pEList->nExpr + 1, 0, (char*)NULL, P4_KEYDEF);
 
 		VdbeComment((v, "Output table"));
 	}
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index f3db92c..de1349b 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);
+					     nKey, 0, (char*)NULL, P4_KEYDEF);
 	} 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 b4b59da..c6710d2 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;
 }