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 3BA8A21083 for ; Tue, 10 Jul 2018 07:48:12 -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 eRpFr11zM_If for ; Tue, 10 Jul 2018 07:48:12 -0400 (EDT) Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 7608C21080 for ; Tue, 10 Jul 2018 07:48:11 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls References: <94a3289e26898b8f779450bf620d8fef454844c7.1531140049.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <96598d88-35f2-3075-6f90-f2efecd2f0cd@tarantool.org> Date: Tue, 10 Jul 2018 14:48:09 +0300 MIME-Version: 1.0 In-Reply-To: <94a3289e26898b8f779450bf620d8fef454844c7.1531140049.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Kirill Shcherbatov , tarantool-patches@freelists.org Hello. Thanks for the patch! On 09/07/2018 15:42, Kirill Shcherbatov wrote: > Made vdbe_emit_open_cursor calls consistent: > now it uses index id everywhere. > This required to change a way to detect that > VDBE has openned Read cursor to specified table > in vdbe_has_table_read to write result of insert > in temp table if required. > --- Where are the issue and the branch? > https://github.com/tarantool/tarantool/compare/kshch/kshch/vdbe_emit_open_cursor-refactor This is not branch and by this link I see message: "There isn’t anything to compare." > over branch > https://github.com/tarantool/tarantool/issues/2847 How is this linked? The issue was found under working with this issue, but it has been actual before and remains actual after. See 7 comments below. And I have pushed my own patch for LoadPtr removal on the top of your branch. Please, fix my comments below, and fix the patch I have pushed (most likely it does not pass tests now). > > src/box/sql/analyze.c | 3 +- > src/box/sql/build.c | 4 ++- > src/box/sql/expr.c | 5 ++- > src/box/sql/fkey.c | 3 +- > src/box/sql/insert.c | 95 ++++++++++++++++++++++++++++++--------------------- > src/box/sql/select.c | 4 +-- > src/box/sql/where.c | 10 ++++-- > 7 files changed, 75 insertions(+), 49 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 1c00842..82447dc 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1108,6 +1108,7 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id, > struct space *space) > { > assert(space != NULL); > + assert(index_id == SQLITE_PAGENO_TO_INDEXID(index_id)); 1. What the hell? Why? > struct Vdbe *vdbe = parse_context->pVdbe; > int space_ptr_reg = ++parse_context->nMem; > sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space, > addr1 = sqlite3VdbeAddOp2(v, OP_SorterSort, iSorter, 0); 2. vdbe_emit_open_cursor has outdated comment in the header. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 58e159c..77567e7 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -179,41 +179,50 @@ sqlite3TableAffinity(Vdbe * v, Table * pTab, int iReg) > } > } > > -/* > - * Return non-zero if the table pTab in database or any of its indices > - * have been opened at any point in the VDBE program. This is used to see if > - * a statement of the form "INSERT INTO SELECT ..." can > - * run for the results of the SELECT. > +/** > + * Return true if the table pTab in database or any of its 3. No pTab. > + * indices have been opened at any point in the VDBE program. > + * This is used to see if a statement of the form > + * "INSERT INTO SELECT ..." can run for the results of the > + * SELECT. > + * > + * @param parser Parse context. > + * @param table Table AST object. > + * @retval true or false 4. Unhelpful comment. Obviously bool is either true or false. > */ > -static int > -readsTable(Parse * p, Table * pTab) > +static bool > +vdbe_has_table_read(struct Parse *parser, struct Table *table) 5. You can make table be const struct Table *. > { > - Vdbe *v = sqlite3GetVdbe(p); > - int i; > - int iEnd = sqlite3VdbeCurrentAddr(v); > - > - for (i = 1; i < iEnd; i++) { > - VdbeOp *pOp = sqlite3VdbeGetOp(v, i); > - assert(pOp != 0); > - /* Currently, there is no difference between > - * Read and Write cursors. > + struct Vdbe *v = sqlite3GetVdbe(parser); > + int last_instr = sqlite3VdbeCurrentAddr(v); > + for (int i = 1; i < last_instr; i++) { > + struct VdbeOp *pOp = sqlite3VdbeGetOp(v, i); 6. When a function is refactored completely, we should use Tarantool code style for variable names. > + assert(pOp != NULL); > + /* > + * Currently, there is no difference between Read > + * and Write cursors. > */ > - if (pOp->opcode == OP_OpenRead || > - pOp->opcode == OP_OpenWrite) { > - Index *pIndex; > - int tnum = pOp->p2; > - if (tnum == pTab->tnum) { > - return 1; > - } > - for (pIndex = pTab->pIndex; pIndex; > - pIndex = pIndex->pNext) { > - if (tnum == pIndex->tnum) { > - return 1; > - } > + if (pOp->opcode == OP_OpenRead || pOp->opcode == OP_OpenWrite) { > + assert(i > 1); > + struct VdbeOp *space_var_op = > + sqlite3VdbeGetOp(v, i - 1); > + assert(space_var_op != NULL); > + assert(space_var_op->opcode == OP_LoadPtr); > + struct space *space = space_var_op->p4.space; > + > + if (space == space_by_id(table->def->id)) 7. Why do you need space_by_id()? Why not just space->id == table->def->id? > + return true; > + > + int idx_id = pOp->p2; > + for (struct Index *pIndex = table->pIndex; > + pIndex != NULL; pIndex = pIndex->pNext) { > + if (idx_id == > + SQLITE_PAGENO_TO_INDEXID(pIndex->tnum)) > + return true; > } > } > } > - return 0; > + return false; > } > >