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 1458F2B62B for ; Wed, 3 Oct 2018 22:21: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 7YzfMnjjQYEx for ; Wed, 3 Oct 2018 22:21:49 -0400 (EDT) Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 4D3F22B59D for ; Wed, 3 Oct 2018 22:21: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: refactor SQL cursor to remove write ones From: "n.pettik" In-Reply-To: <20181003095759.2cdk54garm4na3f3@tarantool.org> Date: Thu, 4 Oct 2018 05:21:43 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <4A0EC663-775A-4D4B-8458-A27ADB72B71C@tarantool.org> References: <4aec5b52bc28f47ad2af0ac6f4e64be0712f378b.1538389679.git.kyukhin@tarantool.org> <7816B3E4-321F-446F-B19A-ED3E241D5D86@tarantool.org> <20181003095759.2cdk54garm4na3f3@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 >> In general patch is OK, I have spotted only several nits. >> Also, I support your intention (which we discussed outside mailing = list) to make >> system spaces be =E2=80=98pre-loaded=E2=80=99. It means that all = required system spaces will be >> stored in predefined (at the start of program) registers. Moreover, I = suggest to >> attempt at involving not only system spaces. Idea is following: >> each OP_CursorOpen during compilation stage skips register where = space >> should be stored. At the end of compilation (sql_finish_coding) we = know that >> totally program uses n registers. Then, we can allocate n + m (where = m is number >> of distinct spaces which are used in out query) registers to hold = pointers to required >> spaces. Finally, we patch each OP_CursorOpen and substitute void arg = with >> number of register where required space is stored. This approach for = instance allows >> to update all space pointers after schema changes with ease. > Great! I'll do that in follow up patches. Ok, then register it as an (set of) issues(es) (in case you are going to implement them separately). >> Finally, now I think about renaming CursorOpen to IteratorOpen: in = fact after your >> patch cursor has turned almost in wrapper around index iterator. Such = naming >> would underline the fact that =E2=80=98cursor=E2=80=99 is used only = for read iterations over the space. > Yes, sure. Renamed. You forgot to update name of opcode in commit message. Also, you deleted not all mentions of OP_Cursor(Re)Open - just grep it >>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c >>> index 9aa7058..ebde698 100644 >>> --- a/src/box/sql/delete.c >>> +++ b/src/box/sql/delete.c >>> @@ -69,7 +69,7 @@ sql_lookup_table(struct Parse *parse, struct = SrcList_item *tbl_name) >>>=20 >>> void >>> sql_materialize_view(struct Parse *parse, const char *name, struct = Expr *where, >>> - int cursor) >>> + int cursor, int reg_eph) >>=20 >> This function is used twice: to materialize view during firing = INSTEAD OF DELETE >> and INSTEAD OF UPDATE triggers. In both cases, you pass reg_eph which = is >> not used after this function invocation. So I guess you can remove = reg_eph arg. > IMHO, allocating a register w/o assigning it to local variable is less > straightforward, but OK, done. It still can be simplified - see comments below. >>> @@ -5261,7 +5303,7 @@ resetAccumulator(Parse * pParse, AggInfo * = pAggInfo) >>> /* Verify that all AggInfo registers are within the range = specified by >>> * AggInfo.mnReg..AggInfo.mxReg >>> */ >>> - assert(nReg =3D=3D pAggInfo->mxReg - pAggInfo->mnReg + 1); >>> + assert(nReg <=3D pAggInfo->mxReg - pAggInfo->mnReg + 1); >>=20 >> Hm? Explain pls this change. > I'd avoid explaining it in the code. So, will do it here. > Issue is that aggregate function may or may not contain distinct = clause. > If so - one extra reg per such func will be used. Right hand size of = the > assert doesn't take that into account and I see no reason to do that. > That's why, new assertion is that number of allocated registers for > aggregate is greater or even than sum of number of aggregate columns = and > functions. >=20 >>> + cur =3D allocateCursor(p, pOp->p1, >>> + space->def->exact_field_count =3D=3D 0 ? >>> + space->def->field_count : >>> + space->def->exact_field_count, >>> + CURTYPE_TARANTOOL); >>> + if (cur =3D=3D NULL) >>> + goto no_mem; >>> + struct BtCursor *bt_cur =3D cur->uc.pCursor; >>> + bt_cur->curFlags |=3D space->def->id =3D=3D 0 ? = BTCF_TEphemCursor : >>> + BTCF_TaCursor; >>> + bt_cur->space =3D space; >>> + bt_cur->index =3D index; >>> + bt_cur->eState =3D CURSOR_INVALID; >>=20 >> Grep CURSOR_INVALID - it is not used anywhere else. >> Mb it is worth to remove it? > eState flag is heavily used by cursors machinery. I don't > think we can evict it right now. I meant CURSOR_INVALID is not used (you can grep its usages). So basically we can make eState to be boolean value: cursor is whether broken (invalid) or not. I don=E2=80=99t insist on = this change tho. > -- > Regards, Kirill Yukhin >=20 > commit 5bd0cf10ac94cfbaf5d076a3f5a347811fcf4c11 > Author: Kirill Yukhin > Date: Tue Sep 25 19:01:10 2018 +0300 >=20 > sql: refactor SQL cursor to remove write ones >=20 > In Tarantool opening and positioning cursor for writing > have no sense. So refactor SQL code, to perform: > - Creation of ephemeral table w/o any cursors machinery. > No op-code returns register which contains plain pointer > to the ephemeral space. > - OpenRead/OpenWrite opcodes were replaced with single > CursorOpen op-code, which establishes new cursor w/ IteratorOpen > intention to subsequent read from the space. This opcode > accepts both plain pointer (in P4 operand) and register > which contains pointer (inn P3) to the ephemeral space. Typo: in P3 > diff --git a/src/box/sql.c b/src/box/sql.c > index ab4a587..f3836f2 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -355,31 +355,15 @@ int tarantoolSqlite3Count(BtCursor *pCur, i64 = *pnEntry) > return SQLITE_OK; > } >=20 > -/** > - * Create ephemeral space and set cursor to the first entry. Features = of > - * ephemeral spaces: id =3D=3D 0, name =3D=3D "ephemeral", memtx = engine (in future it > - * can be changed, but now only memtx engine is supported), primary = index > - * which covers all fields and no secondary indexes, given field = number and > - * collation sequence. All fields are scalar and nullable. > - * > - * @param pCur Cursor which will point to the new ephemeral space. > - * @param field_count Number of fields in ephemeral space. > - * @param key_info Keys description for new ephemeral space. > - * > - * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise. > - */ > -int > -tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count, > - struct sql_key_info *key_info) > +struct space * > +sql_ephemeral_space_create(uint32_t field_count, > + struct sql_key_info *key_info) Nit: you can fit now args in one line. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 43be777..7670d53 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -883,7 +883,7 @@ vdbe_emit_open_cursor(struct Parse *parse_context, = int cursor, int index_id, > struct space *space) > { > assert(space !=3D NULL); > - return sqlite3VdbeAddOp4(parse_context->pVdbe, OP_OpenWrite, = cursor, > + return sqlite3VdbeAddOp4(parse_context->pVdbe, OP_IteratorOpen, = cursor, > index_id, 0, (void *) space, = P4_SPACEPTR); > } >=20 > @@ -2693,7 +2693,7 @@ sql_create_index(struct Parse *parse, struct = Token *token, > goto exit_create_index; >=20 > sql_set_multi_write(parse, true); > - sqlite3VdbeAddOp4(vdbe, OP_OpenWrite, cursor, 0, 0, > + sqlite3VdbeAddOp4(vdbe, OP_IteratorOpen, 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 9aa7058..17e1c32 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -69,7 +69,7 @@ sql_lookup_table(struct Parse *parse, struct = SrcList_item *tbl_name) >=20 > void > sql_materialize_view(struct Parse *parse, const char *name, struct = Expr *where, > - int cursor) > + int cursor, int reg_eph) You slightly misunderstood me: you don=E2=80=99t need to pass arg = reg_eph: you already have struct Parse *parse, so can fetch value for register inside function. > @@ -4061,7 +4064,8 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * = pExpr, int target) > int n; > if (pExpr->pLeft->iTable =3D=3D 0) { > pExpr->pLeft->iTable =3D > - sqlite3CodeSubselect(pParse, = pExpr->pLeft, 0); > + sqlite3CodeSubselect(pParse, = pExpr->pLeft, > + 0); Noise diff. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 02c0a5d..2776044 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -64,8 +64,15 @@ typedef struct DistinctCtx DistinctCtx; > struct DistinctCtx { > u8 isTnct; /* True if the DISTINCT keyword is = present */ > u8 eTnctType; /* One of the WHERE_DISTINCT_* operators = */ > - int tabTnct; /* Ephemeral table used for DISTINCT = processing */ > - int addrTnct; /* Address of OP_OpenEphemeral opcode = for tabTnct */ > + /* Ephemeral table's cursor used for DISTINCT processing. It is > + * used for readin from ephemeral space. > + */ > + int cur_eph; > + /* Register, containing a pointer to ephemeral space. It > + * is used for insertions while procesing DISTINCT. > + */ > + int reg_eph; > + int addrTnct; /* Address of OP_OpenEphemeral opcode = for cur_eph */ > }; I fixed comment a little bit: @@ -64,12 +64,14 @@ typedef struct DistinctCtx DistinctCtx; struct DistinctCtx { u8 isTnct; /* True if the DISTINCT keyword is = present */ u8 eTnctType; /* One of the WHERE_DISTINCT_* operators = */ - /* Ephemeral table's cursor used for DISTINCT processing. It is - * used for readin from ephemeral space. + /** + * Ephemeral table's cursor used for DISTINCT processing. + * It is used for reading from ephemeral space. */ int cur_eph; - /* Register, containing a pointer to ephemeral space. It - * is used for insertions while procesing DISTINCT. + /** + * Register, containing a pointer to ephemeral space. + * It is used for insertions while processing DISTINCT. */ > @@ -1039,17 +1059,25 @@ selectInnerLoop(Parse * pParse, = /* The parser context */ > } >=20 > case WHERE_DISTINCT_UNIQUE:{ > - sqlite3VdbeChangeToNoop(v, = pDistinct->addrTnct); > - break; > + /* To handle DISTINCT two op-codes are > + * emitted: OpenTEphemeral & IteratorOpen. > + * addrTnct is address off first insn in > + * a couple. Two evict ephemral space, > + * need to noop both op-codes. > + */ I=E2=80=99ve fixed comment: - /* To handle DISTINCT two op-codes are + /* + * To handle DISTINCT two op-codes are * emitted: OpenTEphemeral & IteratorOpen. - * addrTnct is address off first insn in - * a couple. Two evict ephemral space, - * need to noop both op-codes. + * addrTnct is address of first opcode in + * a couple. To evict ephemeral space, + * we need to noop both op-codes. */ > + sqlite3VdbeChangeToNoop(v, pDistinct->addrTnct); > + sqlite3VdbeChangeToNoop(v, pDistinct->addrTnct + = 1); > + break; > } >=20 >=20 > @@ -5679,7 +5725,7 @@ sqlite3Select(Parse * pParse, /* The = parser context */ > VdbeComment((v, "%s", pItem->pTab->def->name)); > pItem->addrFillSub =3D addrTop; > sqlite3SelectDestInit(&dest, SRT_Coroutine, > - pItem->regReturn); > + pItem->regReturn, -1); > pItem->iSelectId =3D pParse->iNextSelectId; > sqlite3Select(pParse, pSub, &dest); > pItem->pTab->tuple_log_count =3D = pSub->nSelectRow; > @@ -5699,6 +5745,7 @@ sqlite3Select(Parse * pParse, /* The = parser context */ > int retAddr; > assert(pItem->addrFillSub =3D=3D 0); > pItem->regReturn =3D ++pParse->nMem; > + Noise diff. > @@ -5885,7 +5941,15 @@ sqlite3Select(Parse * pParse, /* The = parser context */ > * into an OP_Noop. > */ > if (sSort.addrSortIndex >=3D 0 && sSort.pOrderBy =3D=3D = 0) { > + /* To handle ordering two op-codes are > + * emitted: OpenTEphemeral & IteratorOpen. > + * sSort.addrSortIndex is address off > + * first insn in a couple. Two evict > + * ephemral space, need to noop both > + * op-codes. > + */ I=E2=80=99ve fixed comment: - /* To handle ordering two op-codes are + /* + * To handle ordering two op-codes are * emitted: OpenTEphemeral & IteratorOpen. - * sSort.addrSortIndex is address off - * first insn in a couple. Two evict - * ephemral space, need to noop both + * sSort.addrSortIndex is address of + * first opcode in a couple. To evict + * ephemeral space, we need to noop both * op-codes. */ > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 53188e7..29636a0 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2000,6 +2000,8 @@ struct AggInfo { > FuncDef *pFunc; /* The aggregate function implementation = */ > int iMem; /* Memory location that acts as = accumulator */ > int iDistinct; /* Ephemeral table used to enforce = DISTINCT */ > + /* Register, holding ephemeral's spacepointer. */ Nit: missed space between =E2=80=99space=E2=80=99 and =E2=80=98pointer=E2=80= =99. And start comment from /**. > + int reg_eph; > } *aFunc; > int nFunc; /* Number of entries in aFunc[] */ > }; > @@ -2591,6 +2593,8 @@ struct SelectDest { > u8 eDest; /* How to dispose of the results. On of = SRT_* above. */ > char *zAffSdst; /* Affinity used when eDest=3D=3DSRT_Set = */ > int iSDParm; /* A parameter used by the eDest = disposal method */ > + /* Register containing ephemeral's space pointer. */ Nit: start comment from /**. > diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h > index 8017742..497d989 100644 > --- a/src/box/sql/tarantoolInt.h > +++ b/src/box/sql/tarantoolInt.h > @@ -97,9 +97,22 @@ sql_index_update_table_name(struct index_def *idef, = const char *new_tbl_name, > int tarantoolSqlite3RenameTrigger(const char *zTriggerName, > const char *zOldName, const char = *zNewName); >=20 > -/* Interface for ephemeral tables. */ > -int tarantoolSqlite3EphemeralCreate(BtCursor * pCur, uint32_t = filed_count, > - struct sql_key_info *key_info); > +/** > + * Create ephemeral space. Features of ephemeral spaces: id =3D=3D 0, > + * name =3D=3D "ephemeral", memtx engine (in future it can be = changed, > + * but now only memtx engine is supported), primary index which > + * covers all fields and no secondary indexes, given field number > + * and collation sequence. All fields are scalar and nullable. > + * > + * @param field_count Number of fields in ephemeral space. > + * @param key_info Keys description for new ephemeral space. > + * > + * @retval Pointer to created space, NULL if error. > + */ > +struct space * > +sql_ephemeral_space_create(uint32_t filed_count, > + struct sql_key_info *key_info); Nit: you can fit it in one line. > @@ -276,9 +280,7 @@ sqlite3Update(Parse * pParse, /* The = parser context */ > = pPk->def); > sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, pk_part_count, > regKey, zAff, pk_part_count); > - sqlite3VdbeAddOp2(v, OP_IdxInsert, iEph, regKey); > - /* Set flag to save memory allocating one by malloc. */ > - sqlite3VdbeChangeP5(v, 1); You misunderstood me: you should change P5 to 1 for OP_MakeRecord = opcode. To be more precise see patch: = https://github.com/tarantool/tarantool/commit/a5576aa77bf900d0ceae126ba46a= 80c56078df6c > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 00ffb0c..aa602df 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3054,51 +3054,37 @@ case OP_TTransaction: { > break; > } >=20 > -/* Opcode: OpenRead P1 P2 P3 P4 P5 > +/* Opcode: CursorReopen P1 P2 P3 P4 P5 IteratorReopen? > * Synopsis: index id =3D P2, space ptr =3D 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 =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 > - * Synopsis: index id =3D P2, space ptr =3D P4 > +/* Opcode: CursorOpen P1 P2 P3 P4 P5 Don=E2=80=99t forget to rename opcode even in comments. > diff --git a/src/box/sql/whereInt.h b/src/box/sql/whereInt.h > index 8a3f2ac..1aeb360 100644 > --- a/src/box/sql/whereInt.h > +++ b/src/box/sql/whereInt.h > @@ -416,7 +416,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 */ IteratorOpen?