[tarantool-patches] [PATCH] sql: repair SQL cursor handling opcodes

[Kirill Yukhin <kyukhin@tarantool.org>]

From Tarantool's backend perspective there's no
difference between read and write cursor. More preciesly,
write cursor are useless and are used to store information
about where to update data. The patch removes legacy
OpenRead/OpenWrite op-codes and replaces them w/ single
OpenCursor. It also rename ReopenIndex to CursorReopen for
the sake of consitency.

Closes #3182
---
Issue: https://github.com/tarantool/tarantool/issues/3182
Branch: https://github.com/tarantool/tarantool/commits/kyukhin/gh-3182-repair-cursors

 src/box/sql/analyze.c   |  4 +--
 src/box/sql/build.c     |  6 ++--
 src/box/sql/delete.c    |  2 +-
 src/box/sql/insert.c    | 25 +++++---------
 src/box/sql/sqliteInt.h |  6 ++--
 src/box/sql/trigger.c   |  2 +-
 src/box/sql/update.c    |  5 ++-
 src/box/sql/vdbe.c      | 91 ++++++++++++++++++-------------------------------
 src/box/sql/where.c     | 13 +++----
 src/box/sql/whereInt.h  |  2 +-
 10 files changed, 60 insertions(+), 96 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 00d96d2..a34b189 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -817,7 +817,7 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
 	iTabCur = iTab++;
 	iIdxCur = iTab++;
 	pParse->nTab = MAX(pParse->nTab, iTab);
-	sqlite3OpenTable(pParse, iTabCur, pTab, OP_OpenRead);
+	sqlite3OpenTable(pParse, iTabCur, pTab);
 	sqlite3VdbeLoadString(v, regTabname, pTab->def->name);
 
 	for (pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext) {
@@ -885,7 +885,7 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
 		struct space *space = space_by_id(pIdx->def->space_id);
 		int idx_id = pIdx->def->iid;
 		assert(space != NULL);
-		sqlite3VdbeAddOp4(v, OP_OpenRead, iIdxCur, idx_id, 0,
+		sqlite3VdbeAddOp4(v, OP_CursorOpen, iIdxCur, idx_id, 0,
 				  (void *) space, P4_SPACEPTR);
 		VdbeComment((v, "%s", pIdx->def->name));
 
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index da7668b..8130c85 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1080,7 +1080,7 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
 		      struct space *space)
 {
 	assert(space != NULL);
-	return sqlite3VdbeAddOp4(parse_context->pVdbe, OP_OpenWrite, cursor,
+	return sqlite3VdbeAddOp4(parse_context->pVdbe, OP_CursorOpen, cursor,
 				 index_id, 0, (void *) space, P4_SPACEPTR);
 }
 
@@ -2570,7 +2570,7 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex)
 	/* Open the table. Loop through all rows of the table, inserting index
 	 * records into the sorter.
 	 */
-	sqlite3OpenTable(pParse, iTab, pTab, OP_OpenRead);
+	sqlite3OpenTable(pParse, iTab, pTab);
 	addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iTab, 0);
 	VdbeCoverage(v);
 	regRecord = sqlite3GetTempReg(pParse);
@@ -3141,7 +3141,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
 			goto exit_create_index;
 
 		sql_set_multi_write(parse, true);
-		sqlite3VdbeAddOp4(vdbe, OP_OpenWrite, cursor, 0, 0,
+		sqlite3VdbeAddOp4(vdbe, OP_CursorOpen, cursor, 0, 0,
 				  (void *)space_by_id(BOX_INDEX_ID),
 				  P4_SPACEPTR);
 		sqlite3VdbeChangeP5(vdbe, OPFLAG_SEEKEQ);
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 0be6883..a330661 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -383,7 +383,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 				iAddrOnce = sqlite3VdbeAddOp0(v, OP_Once);
 				VdbeCoverage(v);
 			}
