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 A462224AA4 for ; Fri, 6 Jul 2018 14:13:25 -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 ipGnAhulzc3f for ; Fri, 6 Jul 2018 14:13:25 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 402DD24A4F for ; Fri, 6 Jul 2018 14:13:25 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/2] sql: get rid off sqlite3NestedParse in clean stats References: <355d8c55-abc7-a088-8dba-6fb3bff32b51@tarantool.org> From: Kirill Shcherbatov Message-ID: Date: Fri, 6 Jul 2018 21:13:22 +0300 MIME-Version: 1.0 In-Reply-To: <355d8c55-abc7-a088-8dba-6fb3bff32b51@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Vladislav Shpilevoy > I have pushed my review fixes on the branch. Please, squash, if > you agree, and debug if the tests fail. Below you may find 3 > comments that I fixed. Style violations I did not mentioned here. > Just fixed on the branch. Hello! Thank you for so detailed review fixes. I've squashed your changes, they are looking good for me and they also pass all tests. > Also I have found that vdbe_emit_open_cursor() has the second > parameter named 'index_id', but in some places the function > takes real index_id, in other places it takes tnum, and in vdbe > it is interpreted as tnum. Please, fix this mess in a separate > commit. I think, we should always pass index_id. I've tried to do this and I'll append my diff to the end of this message, but there are some strange things.. This should be investigated separately. I don't mind to do this, but I believe that this may delayed to be not a part of "remove sqlite3NestedParse function". Take a look at the end of this message. > 1. Unused array. > 2. You do not need to lookup pStat for tnum in non-debug build since > you know stat tables id from schema_def.h. This HashFind is useful for > debug assertion only. You've fixed all of this. >> + struct Expr *col_eq_expr = >> + sqlite3PExpr(parse, TK_EQ, col_type_expr, col_name_expr); >> + if (col_type_expr == NULL || col_name_expr == NULL) { > > 3. If col_eq_expr == NULL here, then col_type/name_expr leak. As you already coincide (according to you changes on branch), there is no leak as sqlite3PExpr manually releases arguments on failure. ---------------------------------- >From c5c1d325c32a3d9d9e75fbb8f6a8d58e3bec3a34 Mon Sep 17 00:00:00 2001 Message-Id: In-Reply-To: References: From: Kirill Shcherbatov Date: Fri, 6 Jul 2018 20:18:08 +0300 Subject: [PATCH 3/3] crap --- src/box/sql/analyze.c | 3 +-- src/box/sql/build.c | 3 ++- src/box/sql/expr.c | 5 ++++- src/box/sql/fkey.c | 3 ++- src/box/sql/insert.c | 8 ++++++-- src/box/sql/select.c | 4 +--- src/box/sql/vdbe.c | 4 +++- src/box/sql/where.c | 18 ++++++++++++++++-- 8 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index 336d146..ff3e735 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -159,8 +159,7 @@ vdbe_emit_stat_space_open(struct Parse *parse, int stat_cursor, /* Open the sql_stat tables for writing. */ for (uint i = 0; i < lengthof(space_names); ++i) { uint32_t id = space_ids[i]; - int tnum = SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(id, 0); - vdbe_emit_open_cursor(parse, stat_cursor + i, tnum, + vdbe_emit_open_cursor(parse, stat_cursor + i, 0, space_by_id(id)); VdbeComment((v, space_names[i])); } diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 1c00842..5f7a35a 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2448,7 +2448,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum), 0); struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum)); - vdbe_emit_open_cursor(pParse, iIdx, tnum, space); + vdbe_emit_open_cursor(pParse, iIdx, SQLITE_PAGENO_TO_INDEXID(tnum), + space); sqlite3VdbeChangeP5(v, memRootPage >= 0 ? OPFLAG_P2ISREG : 0); addr1 = sqlite3VdbeAddOp2(v, OP_SorterSort, iSorter, 0); diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 3183e3d..b1650cf 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -2470,8 +2470,11 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */ P4_DYNAMIC); struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum)); + uint32_t idx_id = + SQLITE_PAGENO_TO_INDEXID(pIdx-> + tnum); vdbe_emit_open_cursor(pParse, iTab, - pIdx->tnum, space); + idx_id, space); VdbeComment((v, "%s", pIdx->zName)); assert(IN_INDEX_INDEX_DESC == IN_INDEX_INDEX_ASC + 1); diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c index 6c75c47..face9cb 100644 --- a/src/box/sql/fkey.c +++ b/src/box/sql/fkey.c @@ -441,7 +441,8 @@ fkLookupParent(Parse * pParse, /* Parse context */ int regRec = sqlite3GetTempReg(pParse); struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum)); - vdbe_emit_open_cursor(pParse, iCur, pIdx->tnum, space); + uint32_t idx_id = SQLITE_PAGENO_TO_INDEXID(pIdx->tnum); + vdbe_emit_open_cursor(pParse, iCur, idx_id, space); for (i = 0; i < nCol; i++) { sqlite3VdbeAddOp2(v, OP_Copy, aiCol[i] + 1 + regData, diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 58e159c..3beae89 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -1934,11 +1934,15 @@ xferOptimization(Parse * pParse, /* Parser context */ assert(pSrcIdx); struct space *src_space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum)); - vdbe_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space); + vdbe_emit_open_cursor(pParse, iSrc, + SQLITE_PAGENO_TO_INDEXID(pSrcIdx->tnum), + src_space); VdbeComment((v, "%s", pSrcIdx->zName)); struct space *dest_space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum)); - vdbe_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space); + vdbe_emit_open_cursor(pParse, iDest, + SQLITE_PAGENO_TO_INDEXID(pDestIdx->tnum), + dest_space); VdbeComment((v, "%s", pDestIdx->zName)); addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0); VdbeCoverage(v); diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 52b3fdd..ceb7e34 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -6279,9 +6279,7 @@ sqlite3Select(Parse * pParse, /* The parser context */ * Open the cursor, execute the OP_Count, * close the cursor. */ - vdbe_emit_open_cursor(pParse, cursor, - space->def->id << 10, - space); + vdbe_emit_open_cursor(pParse, cursor, 0, space); sqlite3VdbeAddOp2(v, OP_Count, cursor, sAggInfo.aFunc[0].iMem); sqlite3VdbeAddOp1(v, OP_Close, cursor); diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 7a4d376..00b67dc 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -3208,12 +3208,14 @@ case OP_OpenWrite: "need to re-compile SQL statement"); goto abort_due_to_error; } + pOp->p2 = SQLITE_PAGENO_TO_INDEXID(p2); p2 = pOp->p2; + pIn3 = &aMem[pOp->p3]; assert(pIn3->flags & MEM_Ptr); struct space *space = ((struct space *) pIn3->u.p); assert(space != NULL); - struct index *index = space_index(space, SQLITE_PAGENO_TO_INDEXID(p2)); + struct index *index = space_index(space, p2); assert(index != NULL); /* * Since Tarantool iterator provides the full tuple, diff --git a/src/box/sql/where.c b/src/box/sql/where.c index 85143ed..f83456a 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -4759,10 +4759,24 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */ assert(iIndexCur >= 0); if (op) { if (pIx != NULL) { + /* + * This diff cause really strange + * effects for memtx. + * Failed to allocate * bytes for tuple: + * tuple is too large. + * Check 'memtx_max_tuple_size' + * configuration option. + */ + uint32_t space_id = + SQLITE_PAGENO_TO_SPACEID(pIx-> + tnum); struct space *space = - space_by_id(SQLITE_PAGENO_TO_SPACEID(pIx->tnum)); + space_by_id(space_id); + uint32_t idx_id = + SQLITE_PAGENO_TO_INDEXID(pIx-> + tnum); vdbe_emit_open_cursor(pParse, iIndexCur, - pIx->tnum, space); + idx_id, space); } else { vdbe_emit_open_cursor(pParse, iIndexCur, idx_def->iid, -- 2.7.4 ---------------------------------- [001] sql-tap/misc3.test.lua vinyl [ pass ] [002] sql-tap/misc3.test.lua memtx [002] not ok 2 - misc3-1.2: Execution failed: Failed to allocate 6553623 bytes for tuple: tuple is too large. Check 'memtx_max_tuple_size' configuration option. # [002] Traceback: [002] [Lua ] function 'do_test' at <./sqltester.lua:134> [002] [main] at [002] [002] not ok 25 - misc3-4.1: Execution failed: Failed to allocate 12131 bytes in slab allocator for memtx_tuple # [002] Traceback: [002] [Lua ] function 'do_execsql_test' at <./sqltester.lua:134> [002] [main] at [002] [002] not ok 26 - misc3-4.2 # [002] Traceback: [002] [Lua ] function 'do_execsql_test' at <./sqltester.lua:134> [002] [main] at [002] [002] not ok 27 - misc3-4.3 # [002] Traceback: [002] [Lua ] function 'do_execsql_test' at <./sqltester.lua:134> [002] [main] at [002] [002] Rejected result file: sql-tap/misc3.reject [002] [ fail ] [Main process] Got failed test; gently terminate all workers... [002] Worker "002_sql-tap" got failed test; stopping the server...