Re: [Tarantool-patches] [PATCH v5 4/5] sql: remove control pragmas

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

I've pushed my review fixes on top of this commit. See it below
and on the branch. If you agree, then squash. Otherwise lets
discuss.

================================================================================

commit 66b927c57c84b49ef2244efb49c0986928fb8e2b
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Fri Dec 27 17:32:35 2019 +0300

    Review fixes 4/5

diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d859d6380..f4d6e52cd 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3771,7 +3771,6 @@ sql_space_def_check_format(const struct space_def *space_def);
 void sqlDetach(Parse *, Expr *);
 int sqlAtoF(const char *z, double *, int);
 int sqlGetInt32(const char *, int *);
-int sqlAtoi(const char *);
 
 /**
  * Return number of symbols in the given string.
@@ -3959,8 +3958,6 @@ int
 sql_rem_int(int64_t lhs, bool is_lhs_neg, int64_t rhs, bool is_rhs_neg,
 	    int64_t *res, bool *is_res_neg);
 
-u8 sqlGetBoolean(const char *z, u8);
-
 const void *sqlValueText(sql_value *);
 int sqlValueBytes(sql_value *);
 void sqlValueSetStr(sql_value *, int, const void *,
diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index 0e115accf..f908e9ced 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -495,8 +495,6 @@ sql_atoi64(const char *z, int64_t *val, bool *is_neg, int length)
  * This routine accepts both decimal and hexadecimal notation for integers.
  *
  * Any non-numeric characters that following zNum are ignored.
- * This is different from sqlAtoi64() which requires the
- * input number to be zero-terminated.
  */
 int
 sqlGetInt32(const char *zNum, int *pValue)
@@ -553,19 +551,6 @@ sqlGetInt32(const char *zNum, int *pValue)
 	return 1;
 }
 
