Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v1 1/1] sql: remove registerTrace() from mem.c
@ 2021-12-01  8:06 Mergen Imeev via Tarantool-patches
  2021-12-02 10:42 ` Kirill Yukhin via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-12-01  8:06 UTC (permalink / raw)
  To: kyukhin; +Cc: tarantool-patches

This patch changes the way values are displayed in debug information.
---
https://github.com/tarantool/tarantool/tree/imeevma/gh-3050-drop-registerTrace

 src/box/sql/mem.c  | 119 ---------------------------------------------
 src/box/sql/mem.h  |   3 --
 src/box/sql/vdbe.c |  13 +++--
 3 files changed, 8 insertions(+), 127 deletions(-)

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 76d129c4c..296c96aba 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2595,125 +2595,6 @@ sqlVdbeCheckMemInvariants(Mem * p)
 	}
 	return 1;
 }
-
-/*
- * Write a nice string representation of the contents of cell pMem
- * into buffer zBuf, length nBuf.
- */
-void
-sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
-{
-	char *zCsr = zBuf;
-	int f = pMem->flags;
-
-	if (pMem->type == MEM_TYPE_BIN) {
-		int i;
-		char c;
-		if ((f & MEM_Static) != 0) {
-			c = 't';
-			assert((f & MEM_Ephem) == 0);
-		} else if (f & MEM_Ephem) {
-			c = 'e';
-			assert((f & MEM_Static) == 0);
-		} else {
-			c = 's';
-		}
-
-		sql_snprintf(100, zCsr, "%c", c);
-		zCsr += sqlStrlen30(zCsr);
-		sql_snprintf(100, zCsr, "%d[", pMem->n);
-		zCsr += sqlStrlen30(zCsr);
-		for(i=0; i<16 && i<pMem->n; i++) {
-			sql_snprintf(100, zCsr, "%02X", ((int)pMem->z[i] & 0xFF));
-			zCsr += sqlStrlen30(zCsr);
-		}
-		for(i=0; i<16 && i<pMem->n; i++) {
-			char z = pMem->z[i];
-			if (z<32 || z>126) *zCsr++ = '.';
-			else *zCsr++ = z;
-		}
-		sql_snprintf(100, zCsr, "]%s", "(8)");
-		zCsr += sqlStrlen30(zCsr);
-		*zCsr = '\0';
-	} else if (pMem->type == MEM_TYPE_STR) {
-		int j, k;
-		zBuf[0] = ' ';
-		if ((f & MEM_Static) != 0) {
-			zBuf[1] = 't';
-			assert((f & MEM_Ephem) == 0);
-		} else if (f & MEM_Ephem) {
-			zBuf[1] = 'e';
-			assert((f & MEM_Static) == 0);
-		} else {
-			zBuf[1] = 's';
-		}
-		k = 2;
-		sql_snprintf(100, &zBuf[k], "%d", pMem->n);
-		k += sqlStrlen30(&zBuf[k]);
-		zBuf[k++] = '[';
-		for(j=0; j<15 && j<pMem->n; j++) {
-			u8 c = pMem->z[j];
-			if (c>=0x20 && c<0x7f) {
-				zBuf[k++] = c;
-			} else {
-				zBuf[k++] = '.';
-			}
-		}
-		zBuf[k++] = ']';
-		sql_snprintf(100,&zBuf[k],"(8)");
-		k += sqlStrlen30(&zBuf[k]);
-		zBuf[k++] = 0;
-	}
-}
-
-/*
- * Print the value of a register for tracing purposes:
- */
-static void
-memTracePrint(Mem *p)
-{
-	switch (p->type) {
-	case MEM_TYPE_NULL:
-		printf(" NULL");
-		return;
-	case MEM_TYPE_INT:
-		printf(" i:%lld", p->u.i);
-		return;
-	case MEM_TYPE_UINT:
-		printf(" u:%"PRIu64"", p->u.u);
-		return;
-	case MEM_TYPE_DOUBLE:
-		printf(" r:%g", p->u.r);
-		return;
-	case MEM_TYPE_INVALID:
-		printf(" undefined");
-		return;
-	case MEM_TYPE_BOOL:
-		printf(" bool:%s", SQL_TOKEN_BOOLEAN(p->u.b));
-		return;
-	case MEM_TYPE_UUID:
-		printf(" uuid:%s", tt_uuid_str(&p->u.uuid));
-		return;
-	case MEM_TYPE_DEC:
-		printf(" decimal:%s", decimal_str(&p->u.d));
-		return;
-	default: {
-		char zBuf[200];
-		sqlVdbeMemPrettyPrint(p, zBuf);
-		printf(" %s", zBuf);
-		if ((p->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 0)
-			printf(" subtype=0x%02x", SQL_SUBTYPE_MSGPACK);
-		return;
-	}
-	}
-}
-
-void
-registerTrace(int iReg, Mem *p) {
-	printf("REG[%d] = ", iReg);
-	memTracePrint(p);
-	printf("\n");
-}
 #endif
 
 static int
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index e59f8ea44..ba36d17a7 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -784,9 +784,6 @@ mem_mp_type(const struct Mem *mem);
 
 #ifdef SQL_DEBUG
 int sqlVdbeCheckMemInvariants(struct Mem *);
-void sqlVdbeMemPrettyPrint(Mem * pMem, char *zBuf);
-void
-registerTrace(int iReg, Mem *p);
 
 /*
  * Return true if a memory cell is not marked as invalid.  This macro
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 0c4e38557..1d68d387f 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -253,7 +253,8 @@ allocateCursor(
 
 #ifdef SQL_DEBUG
 #  define REGISTER_TRACE(P,R,M)					\
-	if(P->sql_flags&SQL_VdbeTrace) registerTrace(R,M);
+	if ((P->sql_flags & SQL_VdbeTrace) != 0)		\
+		printf("REG[%d] = %s\n", R, mem_str(M));
 #else
 #  define REGISTER_TRACE(P,R,M)
 #endif
@@ -4393,11 +4394,13 @@ default: {          /* This is really OP_Noop and OP_Explain */
 		if ((p->sql_flags & SQL_VdbeTrace) != 0) {
 			u8 opProperty = sqlOpcodeProperty[pOrigOp->opcode];
 			if (rc!=0) printf("rc=%d\n",rc);
-			if (opProperty & (OPFLG_OUT2)) {
-				registerTrace(pOrigOp->p2, &aMem[pOrigOp->p2]);
+			if ((opProperty & OPFLG_OUT2) != 0) {
+				REGISTER_TRACE(p, pOrigOp->p2,
+					       &aMem[pOrigOp->p2]);
 			}
-			if (opProperty & OPFLG_OUT3) {
-				registerTrace(pOrigOp->p3, &aMem[pOrigOp->p3]);
+			if ((opProperty & OPFLG_OUT3) != 0) {
+				REGISTER_TRACE(p, pOrigOp->p3,
+					       &aMem[pOrigOp->p3]);
 			}
 		}
 #endif  /* SQL_DEBUG */
-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove registerTrace() from mem.c
  2021-12-01  8:06 [Tarantool-patches] [PATCH v1 1/1] sql: remove registerTrace() from mem.c Mergen Imeev via Tarantool-patches
@ 2021-12-02 10:42 ` Kirill Yukhin via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Kirill Yukhin via Tarantool-patches @ 2021-12-02 10:42 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Hello,

