[tarantool-patches] [PATCH] sql: remove pragma schema_version

[tarantool-patches] [PATCH] sql: remove pragma schema_version

[Kirill Yukhin]

This pragma is dead and produces nothing else but segfault.
Along w/ this pragma, remove now dead opcodes which set/read
schema_version and all related routines.
Also, improve opcode generation script.

Part of #3541
---
Branch: https://github.com/tarantool/tarantool/commits/kyukhin/gh-3541-remove-schema-version-pragma
Issue: https://github.com/tarantool/tarantool/issues/3541

 extra/mkopcodeh.sh    |  33 ++++++++++++--
 src/box/sql/build.c   |  29 ------------
 src/box/sql/pragma.c  | 122 +++++++-------------------------------------------
 src/box/sql/pragma.h  |  12 -----
 src/box/sql/trigger.c |   2 -
 src/box/sql/vdbe.c    |  45 +------------------
 6 files changed, 47 insertions(+), 196 deletions(-)

diff --git a/extra/mkopcodeh.sh b/extra/mkopcodeh.sh
index 63ad0d5..9e97a50 100755
--- a/extra/mkopcodeh.sh
+++ b/extra/mkopcodeh.sh
@@ -35,6 +35,7 @@ set -f   # disable pathname expansion
 
 currentOp=""
 nOp=0
+mxTk=-1
 newline="$(printf '\n')"
 IFS="$newline"
 while read line; do
@@ -106,6 +107,9 @@ while read line; do
                         eval "ARRAY_used_$val=1"
                         eval "ARRAY_sameas_$val=$sym"
                         eval "ARRAY_def_$val=$name"
+			if [ $val -gt $mxTk ] ; then
+                            mxTk=$val
+			fi
                     fi
                 ;;
                 jump) eval "ARRAY_jump_$name=1" ;;
@@ -220,8 +224,12 @@ while [ "$i" -lt "$nOp" ]; do
     i=$((i + 1))
 done
 max="$cnt"
+echo "//*************** $max $nOp $mxTk"
+if [ $mxTk -lt $nOp ] ; then
+    mxTk=$nOp
+fi
 i=0
-while [ "$i" -lt "$nOp" ]; do
+while [ "$i" -le "$mxTk" ]; do
     eval "used=\${ARRAY_used_$i:-}"
     if [ -z "$used" ]; then
         eval "ARRAY_def_$i=OP_NotUsed_$i"
@@ -251,9 +259,21 @@ done
 # Generate the bitvectors:
 ARRAY_bv_0=0
 i=0
-while [ "$i" -le "$max" ]; do
+while [ "$i" -le "$mxTk" ]; do
+    eval "is_exists=\${ARRAY_def_$i:-}"
+    if [ ! -n "$is_exists" ] ; then
+    echo "//SKIP $i"
+        i=$((i + 1))
+	continue
+    fi
     eval "name=\$ARRAY_def_$i"
     x=0
+    eval "is_exists=\${ARRAY_jump_$name:-}"
+    if [ ! -n "$is_exists" ] ; then
+    echo "//SKIP2 $i"
+        i=$((i + 1))
+	continue
+    fi
     eval "jump=\$ARRAY_jump_$name"
     eval "in1=\$ARRAY_in1_$name"
     eval "in2=\$ARRAY_in2_$name"
@@ -283,11 +303,16 @@ printf '%s\n' "#define OPFLG_OUT2        0x10  /* out2:  P2 is an output */"
 printf '%s\n' "#define OPFLG_OUT3        0x20  /* out3:  P3 is an output */"
 printf '%s\n' "#define OPFLG_INITIALIZER {\\"
 i=0
-while [ "$i" -le "$max" ]; do
+while [ "$i" -le "$mxTk" ]; do
     if [ "$((i % 8))" -eq 0 ]; then
         printf '/* %3d */' "$i"
     fi
-    eval "bv=\$ARRAY_bv_$i"
+    eval "is_exists=\${ARRAY_bv_$i:-}"
+    if [ ! -n "$is_exists" ] ; then
+        bv=0
+    else
+        eval "bv=\$ARRAY_bv_$i"
+    fi
     printf ' 0x%02x,' "$bv"
     if [ "$((i % 8))" -eq 7 ]; then
         printf '%s\n' "\\"
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5772ee5..dcbeeb0 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1086,33 +1086,6 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
 	return sqlite3VdbeAddOp4(parse_context->pVdbe, OP_OpenWrite, cursor,
 				 index_id, 0, (void *) space, P4_SPACEPTR);
 }
