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

["n.pettik" <korablev@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

Nit: “renames”.

> ReopenIndex to CursorReopen for
> the sake of consitency.
> 
> Closes #3182

Are you sure that it is relevant ticket? I mean this patch only renames
opcodes, but I guess ticket is about eliminating write cursors.

> 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

P3 in fact is unused.

>  *
> - * 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.

It is assertion fault, actually.
Also, please describe P5 argument.

>  */
> -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
> 	 */

Nit: Make this comment obey Tarantool codestyle and fit into 66 chars:

/*
 * ...
 */

> -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.
> -	 */

Why did you delete this comment? It was useful IMHO: it explained
why cursor which is opened on index contains number of fields == space fields.

> -	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++;
> 			}

Consider next diff:

+++ b/src/box/sql/where.c
@@ -4699,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_CursorOpen;
+                       bool is_cursor_open_required = true;
                        /* iAuxArg is always set if to a positive value if ONEPASS is possible */
                        assert(iAuxArg != 0
                               || (pWInfo->
@@ -4726,7 +4726,7 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
                                 * the PRIMARY KEY.  No need for a separate index
                                 */
                                iIndexCur = pLevel->iTabCur;
-                               op = 0;
+                               is_cursor_open_required = false;
                        } else if (pWInfo->eOnePass != ONEPASS_OFF) {
                                if (pIx != NULL) {
                                        Index *pJ = pTabItem->pTab->pIndex;
@@ -4753,18 +4753,16 @@ sqlite3WhereBegin(Parse * pParse,       /* The parser context */
                                        }
                                }
                                assert(wctrlFlags & WHERE_ONEPASS_DESIRED);
-                               op = OP_CursorOpen;
                                pWInfo->aiCurOnePass[1] = iIndexCur;
                        } else if (iAuxArg
                                   && (wctrlFlags & WHERE_OR_SUBCLAUSE) != 0) {
                                iIndexCur = iAuxArg;
-                               op = OP_CursorReopen;
                        } else {
                                iIndexCur = pParse->nTab++;
                        }
                        pLevel->iIdxCur = iIndexCur;
                        assert(iIndexCur >= 0);
-                       if (op) {
+                       if (is_cursor_open_required) {
                                if (pIx != NULL) {
                                        uint32_t space_id =
                                                pIx->pTable->def->id;