On 01 дек 11:06, imeevma@tarantool.org wrote:
> This patch changes the way values are displayed in debug information.
> ---
> https://github.com/tarantool/tarantool/tree/imeevma/gh-3050-drop-registerTrace

LGTM.
I've checked your patch into master.

--
Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove registerTrace() from mem.c
  2021-12-02 23:46     ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-12-03 14:48       ` Mergen Imeev via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-12-03 14:48 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

On Fri, Dec 03, 2021 at 12:46:34AM +0100, Vladislav Shpilevoy wrote:
> On 01.12.2021 08:59, Mergen Imeev via Tarantool-patches wrote:
> > Hi! Thank you for the review! My answer below.
> > 
> > On Tue, Nov 30, 2021 at 10:00:44PM +0100, Vladislav Shpilevoy wrote:
> >> Hi! Thanks for the patch looks good, but I would drop this 'register trace'
> >> entirely. Everything that uses 'printf' is dead anyway.
> >>
> > I'm not sure about that. For example, I have executed these statements on master and on my branch:
> 
> It is a misunderstanding. I didn't mean you patch does not
> change anything. I meant that this whole mechanism of 'register traces'
> is dead IMO. It is unusable. For example, if you need to debug something
> on a remote server, you won't see these printf() results anyway. Because
> you perhaps will talk to the server using IProto. So whatever the server
> prints to stdout you simply won't see. You will use box.execute EXPLAIN,
> right?
> 
> As 'dead' here I meant useless/unusable/not-worth-the-efforts-of-fixing.
> Not unreachable.
Thank you for the explanation! I will think about what we can do to replace
this functionality by something more useful.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove registerTrace() from mem.c
  2021-12-01  7:59   ` Mergen Imeev via Tarantool-patches