-/*
- * Generate code that will increment the schema cookie.
- *
- * The schema cookie is used to determine when the schema for the
- * database changes.  After each schema change, the cookie value
- * changes.  When a process first reads the schema it records the
- * cookie.  Thereafter, whenever it goes to access the database,
- * it checks the cookie to make sure the schema has not changed
- * since it was last read.
- *
- * This plan is not completely bullet-proof.  It is possible for
- * the schema to change multiple times and for the cookie to be
- * set back to prior value.  But schema changes are infrequent
- * and the probability of hitting the same cookie value is only
- * 1 chance in 2^32.  So we're safe enough.
- *
- * IMPLEMENTATION-OF: R-34230-56049 SQLite automatically increments
- * the schema-version whenever the schema changes.
- */
-void
-sqlite3ChangeCookie(Parse * pParse)
-{
-	sqlite3 *db = pParse->db;
-	Vdbe *v = pParse->pVdbe;
-	sqlite3VdbeAddOp3(v, OP_SetCookie, 0, 0,
-			  db->pSchema->schema_cookie + 1);
-}
 
 /*
  * Measure the number of characters needed to output the given
@@ -1495,7 +1468,6 @@ parseTableSchemaRecord(Parse * pParse, int iSpaceId, char *zStmt)
 		makeIndexSchemaRecord(pParse, pIdx, iSpaceId, ++i, NULL);
 	}
 
-	sqlite3ChangeCookie(pParse);
 	sqlite3VdbeAddParseSchema2Op(v, iTop, pParse->nMem - iTop + 1);
 }
 
@@ -2891,7 +2863,6 @@ sql_create_index(struct Parse *parse, struct Token *token,
 		 * Reparse the schema. Code an OP_Expire
 		 * to invalidate all pre-compiled statements.
 		 */
-		sqlite3ChangeCookie(parse);
 		sqlite3VdbeAddParseSchema2Op(vdbe, first_schema_col, 4);
 		sqlite3VdbeAddOp0(vdbe, OP_Expire);
 	}
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index d427f78..0d317ee 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -813,112 +813,25 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 			break;
 		}
 