-			sqlite3VdbeAddOp4(v, OP_OpenWrite, tab_cursor, 0, 0,
+			sqlite3VdbeAddOp4(v, OP_CursorOpen, tab_cursor, 0, 0,
 					  (void *) space, P4_SPACEPTR);
 			VdbeComment((v, "%s", space->index[0]->def->name));
 
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 0e92f62..997115a 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -45,12 +45,10 @@
 void
 sqlite3OpenTable(Parse * pParse,	/* Generate code into this VDBE */
 		 int iCur,	/* The cursor number of the table */
-		 Table * pTab,	/* The table to be opened */
-		 int opcode)	/* OP_OpenRead or OP_OpenWrite */
+		 Table * pTab)	/* The table to be opened */
 {
 	Vdbe *v;
 	v = sqlite3GetVdbe(pParse);
-	assert(opcode == OP_OpenWrite || opcode == OP_OpenRead);
 
 	struct space *space = space_by_id(pTab->def->id);
 	assert(space->index_count > 0);
@@ -199,11 +197,7 @@ vdbe_has_table_read(struct Parse *parser, const struct Table *table)
 	for (int i = 1; i < last_instr; i++) {
 		struct VdbeOp *op = sqlite3VdbeGetOp(v, i);
 		assert(op != NULL);
-		/*
-		 * Currently, there is no difference between Read
-		 * and Write cursors.
-		 */
-		if (op->opcode == OP_OpenRead || op->opcode == OP_OpenWrite) {
+		if (op->opcode == OP_CursorOpen) {
 			assert(op->p4type == P4_SPACEPTR);
 			struct space *space = op->p4.space;
 			if (space->def->id == table->def->id)
@@ -613,9 +607,9 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 	if (!is_view) {
 		int nIdx;
 		nIdx =
-		    sqlite3OpenTableAndIndices(pParse, pTab, OP_OpenWrite, 0,
-					       -1, 0, &iDataCur, &iIdxCur,
-					       on_error, 0);
+		    sqlite3OpenTableAndIndices(pParse, pTab, 0, -1, 0,
+					       &iDataCur, &iIdxCur, on_error,
+					       0);
 
 		aRegIdx = sqlite3DbMallocRawNN(db, sizeof(int) * (nIdx + 1));
 		if (aRegIdx == 0) {
@@ -1501,7 +1495,6 @@ vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id,
 int
 sqlite3OpenTableAndIndices(Parse * pParse,	/* Parsing context */
 			   Table * pTab,	/* Table to be opened */
-			   int op,		/* OP_OpenRead or OP_OpenWrite */
 			   u8 p5,		/* P5 value for OP_Open* opcodes */
 			   int iBase,		/* Use this for the table cursor, if there is one */
 			   u8 * aToOpen,	/* If not NULL: boolean for each table and index */
@@ -1515,8 +1508,6 @@ sqlite3OpenTableAndIndices(Parse * pParse,	/* Parsing context */
 	Index *pIdx;
 	Vdbe *v;
 
-	assert(op == OP_OpenRead || op == OP_OpenWrite);
-	assert(op == OP_OpenWrite || p5 == 0);
 	v = sqlite3GetVdbe(pParse);
 	assert(v != 0);
 	if (iBase < 0)
@@ -1573,9 +1564,9 @@ sqlite3OpenTableAndIndices(Parse * pParse,	/* Parsing context */
 				p5 = 0;
 			}
 			if (aToOpen == 0 || aToOpen[i + 1]) {
-				sqlite3VdbeAddOp4(v, op, iIdxCur, pIdx->def->iid,
-						  0, (void *) space,
-						  P4_SPACEPTR);
+				sqlite3VdbeAddOp4(v, OP_CursorOpen, iIdxCur,
+						  pIdx->def->iid, 0,
+						  (void *) space, P4_SPACEPTR);
 				sqlite3VdbeChangeP5(v, p5);
 				VdbeComment((v, "%s", pIdx->def->name));
 			}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index abb6829..3884610 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3476,7 +3476,7 @@ void sqlite3EndTable(Parse *, Token *, Token *, Select *);
 /**
  * Create cursor which will be positioned to the space/index.
  * It makes space lookup and loads pointer to it into register,
- * which is passes to OP_OpenWrite as an argument.
+ * which is passed to OP_CursorOpen as an argument.
  *
  * @param parse_context Parse context.
  * @param cursor Number of cursor to be created.
@@ -3627,7 +3627,7 @@ Select *sqlite3SelectNew(Parse *, ExprList *, SrcList *, Expr *, ExprList *,
 struct Table *
 sql_list_lookup_table(struct Parse *parse, struct SrcList *src_list);
 
-void sqlite3OpenTable(Parse *, int iCur, Table *, int);
+void sqlite3OpenTable(Parse *, int iCur, Table *);
 /**
  * Generate code for a DELETE FROM statement.
  *
@@ -3922,7 +3922,7 @@ void
 vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id,
 			       struct on_conflict *on_conflict);
 
-int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *,
+int sqlite3OpenTableAndIndices(Parse *, Table *, u8, int, u8 *, int *,
 			       int *, u8, u8);
 void
 sql_set_multi_write(Parse *, bool);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index bd730c4..de5878c 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -215,7 +215,7 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
 			goto triggerfinish_cleanup;
 
 		int cursor = parse->nTab++;
-		sqlite3OpenTable(parse, cursor, sys_trigger, OP_OpenWrite);
+		sqlite3OpenTable(parse, cursor, sys_trigger);
 
 		/*
 		 * makerecord(cursor(iRecord),
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 9e9fa3a..8014d6e 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -391,9 +391,8 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 			if (aiCurOnePass[1] >= 0)
 				aToOpen[aiCurOnePass[1] - iBaseCur] = 0;
 		}
-		sqlite3OpenTableAndIndices(pParse, pTab, OP_OpenWrite, 0,
-					   iBaseCur, aToOpen, 0, 0,
-					   on_error, 1);
+		sqlite3OpenTableAndIndices(pParse, pTab, 0, iBaseCur, aToOpen,
+					   0, 0, on_error, 1);
 	}
 
 	/* Top of the update loop */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index dc3f5c0..4b20a27 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3057,51 +3057,35 @@ case OP_TTransaction: {
 	break;
 }
 
-/* Opcode: OpenRead P1 P2 P3 P4 P5
+/* Opcode: CursorReopen P1 P2 P3 P4 P5
  * Synopsis: index id = P2, space ptr = P4
  *
- * Open a cursor for a space specified by pointer in P4 and index
- * id in P2. Give the new cursor an identifier of P1. The P1
- * values need not be contiguous but all P1 values should be
- * small integers. It is an error for P1 to be negative.
- */
-/* Opcode: ReopenIdx P1 P2 P3 P4 P5
- * Synopsis: index id = P2, space ptr = P4
- *
- * The ReopenIdx opcode works exactly like OpenRead except that
- * it first checks to see if the cursor on P1 is already open
+ * The CursorReopen opcode works exactly like CursorOpen except
+ * that it first checks to see if the cursor on P1 is already open
  * with the same index and if it is this opcode becomes a no-op.
- * In other words, if the cursor is already open, do not reopen it.
+ * In other words, if the cursor is already open, do not reopen
+ * it.
  *
- * The ReopenIdx opcode may only be used with P5 == 0.
+ * The CursorReopen opcode may only be used with P5 == 0.
  */
-/* Opcode: OpenWrite P1 P2 P3 P4 P5
+/* Opcode: CursorOpen P1 P2 * P4 P5
  * Synopsis: index id = P2, space ptr = P4
  *
- * For now, OpenWrite is an alias for OpenRead.
- * It exists just due legacy reasons and should be removed:
- * it isn't neccessary to open cursor to make insertion or
- * deletion.
+ * Open a cursor for a space specified by pointer in P4 and index
+ * id in P2. Give the new cursor an identifier of P1. The P1
+ * values need not be contiguous but all P1 values should be
+ * small integers. It is an error for P1 to be negative.
  */
-case OP_ReopenIdx: {
-	int nField;
-	int p2;
-	VdbeCursor *pCur;
-	BtCursor *pBtCur;
-
-	assert(pOp->p5==0 || pOp->p5==OPFLAG_SEEKEQ);
-	pCur = p->apCsr[pOp->p1];
-	p2 = pOp->p2;
-	if (pCur && pCur->uc.pCursor->space == pOp->p4.space &&
-	    pCur->uc.pCursor->index->def->iid == (uint32_t)p2)
+case OP_CursorReopen: {
+	assert(pOp->p5 == 0);
+	struct VdbeCursor *cur = p->apCsr[pOp->p1];
+	if (cur != NULL && cur->uc.pCursor->space == pOp->p4.space &&
+	    cur->uc.pCursor->index->def->iid == (uint32_t)pOp->p2)
 		goto open_cursor_set_hints;
 	/* If the cursor is not currently open or is open on a different
-	 * index, then fall through into OP_OpenRead to force a reopen
+	 * index, then fall through into OP_OpenCursor to force a reopen
 	 */
-case OP_OpenRead:
-case OP_OpenWrite:
-
-	assert(pOp->opcode==OP_OpenWrite || pOp->p5==0 || pOp->p5==OPFLAG_SEEKEQ);
+case OP_CursorOpen:
 	if (box_schema_version() != p->schema_ver &&
 	    (pOp->p5 & OPFLAG_SYSTEMSP) == 0) {
 		p->expired = 1;
@@ -3110,34 +3094,27 @@ case OP_OpenWrite:
 				    "need to re-compile SQL statement");
 		goto abort_due_to_error;
 	}
-	p2 = pOp->p2;
 	struct space *space = pOp->p4.space;
 	assert(space != NULL);
-	struct index *index = space_index(space, p2);
+	struct index *index = space_index(space, pOp->p2);
 	assert(index != NULL);
-	/*
-	 * Since Tarantool iterator provides the full tuple,
-	 * we need a number of fields as wide as the table itself.
-	 * Otherwise, not enough slots for row parser cache are
-	 * allocated in VdbeCursor object.
-	 */
-	nField = space->def->field_count;
-	assert(pOp->p1>=0);
-	assert(nField>=0);
-	pCur = allocateCursor(p, pOp->p1, nField, CURTYPE_TARANTOOL);
-	if (pCur==0) goto no_mem;
-	pCur->nullRow = 1;
-	pBtCur = pCur->uc.pCursor;
-	pBtCur->curFlags |= BTCF_TaCursor;
-	pBtCur->space = space;
-	pBtCur->index = index;
-	pBtCur->eState = CURSOR_INVALID;
+	assert(pOp->p1 >= 0);
+	cur = allocateCursor(p, pOp->p1, space->def->field_count,
+			     CURTYPE_TARANTOOL);
+	if (cur == NULL)
+		goto no_mem;
+	struct BtCursor *bt_cur = cur->uc.pCursor;
+	bt_cur->curFlags |= BTCF_TaCursor;
+	bt_cur->space = space;
+	bt_cur->index = index;
+	bt_cur->eState = CURSOR_INVALID;
 	/* Key info still contains sorter order and collation. */
-	pCur->key_def = index->def->key_def;
-
+	cur->key_def = index->def->key_def;
+	cur->nullRow = 1;
 open_cursor_set_hints:
-	pCur->uc.pCursor->hints = pOp->p5 & OPFLAG_SEEKEQ;
-	if (rc) goto abort_due_to_error;
+	cur->uc.pCursor->hints = pOp->p5 & OPFLAG_SEEKEQ;
+	if (rc != 0)
+		goto abort_due_to_error;
 	break;
 }
 
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 1173576..d54e3dc 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -4678,12 +4678,9 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 			/* Do nothing */
 		} else if ((pLoop->wsFlags & WHERE_IDX_ONLY) == 0 &&
 			   (wctrlFlags & WHERE_OR_SUBCLAUSE) == 0) {
-			int op = OP_OpenRead;
-			if (pWInfo->eOnePass != ONEPASS_OFF) {
-				op = OP_OpenWrite;
+			if (pWInfo->eOnePass != ONEPASS_OFF)
 				pWInfo->aiCurOnePass[0] = pTabItem->iCursor;
-			};
-			sqlite3OpenTable(pParse, pTabItem->iCursor, pTab, op);
+			sqlite3OpenTable(pParse, pTabItem->iCursor, pTab);
 			assert(pTabItem->iCursor == pLevel->iTabCur);
 			testcase(pWInfo->eOnePass == ONEPASS_OFF
 				 && pTab->nCol == BMS - 1);
@@ -4702,7 +4699,7 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 			struct index_def *idx_def = pLoop->index_def;
 			struct space *space = space_cache_find(pTabItem->pTab->def->id);
 			int iIndexCur;
-			int op = OP_OpenRead;
+			int op = OP_CursorOpen;
 			/* iAuxArg is always set if to a positive value if ONEPASS is possible */
 			assert(iAuxArg != 0
 			       || (pWInfo->
@@ -4756,12 +4753,12 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 					}
 				}
 				assert(wctrlFlags & WHERE_ONEPASS_DESIRED);
-				op = OP_OpenWrite;
+				op = OP_CursorOpen;
 				pWInfo->aiCurOnePass[1] = iIndexCur;
 			} else if (iAuxArg
 				   && (wctrlFlags & WHERE_OR_SUBCLAUSE) != 0) {
 				iIndexCur = iAuxArg;
-				op = OP_ReopenIdx;
+				op = OP_CursorReopen;
 			} else {
 				iIndexCur = pParse->nTab++;
 			}
diff --git a/src/box/sql/whereInt.h b/src/box/sql/whereInt.h
index 548cbcb..46c7ba4 100644
--- a/src/box/sql/whereInt.h
+++ b/src/box/sql/whereInt.h
@@ -417,7 +417,7 @@ struct WhereInfo {
 	ExprList *pOrderBy;	/* The ORDER BY clause or NULL */
 	ExprList *pDistinctSet;	/* DISTINCT over all these values */
 	LogEst iLimit;		/* LIMIT if wctrlFlags has WHERE_USE_LIMIT */
-	int aiCurOnePass[2];	/* OP_OpenWrite cursors for the ONEPASS opt */
+	int aiCurOnePass[2];	/* OP_CursorOpen cursors for the ONEPASS opt */
 	int iContinue;		/* Jump here to continue with next record */
 	int iBreak;		/* Jump here to break out of the loop */
 	int savedNQueryLoop;	/* pParse->nQueryLoop outside the WHERE loop */
-- 
2.16.2