@ 2021-12-02 23:46     ` Vladislav Shpilevoy via Tarantool-patches
  2021-12-03 14:48       ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-12-02 23:46 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

On 01.12.2021 08:59, Mergen Imeev via Tarantool-patches wrote:
> Hi! Thank you for the review! My answer below.
> 
> On Tue, Nov 30, 2021 at 10:00:44PM +0100, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the patch looks good, but I would drop this 'register trace'
>> entirely. Everything that uses 'printf' is dead anyway.
>>
> I'm not sure about that. For example, I have executed these statements on master and on my branch:

It is a misunderstanding. I didn't mean you patch does not
change anything. I meant that this whole mechanism of 'register traces'
is dead IMO. It is unusable. For example, if you need to debug something
on a remote server, you won't see these printf() results anyway. Because
you perhaps will talk to the server using IProto. So whatever the server
prints to stdout you simply won't see. You will use box.execute EXPLAIN,
right?

As 'dead' here I meant useless/unusable/not-worth-the-efforts-of-fixing.
Not unreachable.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove registerTrace() from mem.c
  2021-11-30 21:00 ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-12-01  7:59   ` Mergen Imeev via Tarantool-patches
  2021-12-02 23:46     ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-12-01  7:59 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review! My answer below.

On Tue, Nov 30, 2021 at 10:00:44PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch looks good, but I would drop this 'register trace'
> entirely. Everything that uses 'printf' is dead anyway.
> 
I'm not sure about that. For example, I have executed these statements on master and on my branch:

box.execute('set session "sql_vdbe_debug" = true;')
box.execute([[select 1;]])

Result on master:

SQL: [select 1;]
VDBE Program Listing:
 103>    0 Init             0    1    0               00 Start at 1
 103>    1 Integer          1    1    0               00 r[1]=1
 103>    2 ResultRow        1    1    0               00 output=r[1]
 103>    3 Halt             0    0    0               00 
VDBE Trace:
 103>    0 Init             0    1    0               00 Start at 1
SQL-trace: select 1;
 103>    1 Integer          1    1    0               00 r[1]=1
REG[1] =  u:1
 103>    2 ResultRow        1    1    0               00 output=r[1]
REG[1] =  u:1
 103>    3 Halt             0    0    0               00 
---
- metadata:
  - name: COLUMN_1
    type: integer
  rows:
  - [1]
...


Result on the branch:

SQL: [select 1;]
VDBE Program Listing:
 103>    0 Init             0    1    0               00 Start at 1
 103>    1 Integer          1    1    0               00 r[1]=1
 103>    2 ResultRow        1    1    0               00 output=r[1]
 103>    3 Halt             0    0    0               00 
VDBE Trace:
 103>    0 Init             0    1    0               00 Start at 1
SQL-trace: select 1;
 103>    1 Integer          1    1    0               00 r[1]=1
REG[1] = integer(1)
 103>    2 ResultRow        1    1    0               00 output=r[1]
REG[1] = integer(1)
 103>    3 Halt             0    0    0               00 
---
- metadata:
  - name: COLUMN_1
    type: integer
  rows:
  - [1]
...

As you can see, the contents of REG[1] are displayed differently.

> LGTM. Drop more code or send this patch further, does not matter.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove registerTrace() from mem.c
  2021-11-24 12:22 Mergen Imeev via Tarantool-patches
