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 B7237251E3 for ; Thu, 17 May 2018 12:13:10 -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 AzgaIjsShyba for ; Thu, 17 May 2018 12:13:10 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 578C2251E1 for ; Thu, 17 May 2018 12:13:10 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] sql: print space names in VDBE dump References: <3fa71f93e60eab628771742d2ac156b4c3ec60a1.1526563390.git.kyukhin@tarantool.org> <4b712e43-cd89-42f6-596a-6a775624592b@tarantool.org> <20180517160104.huzfdqsy7l23mlvc@tarantool.org> From: Vladislav Shpilevoy Message-ID: <689a12ed-0f88-56e1-9688-606f15ed3c56@tarantool.org> Date: Thu, 17 May 2018 19:13:06 +0300 MIME-Version: 1.0 In-Reply-To: <20180517160104.huzfdqsy7l23mlvc@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 Yukhin Cc: tarantool-patches@freelists.org LGTM. On 17/05/2018 19:01, Kirill Yukhin wrote: > Hello Vlad, > Thanks for review. > > Issue: n/a > Branch: https://github.com/tarantool/tarantool/tree/kyukhin/human-readable-s-name-in-trace > > My comments are inlined. Updated patch is in the bottom. > > On 17 мая 18:12, Vladislav Shpilevoy wrote: >> Hello. Thanks for the patch! See 3 comments below. >> >> On 17/05/2018 16:25, Kirill Yukhin wrote: >>> Before the patch, when LoadPtr instruction is dumped, >>> raw pointer value was printed, which was looked like a >>> garbade. The patch inroduces new pointer type: space ptr, >>> which allows to print space name during VDBE dump. >>> --- >> >> 1. Please, post here branch and issue link (if issue exists). > Done. > >> 2. Can not find any similar branch on github by kyukhin/ prefix. > Pushed. > >>> - sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, >>> - (void *) space); >>> + sql_vdbe_add_op4_spaceptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, >>> + space); >> >> 3. I think, all VdbeAppOp functions family must be refactored at once. >> You can introduce P4_SPACEPTR and use >> sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0, space, P4_SPACEPTR);. > Fixed. New function removed. > > -- > Regards, Kirill Yukhin > > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > index 50d212a..998dd5b 100644 > --- a/src/box/sql/analyze.c > +++ b/src/box/sql/analyze.c > @@ -910,8 +910,8 @@ analyzeOneTable(Parse * pParse, /* Parser context */ > struct space *space = > space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum)); > assert(space != NULL); > - sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, > - (void *) space); > + sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0, > + (void*)space, P4_SPACEPTR); > sqlite3VdbeAddOp3(v, OP_OpenRead, iIdxCur, pIdx->tnum, > space_ptr_reg); > sql_vdbe_set_p4_key_def(pParse, pIdx); > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index c96d351..d39f110 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1222,8 +1222,8 @@ emit_open_cursor(Parse *parse_context, int cursor, int entity_id) > assert(space != NULL); > Vdbe *vdbe = parse_context->pVdbe; > int space_ptr_reg = ++parse_context->nMem; > - sqlite3VdbeAddOp4Ptr(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, > - (void *) space); > + sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space, > + P4_SPACEPTR); > return sqlite3VdbeAddOp3(vdbe, OP_OpenWrite, cursor, entity_id, > space_ptr_reg); > } > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 22130ef..5ada87e 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -1613,7 +1613,8 @@ sqlite3OpenTableAndIndices(Parse * pParse, /* Parsing context */ > struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pTab->tnum)); > assert(space != NULL); > int space_ptr_reg = ++pParse->nMem; > - sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, (void *) space); > + sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space, > + P4_SPACEPTR); > > /* One iteration of this cycle adds OpenRead/OpenWrite which > * opens cursor for current index. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index b4b59da..e27fe2f 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -1074,12 +1074,12 @@ case OP_Int64: { /* out2 */ > /* Opcode: LoadPtr * P2 * P4 * > * Synopsis: r[P2] = P4 > * > - * P4 is a generic pointer. Copy it into register P2. > + * P4 is a generic or space pointer. Copy it into register P2. > */ > case OP_LoadPtr: { > pOut = out2Prerelease(p, pOp); > - assert(pOp->p4type == P4_PTR); > - pOut->u.p = pOp->p4.p; > + assert(pOp->p4type == P4_PTR || pOp->p4type == P4_SPACEPTR ); > + pOut->u.p = pOp->p4.space; > pOut->flags = MEM_Ptr; > break; > } > diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h > index 39f8da4..f6dad22 100644 > --- a/src/box/sql/vdbe.h > +++ b/src/box/sql/vdbe.h > @@ -86,6 +86,8 @@ struct VdbeOp { > int (*xAdvance) (BtCursor *, int *); > /** Used when p4type is P4_KEYDEF. */ > struct key_def *key_def; > + /** Used when p4type is P4_SPACEPTR. */ > + struct space *space; > } p4; > #ifdef SQLITE_ENABLE_EXPLAIN_COMMENTS > char *zComment; /* Comment to improve readability */ > @@ -146,6 +148,7 @@ typedef struct VdbeOpList VdbeOpList; > #define P4_BOOL (-17) /* P4 is a bool value */ > #define P4_PTR (-18) /* P4 is a generic pointer */ > #define P4_KEYDEF (-19) /* P4 is a pointer to key_def structure. */ > +#define P4_SPACEPTR (-20) /* P4 is a space pointer */ > > /* Error message codes for OP_Halt */ > #define P5_ConstraintNotNull 1 > @@ -224,7 +227,6 @@ int sqlite3VdbeAddOp3(Vdbe *, int, int, int, int); > int sqlite3VdbeAddOp4(Vdbe *, int, int, int, int, const char *zP4, int); > int sqlite3VdbeAddOp4Dup8(Vdbe *, int, int, int, int, const u8 *, int); > int sqlite3VdbeAddOp4Int(Vdbe *, int, int, int, int, int); > -int sqlite3VdbeAddOp4Ptr(Vdbe *, int, int, int, int, void *); > void sqlite3VdbeEndCoroutine(Vdbe *, int); > #if defined(SQLITE_DEBUG) && !defined(SQLITE_TEST_REALLOC_STRESS) > void sqlite3VdbeVerifyNoMallocRequired(Vdbe * p, int N); > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index e67fcae..1304149 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -444,16 +444,6 @@ sqlite3VdbeAddOp4Int(Vdbe * p, /* Add the opcode to this VM */ > return addr; > } > > -int > -sqlite3VdbeAddOp4Ptr(Vdbe *p, int op, int p1, int p2, int p3, void *ptr) > -{ > - int addr = sqlite3VdbeAddOp3(p, op, p1, p2, p3); > - VdbeOp *pOp = &p->aOp[addr]; > - pOp->p4type = P4_PTR; > - pOp->p4.p = ptr; > - return addr; > -} > - > /* Insert the end of a co-routine > */ > void > @@ -1638,6 +1628,10 @@ displayP4(Op * pOp, char *zTemp, int nTemp) > zTemp[0] = 0; > break; > } > + case P4_SPACEPTR: { > + sqlite3XPrintf(&x, "space", space_name(pOp->p4.space)); > + break; > + } > default:{ > zP4 = pOp->p4.z; > if (zP4 == 0) { >