-#ifndef SQLITE_INTEGRITY_CHECK_ERROR_MAX
-#define SQLITE_INTEGRITY_CHECK_ERROR_MAX 100
-#endif
-
-#ifndef SQLITE_OMIT_SCHEMA_VERSION_PRAGMAS
-		/* *   PRAGMA [schema.]schema_version *   PRAGMA
-		 * [schema.]schema_version = <integer> *
-		 *
-		 * PRAGMA [schema.]user_version *   PRAGMA
-		 * [schema.]user_version = <integer> *
-		 *
-		 * PRAGMA [schema.]freelist_count *
-		 *
-		 * PRAGMA [schema.]data_version *
-		 *
-		 * PRAGMA [schema.]application_id *   PRAGMA
-		 * [schema.]application_id = <integer> *
-		 *
-		 * The pragma's schema_version and user_version are used
-		 * to set or get * the value of the schema-version and
-		 * user-version, respectively. Both * the
-		 * schema-version and the user-version are 32-bit
-		 * signed integers * stored in the database header. *
-		 *
-		 * The schema-cookie is usually only manipulated
-		 * internally by SQLite. It * is incremented by SQLite
-		 * whenever the database schema is modified (by *
-		 * creating or dropping a table or index). The schema
-		 * version is used by * SQLite each time a query is
-		 * executed to ensure that the internal cache * of the
-		 * schema used when compiling the SQL query matches the
-		 * schema of * the database against which the compiled
-		 * query is actually executed. * Subverting this
-		 * mechanism by using "PRAGMA schema_version" to modify *
-		 * the schema-version is potentially dangerous and may
-		 * lead to program * crashes or database corruption.
-		 * Use with caution! *
-		 *
-		 * The user-version is not used internally by SQLite. It
-		 * may be used by * applications for any purpose.
-		 */
-	case PragTyp_HEADER_VALUE:{
-			int iCookie = pPragma->iArg;	/* Which cookie to read
-							 * or write
-							 */
-			if (zRight
-			    && (pPragma->mPragFlg & PragFlg_ReadOnly) == 0) {
-				/* Write the specified cookie value */
-				static const VdbeOpList setCookie[] = {
-					{OP_SetCookie, 0, 0, 0},	/* 1 */
-				};
-				VdbeOp *aOp;
-				sqlite3VdbeVerifyNoMallocRequired(v,
-								  ArraySize
-								  (setCookie));
-				aOp =
-				    sqlite3VdbeAddOpList(v,
-							 ArraySize(setCookie),
-							 setCookie, 0);
-				if (ONLY_IF_REALLOC_STRESS(aOp == 0))
-					break;
-				aOp[0].p1 = 0;
-				aOp[0].p2 = iCookie;
-				aOp[0].p3 = sqlite3Atoi(zRight);
-			} else {
-				/* Read the specified cookie value */
-				static const VdbeOpList readCookie[] = {
-					{OP_ReadCookie, 0, 1, 0},	/* 1 */
-					{OP_ResultRow, 1, 1, 0}
-				};
-				VdbeOp *aOp;
-				sqlite3VdbeVerifyNoMallocRequired(v,
-								  ArraySize
-								  (readCookie));
-				aOp =
-				    sqlite3VdbeAddOpList(v,
-							 ArraySize(readCookie),
-							 readCookie, 0);
-				if (ONLY_IF_REALLOC_STRESS(aOp == 0))
-					break;
-				aOp[0].p1 = 0;
-				aOp[1].p1 = 0;
-				aOp[1].p3 = iCookie;
-				sqlite3VdbeReusable(v);
-			}
-			break;
-		}
-		case PragTyp_DEFAULT_ENGINE: {
-			if (sql_default_engine_set(zRight) != 0) {
-				pParse->rc = SQL_TARANTOOL_ERROR;
-				pParse->nErr++;
-				goto pragma_out;
-			}
-			sqlite3VdbeAddOp0(v, OP_Expire);
-			break;
+	case PragTyp_DEFAULT_ENGINE: {
+		if (sql_default_engine_set(zRight) != 0) {
+			pParse->rc = SQL_TARANTOOL_ERROR;
+			pParse->nErr++;
+			goto pragma_out;
 		}
+		sqlite3VdbeAddOp0(v, OP_Expire);
+		break;
+	}
 
-		/* *   PRAGMA busy_timeout *   PRAGMA busy_timeout = N *
-		 *
-		 * Call sqlite3_busy_timeout(db, N).  Return the current
-		 * timeout value * if one is set.  If no busy handler
-		 * or a different busy handler is set * then 0 is
-		 * returned.  Setting the busy_timeout to 0 or negative *
-		 * disables the timeout.
-		 */
-		/* case PragTyp_BUSY_TIMEOUT */
+	/* *   PRAGMA busy_timeout *   PRAGMA busy_timeout = N *
+	 *
+	 * Call sqlite3_busy_timeout(db, N).  Return the current
+	 * timeout value * if one is set.  If no busy handler
+	 * or a different busy handler is set * then 0 is
+	 * returned.  Setting the busy_timeout to 0 or negative *
+	 * disables the timeout.
+	 */
+	/* case PragTyp_BUSY_TIMEOUT */
 	default:{
 			assert(pPragma->ePragTyp == PragTyp_BUSY_TIMEOUT);
 			if (zRight) {
@@ -945,5 +858,4 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 	sqlite3DbFree(db, zTable);
 }
 
-#endif				/* SQLITE_OMIT_PRAGMA */
 #endif
diff --git a/src/box/sql/pragma.h b/src/box/sql/pragma.h
index 795c98c..6771749 100644
--- a/src/box/sql/pragma.h
+++ b/src/box/sql/pragma.h
@@ -5,7 +5,6 @@
  */
 
 /* The various pragma types */
-#define PragTyp_HEADER_VALUE                   0
 #define PragTyp_BUSY_TIMEOUT                   1
 #define PragTyp_CASE_SENSITIVE_LIKE            2
 #define PragTyp_COLLATION_LIST                 3
@@ -23,7 +22,6 @@
 #define PragFlg_NeedSchema 0x01	/* Force schema load before running */
 #define PragFlg_NoColumns  0x02	/* OP_ResultRow called with zero columns */
 #define PragFlg_NoColumns1 0x04	/* zero columns if RHS argument is present */
-#define PragFlg_ReadOnly   0x08	/* Read-only HEADER_VALUE */
 #define PragFlg_Result0    0x10	/* Acts as query when no argument */
 #define PragFlg_Result1    0x20	/* Acts as query when has one argument */
 #define PragFlg_SchemaOpt  0x40	/* Schema restricts name search if present */
@@ -224,16 +222,6 @@ static const PragmaName aPragmaName[] = {
 	 /* ColNames:  */ 0, 0,
 	 /* iArg:      */ SQLITE_ReverseOrder},
 #endif
-#if !defined(SQLITE_OMIT_SCHEMA_VERSION_PRAGMAS)
-	{ /* zName:     */ "schema_version",
-	 /* ePragTyp:  */ PragTyp_HEADER_VALUE,
-	 /* ePragFlg:  */ PragFlg_NoColumns1 | PragFlg_Result0,
-	 /* ColNames:  */ 0, 0,
-	  /* Tarantool: need to take schema version from
-	   * backend.
-	   */
-	 /* iArg:      */ 0},
-#endif
 #if !defined(SQLITE_OMIT_FLAG_PRAGMAS) && defined(SQLITE_ENABLE_SELECTTRACE)
 	{ /* zName:     */ "select_trace",
 	/* ePragTyp:  */ PragTyp_FLAG,
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index ec0bc98..46f7c31 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -260,7 +260,6 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
 		sqlite3VdbeAddOp1(v, OP_Close, cursor);
 
 		sql_set_multi_write(parse, false);
-		sqlite3ChangeCookie(parse);
 	} else {
 		parse->parsed_ast.trigger = trigger;
 		parse->parsed_ast_type = AST_TYPE_TRIGGER;
@@ -450,7 +449,6 @@ vdbe_code_drop_trigger(struct Parse *parser, const char *trigger_name,
 			  record_to_delete);
 	if (account_changes)
 		sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
-	sqlite3ChangeCookie(parser);
 }
 
 void
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 2144d95..d8ddb3a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3076,53 +3076,10 @@ case OP_TTransaction: {
 	break;
 }
 
-/* Opcode: ReadCookie P1 P2 P3 * *
- *
- * Read cookie number P3 from database P1 and write it into register P2.
- * P3==1 is the schema version.  P3==2 is the database format.
- * P3==3 is the recommended pager cache size, and so forth.  P1==0 is
- * the main database file and P1==1 is the database file used to store
- * temporary tables.
- *
- * There must be a read-lock on the database (either a transaction
- * must be started or there must be an open cursor) before
- * executing this instruction.
- */
-case OP_ReadCookie: {               /* out2 */
-	pOut = out2Prerelease(p, pOp);
-	pOut->u.i = 0;
-	break;
-}
-
-/* Opcode: SetCookie P1 P2 P3 * *
- *
- * Write the integer value P3 into the schema version.
- * P2==3 is the recommended pager cache
- * size, and so forth.  P1==0 is the main database file and P1==1 is the
- * database file used to store temporary tables.
- *
- * A transaction must be started before executing this opcode.
- */
-case OP_SetCookie: {
-	assert(pOp->p1==0);
-	/* See note about index shifting on OP_ReadCookie */
-	/* When the schema cookie changes, record the new cookie internally */
-	user_session->sql_flags |= SQLITE_InternChanges;
-	if (pOp->p1==1) {
-		/* Invalidate all prepared statements whenever the TEMP database
-		 * schema is changed.  Ticket #1644
-		 */
-		sqlite3ExpirePreparedStatements(db);
-		p->expired = 0;
-	}
-	if (rc) goto abort_due_to_error;
-	break;
-}
-
 /* Opcode: OpenRead P1 P2 P3 P4 P5
  * Synopsis: index id = P2, space ptr = P4
  *
- * Open a cursor for a space specified by pointer in P4 and index
+\ * Open a cursor for a space specified by pointer in P4 and index
  * id in P2. Give the new cursor an identifier of P1. The P1
  * values need not be contiguous but all P1 values should be
  * small integers. It is an error for P1 to be negative.
-- 
2.16.2

[tarantool-patches] Re: [PATCH] sql: remove pragma schema_version

[Kirill Yukhin]

On 31 июл 13:54, Kirill Yukhin wrote:
> This pragma is dead and produces nothing else but segfault.
> Along w/ this pragma, remove now dead opcodes which set/read
> schema_version and all related routines.
> Also, improve opcode generation script.
> 
> Part of #3541
> ---
> Branch: https://github.com/tarantool/tarantool/commits/kyukhin/gh-3541-remove-schema-version-pragma
> Issue: https://github.com/tarantool/tarantool/issues/3541
Removed junk

modified   src/box/sql/vdbe.c
modified   src/box/sql/vdbe.c
@@ -3079,7 +3079,7 @@ case OP_TTransaction: {
 /* Opcode: OpenRead P1 P2 P3 P4 P5
  * Synopsis: index id = P2, space ptr = P4
  *
-\ * Open a cursor for a space specified by pointer in P4 and index
+ * Open a cursor for a space specified by pointer in P4 and index
  * id in P2. Give the new cursor an identifier of P1. The P1
  * values need not be contiguous but all P1 values should be
  * small integers. It is an error for P1 to be negative.

--
Regards, Kirill Yukhin

[tarantool-patches] Re: [PATCH] sql: remove pragma schema_version

[Vladislav Shpilevoy]

Hi! Thanks for the patch! I've pushed my fixes as a separate
commit on the branch.

[tarantool-patches] Re: [PATCH] sql: remove pragma schema_version

[Kirill Yukhin]

Hello Vlad,
On 31 июл 15:33, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch! I've pushed my fixes as a separate
> commit on the branch.

Thanks, I've squashed your changes.
Also, I've squashed into main patch this extra:

diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index d846fbd..91b6515 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -111,10 +111,7 @@ sqlite3AlterRenameTable(Parse * pParse,	/* Parser context. */
 				pTab->def->name);
 		goto exit_rename_table;
 	}