@ 2021-11-30 21:00 ` Vladislav Shpilevoy via Tarantool-patches
  2021-12-01  7:59   ` Mergen Imeev via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-11-30 21:00 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

Hi! Thanks for the patch looks good, but I would drop this 'register trace'
entirely. Everything that uses 'printf' is dead anyway.

LGTM. Drop more code or send this patch further, does not matter.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Tarantool-patches] [PATCH v1 1/1] sql: remove registerTrace() from mem.c
@ 2021-11-24 12:22 Mergen Imeev via Tarantool-patches
  2021-11-30 21:00 ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-11-24 12:22 UTC (permalink / raw)
  To: v.shpilevoy; +Cc: tarantool-patches

This patch changes the way values are displayed in debug information.
---
https://github.com/tarantool/tarantool/issues/3050
https://github.com/tarantool/tarantool/tree/imeevma/gh-3050-drop-registerTrace

 src/box/sql/mem.c  | 119 ---------------------------------------------
 src/box/sql/mem.h  |   3 --
 src/box/sql/vdbe.c |  13 +++--
 3 files changed, 8 insertions(+), 127 deletions(-)

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 530bde615..b7bf5e892 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -2595,125 +2595,6 @@ sqlVdbeCheckMemInvariants(Mem * p)
 	}
 	return 1;
 }
