[tarantool-patches] Re: [PATCH] sql: print space names in VDBE dump
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu May 17 19:13:06 MSK 2018
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<name=%s>", space_name(pOp->p4.space));
> + break;
> + }
> default:{
> zP4 = pOp->p4.z;
> if (zP4 == 0) {
>
More information about the Tarantool-patches
mailing list