[tarantool-patches] Re: [PATCH v2 2/6] sql: fix "PRAGMA parser_trace" result

["n.pettik" <korablev@tarantool.org>]

> Part of #3832

Why is this patch part of #3832?
Couldn’t you implement #3832 without this patch?

> ---
> src/box/sql/pragma.c     | 20 +++++++++++++-------
> src/box/sql/pragma.h     |  4 ++--
> src/box/sql/sqliteInt.h  |  2 ++
> test/sql/errinj.result   | 19 +++++++++++++++++++
> test/sql/errinj.test.lua | 16 ++++++++++++++++
> 5 files changed, 52 insertions(+), 9 deletions(-)
> 
> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
> index c052015..1fe5b3e 100644
> --- a/src/box/sql/pragma.c
> +++ b/src/box/sql/pragma.c
> @@ -568,15 +568,21 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
> 	}
> #ifndef NDEBUG
> 	case PragTyp_PARSER_TRACE:{
> -			if (zRight) {
> -				if (sqlite3GetBoolean(zRight, 0)) {
> -					sqlite3ParserTrace(stdout, "parser: ");
> -				} else {
> -					sqlite3ParserTrace(0, 0);
> -				}
> +		int mask = pPragma->iArg;
> +		if (zRight == NULL) {
> +			returnSingleInt(v,
> +					(user_session->sql_flags & mask) != 0);
> +		} else {
> +			if (sqlite3GetBoolean(zRight, 0)) {
> +				sqlite3ParserTrace(stdout, "parser: ");
> +				user_session->sql_flags |= mask;
> +			} else {
> +				sqlite3ParserTrace(0, 0);
> +				user_session->sql_flags &= ~mask;
> 			}
> -			break;
> 		}

Why can’t you simply set to this pragma type ‘flag’?
It works almost that way.

> +		break;
> +	}
> #endif
> 
> 		/*
> diff --git a/src/box/sql/pragma.h b/src/box/sql/pragma.h
> index 84ab478..59e0e1e 100644
> --- a/src/box/sql/pragma.h
> +++ b/src/box/sql/pragma.h
> @@ -138,9 +138,9 @@ static const PragmaName aPragmaName[] = {
> #if defined(SQLITE_DEBUG)
> 	{ /* zName:     */ "parser_trace",
> 	 /* ePragTyp:  */ PragTyp_PARSER_TRACE,
> -	 /* ePragFlg:  */ 0,
> +	 /* ePragFlg:  */ PragFlg_Result0 | PragFlg_NoColumns1,
> 	 /* ColNames:  */ 0, 0,
> -	 /* iArg:      */ 0},
> +	 /* iArg:      */ SQLITE_ParserTrace},
> #endif
> 	{ /* zName:     */ "query_only",
> 	 /* ePragTyp:  */ PragTyp_FLAG,
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 1ec52b8..3d4dead 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -1563,6 +1563,8 @@ struct sqlite3 {
>  * Possible values for the sqlite3.flags.
>  */
> #define SQLITE_VdbeTrace      0x00000001	/* True to trace VDBE execution */
> +/* Debug print info about SQL query as it parsed */
> +#define SQLITE_ParserTrace    0x00000002

Let's avoid using ’sqlite’ prefix in any added code.

> #define SQLITE_FullColNames   0x00000004	/* Show full column names on SELECT */
> #define SQLITE_ShortColNames  0x00000040	/* Show short columns names */
> #define SQLITE_CountRows      0x00000080	/* Count rows changed by INSERT, */
> diff --git a/test/sql/errinj.result b/test/sql/errinj.result
> index cb993f8..6ba7e72 100644
> --- a/test/sql/errinj.result
> +++ b/test/sql/errinj.result
> @@ -280,3 +280,22 @@ errinj.set("ERRINJ_WAL_IO", false)
> box.sql.execute("DROP TABLE t3;")
> ---
> ...
> +--
> +-- gh-3832: Some statements do not return column type
> +-- This test placed here because it should be skipped in release
> +-- build.
> +-- Check that "PRAGMA parser_trace" returns 0 or 1 if called
> +-- without parameter.
> +result = box.sql.execute('PRAGMA parser_trace')
> +---
> +...
> +-- Should be TRUE.
> +result[1][1] == 0 or result[1][1] == 1
> +---
> +- true
> +...
> +-- Check that "PRAGMA parser_trace" returns nothing if called
> +-- with parameter.
> +box.sql.execute('PRAGMA parser_trace = '.. result[1][1])
> +---
> +...
> diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
> index fa7f9f2..a938eda 100644
> --- a/test/sql/errinj.test.lua
> +++ b/test/sql/errinj.test.lua
> @@ -97,3 +97,19 @@ box.sql.execute("ALTER TABLE t3 DROP CONSTRAINT fk1;")
> box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
> errinj.set("ERRINJ_WAL_IO", false)
> box.sql.execute("DROP TABLE t3;")
> +
> +--
> +-- gh-3832: Some statements do not return column type
> +
> +-- This test placed here because it should be skipped in release
> +-- build.

Personally I'd rather create new test file for this (debug) purpose.
Also, your test doesn’t check value which has been set (it checks
only that it is bool):

Pragma parser_trace=1
pragma parser_trace — this must return 1, not 0.