-	/* Begin a transaction for database.
-	 * Then modify the schema cookie (since the ALTER TABLE modifies the
-	 * schema).
-	 */
+	/* Begin a transaction for database. */
 	v = sqlite3GetVdbe(pParse);
 	if (v == 0) {
 		goto exit_rename_table;
@@ -304,7 +301,7 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
 	pNew->addColOffset = pTab->addColOffset;
 	pNew->nTabRef = 1;
 
-	/* Begin a transaction and increment the schema cookie.  */
+	/* Begin a transaction. */
 	sql_set_multi_write(pParse, false);
 	v = sqlite3GetVdbe(pParse);
 	if (!v)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index dcbeeb0..d812eb9 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -179,10 +179,8 @@ sqlite3LocateTable(Parse * pParse,	/* context in which to report errors */
 	if (p == NULL) {
 		const char *zMsg =
 		    flags & LOCATE_VIEW ? "no such view" : "no such table";
-		if ((flags & LOCATE_NOERR) == 0) {
+		if ((flags & LOCATE_NOERR) == 0)
 			sqlite3ErrorMsg(pParse, "%s: %s", zMsg, zName);
-			pParse->checkSchema = 1;
-		}
 	}
 
 	return p;
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 432e003..edd0c31 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -469,7 +469,6 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 				sqlite3ErrorMsg(pParse,
 						"table %S has no column named %s",
 						pTabList, 0, pColumn->a[i].zName);
-				pParse->checkSchema = 1;
 				goto insert_cleanup;
 			}
 			if (bit_test(used_columns, j)) {
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index a4767f2..280ecd9 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -434,7 +434,6 @@ lookupName(Parse * pParse,	/* The parsing context */
 		} else {
 			sqlite3ErrorMsg(pParse, "%s: %s", zErr, zCol);
 		}
-		pParse->checkSchema = 1;
 		pTopNC->nErr++;
 	}
 
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index d060a46..d824b8b 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -4385,7 +4385,6 @@ sqlite3IndexedByLookup(Parse * pParse, struct SrcList_item *pFrom)
 		if (!pIdx) {
 			sqlite3ErrorMsg(pParse, "no such index: %s", zIndexedBy,
 					0);
-			pParse->checkSchema = 1;
 			return SQLITE_ERROR;
 		}
 		pFrom->pIBIndex = pIdx;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 8b0b4b7..468b6c6 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2824,7 +2824,6 @@ struct Parse {
 	Vdbe *pVdbe;		/* An engine for executing database bytecode */
 	int rc;			/* Return code from execution */
 	u8 colNamesSet;		/* TRUE after OP_ColumnName has been issued to pVdbe */
-	u8 checkSchema;		/* Causes schema cookie check after an error */
 	u8 nTempReg;		/* Number of temporary registers in aTempReg[] */
 	u8 isMultiWrite;	/* True if statement may modify/insert multiple rows */
 	u8 mayAbort;		/* True if statement may throw an ABORT exception */
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 489a456..a2f6c73 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -217,7 +217,6 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 		if (j >= (int)def->field_count) {
 			sqlite3ErrorMsg(pParse, "no such column: %s",
 					pChanges->a[i].zName);
-			pParse->checkSchema = 1;
 			goto update_cleanup;
 		}
 	}


--
Regards, Kirill Yukhin

[tarantool-patches] Re: [PATCH] sql: remove pragma schema_version

[Kirill Yukhin]

Hello,
On 31 июл 13:54, Kirill Yukhin wrote:
> This pragma is dead and produces nothing else but segfault.
> Along w/ this pragma, remove now dead opcodes which set/read
> schema_version and all related routines.
> Also, improve opcode generation script.
> 
> Part of #3541
> ---
> Branch: https://github.com/tarantool/tarantool/commits/kyukhin/gh-3541-remove-schema-version-pragma
> Issue: https://github.com/tarantool/tarantool/issues/3541
I've checked the patch into 2.0 branch.

--
Regards, Kirill Yukhin