[tarantool-patches] Re: [PATCH] sql: repair SQL cursor handling opcodes
n.pettik
korablev at tarantool.org
Mon Sep 3 15:28:45 MSK 2018
> 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;
More information about the Tarantool-patches
mailing list