-/*
- * Return a 32-bit integer value extracted from a string.  If the
- * string is not an integer, just return 0.
- */
-int
-sqlAtoi(const char *z)
-{
-	int x = 0;
-	if (z)
-		sqlGetInt32(z, &x);
-	return x;
-}
-
 /*
  * The variable-length integer encoding is as follows:
  *
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 8e12c37a0..c5c0a9790 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4778,8 +4778,8 @@ case OP_Program: {        /* jump */
 
 	/* If the p5 flag is clear, then recursive invocation of triggers is
 	 * disabled for backwards compatibility (p5 is set if this sub-program
-	 * is really a trigger, not a foreign key action, and the flag set
-	 * and cleared by the "PRAGMA recursive_triggers" command is clear).
+	 * is really a trigger, not a foreign key action, and the setting
+	 * 'recursive_triggers' is not set).
 	 *
 	 * It is recursive invocation of triggers, at the SQL level, that is
 	 * disabled. In some cases a single trigger may generate more than one
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 1e585d89d..0f2703091 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -199,11 +199,6 @@ int sqlVdbeAddOp4(Vdbe *, int, int, int, int, const char *zP4, int);
 int sqlVdbeAddOp4Dup8(Vdbe *, int, int, int, int, const u8 *, int);
 int sqlVdbeAddOp4Int(Vdbe *, int, int, int, int, int);
 void sqlVdbeEndCoroutine(Vdbe *, int);
-#if defined(SQL_DEBUG) && !defined(SQL_TEST_REALLOC_STRESS)
-void sqlVdbeVerifyNoResultRow(Vdbe * p);
-#else
-#define sqlVdbeVerifyNoResultRow(A)
-#endif
 void sqlVdbeChangeOpcode(Vdbe *, u32 addr, u8);
 void sqlVdbeChangeP1(Vdbe *, u32 addr, int P1);
 void sqlVdbeChangeP2(Vdbe *, u32 addr, int P2);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 5b4bc0182..43e0566bb 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -533,24 +533,6 @@ sqlVdbeCurrentAddr(Vdbe * p)
 	return p->nOp;
 }
 
-/*
- * Verify that the VM passed as the only argument does not contain
- * an OP_ResultRow opcode. Fail an assert() if it does. This is used
- * by code in pragma.c to ensure that the implementation of certain
- * pragmas comports with the flags specified in the mkpragmatab.tcl
- * script.
- */
-#if defined(SQL_DEBUG) && !defined(SQL_TEST_REALLOC_STRESS)
-void
-sqlVdbeVerifyNoResultRow(Vdbe * p)
-{
-	int i;
-	for (i = 0; i < p->nOp; i++) {
-		assert(p->aOp[i].opcode != OP_ResultRow);
-	}
-}
-#endif
-
 /*
  * This function returns a pointer to the array of opcodes associated with
  * the Vdbe passed as the first argument. It is the callers responsibility
diff --git a/test/sql-tap/lua/sqltester.lua b/test/sql-tap/lua/sqltester.lua
index de5ef6b8b..9b0218e63 100644
--- a/test/sql-tap/lua/sqltester.lua
+++ b/test/sql-tap/lua/sqltester.lua
@@ -413,7 +413,7 @@ box.cfg{
 }
 
 local engine = test_run and test_run:get_cfg('engine') or 'memtx'
-_ = box.space._session_settings:update('sql_default_engine', {{'=', 2, engine}})
+box.space._session_settings:update('sql_default_engine', {{'=', 2, engine}})
 
 function test.engine(self)
     return engine
diff --git a/test/sql-tap/select1.test.lua b/test/sql-tap/select1.test.lua
index f309e1e73..bea5b9c7c 100755
--- a/test/sql-tap/select1.test.lua
+++ b/test/sql-tap/select1.test.lua
@@ -2,6 +2,12 @@
 test = require("sqltester")
 test:plan(173)
 
+function set_full_column_names(value)
+    box.space._session_settings:update('sql_full_column_names', {
+        {'=', 2, value}
+    })
+end
+
 --!./tcltestrunner.lua
 -- ["set","testdir",[["file","dirname",["argv0"]]]]
 -- ["source",[["testdir"],"\/tester.tcl"]]
@@ -916,7 +922,7 @@ test:do_catchsql2_test(
 test:do_test(
     "select1-6.1.1",
     function()
-        box.space._session_settings:update('sql_full_column_names', {{'=', 2, true}})
+        set_full_column_names(true)
         return test:catchsql2 "SELECT f1 FROM test1 ORDER BY f2"
     end, {
         -- <select1-6.1.1>
@@ -952,7 +958,7 @@ test:do_test(
             msg = test:execsql2 "SELECT DISTINCT * FROM test1 WHERE f1==11"
             end)
         v = v == true and {0} or {1} 
-        box.space._session_settings:update('sql_full_column_names', {{'=', 2, false}})
+        set_full_column_names(false)
         return table.insert(v,msg) or v
     end, {
         -- <select1-6.1.4>
@@ -1043,13 +1049,13 @@ test:do_catchsql2_test(
 test:do_test(
     "select1-6.5.1",
     function()
-        box.space._session_settings:update('sql_full_column_names', {{'=', 2, true}})
+        set_full_column_names(true)
         local msg
         v = pcall( function ()
                 msg = test:execsql2 "SELECT test1.f1+F2 FROM test1 ORDER BY f2"
             end)
         v = v == true and {0} or {1}
-        box.space._session_settings:update('sql_full_column_names', {{'=', 2, false}})
+        set_full_column_names(false)
         return table.insert(v,msg) or v
     end, {
         -- <select1-6.5.1>
@@ -1123,7 +1129,7 @@ test:do_catchsql2_test(
 test:do_test(
     "select1-6.9.3",
     function()
-        box.space._session_settings:update('sql_full_column_names', {{'=', 2, false}})
+        set_full_column_names(false)
         return test:execsql2 [[
             SELECT test1 . f1, test1 . f2 FROM test1 LIMIT 1
         ]]
@@ -1136,7 +1142,7 @@ test:do_test(
 test:do_test(
     "select1-6.9.4",
     function()
-        box.space._session_settings:update('sql_full_column_names', {{'=', 2, true}})
+        set_full_column_names(true)
         return test:execsql2 [[
             SELECT test1 . f1, test1 . f2 FROM test1 LIMIT 1
         ]]
@@ -1149,7 +1155,7 @@ test:do_test(
 test:do_test(
     "select1-6.9.5",
     function()
-        box.space._session_settings:update('sql_full_column_names', {{'=', 2, true}})
+        set_full_column_names(true)
         return test:execsql2 [[
             SELECT 123.45;
         ]]
@@ -1228,7 +1234,7 @@ test:do_execsql2_test(
 test:do_test(
     "select1-6.9.11",
     function()
-        box.space._session_settings:update('sql_full_column_names', {{'=', 2, true}})
+        set_full_column_names(true)
         return test:execsql2 [[
             SELECT a.f1, b.f2 FROM test1 a, test1 b LIMIT 1
         ]]
@@ -1251,7 +1257,7 @@ test:do_execsql2_test(
 test:do_test(
     "select1-6.9.13",
     function()
-        box.space._session_settings:update('sql_full_column_names', {{'=', 2, false}})
+        set_full_column_names(false)
         return test:execsql2 [[
             SELECT a.f1, b.f1 FROM test1 a, test1 b LIMIT 1
         ]]
@@ -1274,7 +1280,7 @@ test:do_execsql2_test(
 test:do_test(
     "select1-6.9.15",
     function()
-        box.space._session_settings:update('sql_full_column_names', {{'=', 2, true}})
+        set_full_column_names(true)
         return test:execsql2 [[
             SELECT a.f1, b.f1 FROM test1 a, test1 b LIMIT 1
         ]]
@@ -1294,7 +1300,7 @@ test:do_execsql2_test(
         -- </select1-6.9.16>
     })
 
-box.space._session_settings:update('sql_full_column_names', {{'=', 2, false}})
+set_full_column_names(false)
 test:do_catchsql2_test(
         "select1-6.10",
         [[
diff --git a/test/sql/transitive-transactions.result b/test/sql/transitive-transactions.result
index e5868d7b0..91c35a309 100644
--- a/test/sql/transitive-transactions.result
+++ b/test/sql/transitive-transactions.result
@@ -87,7 +87,7 @@ box.space.PARENT:select();
 ---
 - - [1, 1]
 ...
--- Make sure that 'PRAGMA defer_foreign_keys' works.
+-- Make sure that setting 'defer_foreign_keys' works.
 --
 box.execute('DROP TABLE child;')
 box.execute('CREATE TABLE child(id INT PRIMARY KEY, x INT REFERENCES parent(y))')
diff --git a/test/sql/transitive-transactions.test.lua b/test/sql/transitive-transactions.test.lua
index f4df716c3..5565de7fe 100644
--- a/test/sql/transitive-transactions.test.lua
+++ b/test/sql/transitive-transactions.test.lua
@@ -45,7 +45,7 @@ fk_violation_3();
 box.space.CHILD:select();
 box.space.PARENT:select();
 
--- Make sure that 'PRAGMA defer_foreign_keys' works.
+-- Make sure that setting 'defer_foreign_keys' works.
 --
 box.execute('DROP TABLE child;')
 box.execute('CREATE TABLE child(id INT PRIMARY KEY, x INT REFERENCES parent(y))')