-
-/*
- * Write a nice string representation of the contents of cell pMem
- * into buffer zBuf, length nBuf.
- */
-void
-sqlVdbeMemPrettyPrint(Mem *pMem, char *zBuf)
-{
-	char *zCsr = zBuf;
-	int f = pMem->flags;
-
-	if (pMem->type == MEM_TYPE_BIN) {
-		int i;
-		char c;
-		if ((f & MEM_Static) != 0) {
-			c = 't';
-			assert((f & MEM_Ephem) == 0);
-		} else if (f & MEM_Ephem) {
-			c = 'e';
-			assert((f & MEM_Static) == 0);
-		} else {
-			c = 's';
-		}
-
-		sql_snprintf(100, zCsr, "%c", c);
-		zCsr += sqlStrlen30(zCsr);
-		sql_snprintf(100, zCsr, "%d[", pMem->n);
-		zCsr += sqlStrlen30(zCsr);
-		for(i=0; i<16 && i<pMem->n; i++) {
-			sql_snprintf(100, zCsr, "%02X", ((int)pMem->z[i] & 0xFF));
-			zCsr += sqlStrlen30(zCsr);
-		}
-		for(i=0; i<16 && i<pMem->n; i++) {
-			char z = pMem->z[i];
-			if (z<32 || z>126) *zCsr++ = '.';
-			else *zCsr++ = z;
-		}
-		sql_snprintf(100, zCsr, "]%s", "(8)");
-		zCsr += sqlStrlen30(zCsr);
-		*zCsr = '\0';
-	} else if (pMem->type == MEM_TYPE_STR) {
-		int j, k;
-		zBuf[0] = ' ';
-		if ((f & MEM_Static) != 0) {
-			zBuf[1] = 't';
-			assert((f & MEM_Ephem) == 0);
-		} else if (f & MEM_Ephem) {
-			zBuf[1] = 'e';
-			assert((f & MEM_Static) == 0);
-		} else {
-			zBuf[1] = 's';
-		}
-		k = 2;
-		sql_snprintf(100, &zBuf[k], "%d", pMem->n);
-		k += sqlStrlen30(&zBuf[k]);
-		zBuf[k++] = '[';
-		for(j=0; j<15 && j<pMem->n; j++) {
-			u8 c = pMem->z[j];
-			if (c>=0x20 && c<0x7f) {
-				zBuf[k++] = c;
-			} else {
-				zBuf[k++] = '.';
-			}
-		}
-		zBuf[k++] = ']';
-		sql_snprintf(100,&zBuf[k],"(8)");
-		k += sqlStrlen30(&zBuf[k]);
-		zBuf[k++] = 0;
-	}
-}
-
-/*
- * Print the value of a register for tracing purposes:
- */
-static void
-memTracePrint(Mem *p)
-{
-	switch (p->type) {
-	case MEM_TYPE_NULL:
-		printf(" NULL");
-		return;
-	case MEM_TYPE_INT:
-		printf(" i:%lld", p->u.i);
-		return;
-	case MEM_TYPE_UINT:
-		printf(" u:%"PRIu64"", p->u.u);
-		return;
-	case MEM_TYPE_DOUBLE:
-		printf(" r:%g", p->u.r);
-		return;
-	case MEM_TYPE_INVALID:
-		printf(" undefined");
-		return;
-	case MEM_TYPE_BOOL:
-		printf(" bool:%s", SQL_TOKEN_BOOLEAN(p->u.b));
-		return;
-	case MEM_TYPE_UUID:
-		printf(" uuid:%s", tt_uuid_str(&p->u.uuid));
-		return;
-	case MEM_TYPE_DEC:
-		printf(" decimal:%s", decimal_str(&p->u.d));
-		return;
-	default: {
-		char zBuf[200];
-		sqlVdbeMemPrettyPrint(p, zBuf);
-		printf(" %s", zBuf);
-		if ((p->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 0)
-			printf(" subtype=0x%02x", SQL_SUBTYPE_MSGPACK);
-		return;
-	}
-	}
-}
-
-void
-registerTrace(int iReg, Mem *p) {
-	printf("REG[%d] = ", iReg);
-	memTracePrint(p);
-	printf("\n");
-}
 #endif
 
 static int
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index e59f8ea44..ba36d17a7 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -784,9 +784,6 @@ mem_mp_type(const struct Mem *mem);
 
 #ifdef SQL_DEBUG
 int sqlVdbeCheckMemInvariants(struct Mem *);
-void sqlVdbeMemPrettyPrint(Mem * pMem, char *zBuf);
-void
-registerTrace(int iReg, Mem *p);
 
 /*
  * Return true if a memory cell is not marked as invalid.  This macro
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 0c4e38557..1d68d387f 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -253,7 +253,8 @@ allocateCursor(
 
 #ifdef SQL_DEBUG
 #  define REGISTER_TRACE(P,R,M)					\
-	if(P->sql_flags&SQL_VdbeTrace) registerTrace(R,M);
+	if ((P->sql_flags & SQL_VdbeTrace) != 0)		\
+		printf("REG[%d] = %s\n", R, mem_str(M));
 #else
 #  define REGISTER_TRACE(P,R,M)
 #endif
@@ -4393,11 +4394,13 @@ default: {          /* This is really OP_Noop and OP_Explain */
 		if ((p->sql_flags & SQL_VdbeTrace) != 0) {
 			u8 opProperty = sqlOpcodeProperty[pOrigOp->opcode];
 			if (rc!=0) printf("rc=%d\n",rc);
-			if (opProperty & (OPFLG_OUT2)) {
-				registerTrace(pOrigOp->p2, &aMem[pOrigOp->p2]);
+			if ((opProperty & OPFLG_OUT2) != 0) {
+				REGISTER_TRACE(p, pOrigOp->p2,
+					       &aMem[pOrigOp->p2]);
 			}
-			if (opProperty & OPFLG_OUT3) {
-				registerTrace(pOrigOp->p3, &aMem[pOrigOp->p3]);
+			if ((opProperty & OPFLG_OUT3) != 0) {
+				REGISTER_TRACE(p, pOrigOp->p3,
+					       &aMem[pOrigOp->p3]);
 			}
 		}
 #endif  /* SQL_DEBUG */
-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-12-03 14:48 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-01  8:06 [Tarantool-patches] [PATCH v1 1/1] sql: remove registerTrace() from mem.c Mergen Imeev via Tarantool-patches
2021-12-02 10:42 ` Kirill Yukhin via Tarantool-patches
  -- strict thread matches above, loose matches on Subject: below --
2021-11-24 12:22 Mergen Imeev via Tarantool-patches
2021-11-30 21:00 ` Vladislav Shpilevoy via Tarantool-patches
2021-12-01  7:59   ` Mergen Imeev via Tarantool-patches
2021-12-02 23:46     ` Vladislav Shpilevoy via Tarantool-patches
2021-12-03 14:48       ` Mergen Imeev via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox