From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 323252907B for ; Mon, 3 Sep 2018 08:28:49 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id UBpiGJw8zETU for ; Mon, 3 Sep 2018 08:28:49 -0400 (EDT) Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 5362D28CE9 for ; Mon, 3 Sep 2018 08:28:48 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: repair SQL cursor handling opcodes From: "n.pettik" In-Reply-To: <78b3be169822b4ca81b15a295d04554165811daa.1535614941.git.kyukhin@tarantool.org> Date: Mon, 3 Sep 2018 15:28:45 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <0BAA6FC4-8CC7-42CF-93C1-8F2FBBEE82FF@tarantool.org> References: <78b3be169822b4ca81b15a295d04554165811daa.1535614941.git.kyukhin@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Kirill Yukhin > =46rom 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: =E2=80=9Crenames=E2=80=9D. > ReopenIndex to CursorReopen for > the sake of consitency. >=20 > 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; > } >=20 > -/* Opcode: OpenRead P1 P2 P3 P4 P5 > +/* Opcode: CursorReopen P1 P2 P3 P4 P5 > * Synopsis: index id =3D P2, space ptr =3D 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 =3D P2, space ptr =3D 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 =3D=3D 0. > + * The CursorReopen opcode may only be used with P5 =3D=3D 0. > */ > -/* Opcode: OpenWrite P1 P2 P3 P4 P5 > +/* Opcode: CursorOpen P1 P2 * P4 P5 > * Synopsis: index id =3D P2, space ptr =3D 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=3D=3D0 || pOp->p5=3D=3DOPFLAG_SEEKEQ); > - pCur =3D p->apCsr[pOp->p1]; > - p2 =3D pOp->p2; > - if (pCur && pCur->uc.pCursor->space =3D=3D pOp->p4.space && > - pCur->uc.pCursor->index->def->iid =3D=3D (uint32_t)p2) > +case OP_CursorReopen: { > + assert(pOp->p5 =3D=3D 0); > + struct VdbeCursor *cur =3D p->apCsr[pOp->p1]; > + if (cur !=3D NULL && cur->uc.pCursor->space =3D=3D pOp->p4.space = && > + cur->uc.pCursor->index->def->iid =3D=3D (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=3D=3DOP_OpenWrite || pOp->p5=3D=3D0 || = pOp->p5=3D=3DOPFLAG_SEEKEQ); > +case OP_CursorOpen: > if (box_schema_version() !=3D p->schema_ver && > (pOp->p5 & OPFLAG_SYSTEMSP) =3D=3D 0) { > p->expired =3D 1; > @@ -3110,34 +3094,27 @@ case OP_OpenWrite: > "need to re-compile SQL statement"); > goto abort_due_to_error; > } > - p2 =3D pOp->p2; > struct space *space =3D pOp->p4.space; > assert(space !=3D NULL); > - struct index *index =3D space_index(space, p2); > + struct index *index =3D space_index(space, pOp->p2); > assert(index !=3D 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 =3D=3D = space fields. > - nField =3D space->def->field_count; > - assert(pOp->p1>=3D0); > - assert(nField>=3D0); > - pCur =3D allocateCursor(p, pOp->p1, nField, CURTYPE_TARANTOOL); > - if (pCur=3D=3D0) goto no_mem; > - pCur->nullRow =3D 1; > - pBtCur =3D pCur->uc.pCursor; > - pBtCur->curFlags |=3D BTCF_TaCursor; > - pBtCur->space =3D space; > - pBtCur->index =3D index; > - pBtCur->eState =3D CURSOR_INVALID; > + assert(pOp->p1 >=3D 0); > + cur =3D allocateCursor(p, pOp->p1, space->def->field_count, > + CURTYPE_TARANTOOL); > + if (cur =3D=3D NULL) > + goto no_mem; > + struct BtCursor *bt_cur =3D cur->uc.pCursor; > + bt_cur->curFlags |=3D BTCF_TaCursor; > + bt_cur->space =3D space; > + bt_cur->index =3D index; > + bt_cur->eState =3D CURSOR_INVALID; > /* Key info still contains sorter order and collation. */ > - pCur->key_def =3D index->def->key_def; > - > + cur->key_def =3D index->def->key_def; > + cur->nullRow =3D 1; > open_cursor_set_hints: > - pCur->uc.pCursor->hints =3D pOp->p5 & OPFLAG_SEEKEQ; > - if (rc) goto abort_due_to_error; > + cur->uc.pCursor->hints =3D pOp->p5 & OPFLAG_SEEKEQ; > + if (rc !=3D 0) > + goto abort_due_to_error; > break; > } >=20 > 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) =3D=3D 0 && > (wctrlFlags & WHERE_OR_SUBCLAUSE) =3D=3D 0) { > - int op =3D OP_OpenRead; > - if (pWInfo->eOnePass !=3D ONEPASS_OFF) { > - op =3D OP_OpenWrite; > + if (pWInfo->eOnePass !=3D ONEPASS_OFF) > pWInfo->aiCurOnePass[0] =3D = pTabItem->iCursor; > - }; > - sqlite3OpenTable(pParse, pTabItem->iCursor, = pTab, op); > + sqlite3OpenTable(pParse, pTabItem->iCursor, = pTab); > assert(pTabItem->iCursor =3D=3D = pLevel->iTabCur); > testcase(pWInfo->eOnePass =3D=3D ONEPASS_OFF > && pTab->nCol =3D=3D BMS - 1); > @@ -4702,7 +4699,7 @@ sqlite3WhereBegin(Parse * pParse, /* The = parser context */ > struct index_def *idx_def =3D pLoop->index_def; > struct space *space =3D = space_cache_find(pTabItem->pTab->def->id); > int iIndexCur; > - int op =3D OP_OpenRead; > + int op =3D OP_CursorOpen; > /* iAuxArg is always set if to a positive value = if ONEPASS is possible */ > assert(iAuxArg !=3D 0 > || (pWInfo-> > @@ -4756,12 +4753,12 @@ sqlite3WhereBegin(Parse * pParse, /* The = parser context */ > } > } > assert(wctrlFlags & = WHERE_ONEPASS_DESIRED); > - op =3D OP_OpenWrite; > + op =3D OP_CursorOpen; > pWInfo->aiCurOnePass[1] =3D iIndexCur; > } else if (iAuxArg > && (wctrlFlags & WHERE_OR_SUBCLAUSE) = !=3D 0) { > iIndexCur =3D iAuxArg; > - op =3D OP_ReopenIdx; > + op =3D OP_CursorReopen; > } else { > iIndexCur =3D 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 =3D pLoop->index_def; struct space *space =3D = space_cache_find(pTabItem->pTab->def->id); int iIndexCur; - int op =3D OP_CursorOpen; + bool is_cursor_open_required =3D true; /* iAuxArg is always set if to a positive value = if ONEPASS is possible */ assert(iAuxArg !=3D 0 || (pWInfo-> @@ -4726,7 +4726,7 @@ sqlite3WhereBegin(Parse * pParse, /* The parser = context */ * the PRIMARY KEY. No need for a = separate index */ iIndexCur =3D pLevel->iTabCur; - op =3D 0; + is_cursor_open_required =3D false; } else if (pWInfo->eOnePass !=3D ONEPASS_OFF) { if (pIx !=3D NULL) { Index *pJ =3D = pTabItem->pTab->pIndex; @@ -4753,18 +4753,16 @@ sqlite3WhereBegin(Parse * pParse, /* The = parser context */ } } assert(wctrlFlags & = WHERE_ONEPASS_DESIRED); - op =3D OP_CursorOpen; pWInfo->aiCurOnePass[1] =3D iIndexCur; } else if (iAuxArg && (wctrlFlags & WHERE_OR_SUBCLAUSE) = !=3D 0) { iIndexCur =3D iAuxArg; - op =3D OP_CursorReopen; } else { iIndexCur =3D pParse->nTab++; } pLevel->iIdxCur =3D iIndexCur; assert(iIndexCur >=3D 0); - if (op) { + if (is_cursor_open_required) { if (pIx !=3D NULL) { uint32_t space_id =3D pIx->pTable->def->id;