[tarantool-patches] Re: [PATCH v1 1/1] sql: decrease SELECT_COMPOUND_LIMIT threshold

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> Patch is almost OK, see several minor nitpicks.
Kirill, let's merge it?

> Hard limit 500 seems to be too much. Lets set hard limit 50 (as it was before).
> If user want to set more than 50, he can set it to 0 (according to comments
> it means no limit at all).
Ok, set 50.

> Ok, you may skip it, but anyway, I would add tests where you set
> limit to 0 and process compound select with 31 parts.
Introduced such test.

> You forgot to fix commit message. See previous review comments.
Updated.

> This pragma status is not displayed via simple ‘pragma’.
It is ok, the other non-boolean pragmas have same behavior. 
Displaying sql_default_engine was a big mistake. I've just implemented requirement
from review comment that time, but it was not correct. It looks out-of-order,
as an exception in code there.

> Moreover, you forgot to fix comment (30 -> 50).
> Nit: l would get rid of SQLITE prefix and use simple SQL instead.
> Or, at least, use SQL prefix for new SQLITE_DEFAULT_COMPOUND_SELECT macros.
Not a problem. Complete. SQL_ for SQL_MAX_COMPOUND_SELECT, SQL_LIMIT_COMPOUND_SELECT, SQL_DEFAULT_COMPOUND_SELECT.
I've also refactored comments that contain such macros.
 ===========================================================
Decreased number of default compound SELECTs to 30 to
prevent stack overflow on most clang builds.
Introduced new pragma sql_compound_select_limit to configure
this option on fly.

Closes #3382.

@TarantoolBot document
Title: new pragma sql_compound_select_limit
Now it is allowed to manually set maximum count of compound
selects. Default value is 30. Maximum is 500.
Processing requests with great amount of compound selects with
custom limit may cause stack overflow.
Setting sql_compound_select_limit at 0 disables this limit at
all.
Example:
\set language sql
pragma sql_compound_select_limit=20
---
 src/box/sql/main.c                                 | 11 ++--
 src/box/sql/parse.y                                | 16 +++---
 src/box/sql/pragma.c                               | 11 ++++
 src/box/sql/pragma.h                               |  6 +++
 src/box/sql/select.c                               | 20 ++++---
 src/box/sql/sqliteInt.h                            | 13 ++++-
 src/box/sql/sqliteLimit.h                          |  8 +--
 test/sql-tap/gh2548-select-compound-limit.test.lua | 62 ++++++++++++++++++++--
 test/sql-tap/select7.test.lua                      | 16 +++---
 test/sql-tap/selectG.test.lua                      |  2 +-
 10 files changed, 126 insertions(+), 39 deletions(-)

diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 69b2fec..7c15cef 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1426,7 +1426,7 @@ static const int aHardLimit[] = {
 	SQLITE_MAX_SQL_LENGTH,
 	SQLITE_MAX_COLUMN,
 	SQLITE_MAX_EXPR_DEPTH,
-	SQLITE_MAX_COMPOUND_SELECT,
+	SQL_MAX_COMPOUND_SELECT,
 	SQLITE_MAX_VDBE_OP,
 	SQLITE_MAX_FUNCTION_ARG,
 	SQLITE_MAX_ATTACHED,
@@ -1447,8 +1447,8 @@ static const int aHardLimit[] = {
 #if SQLITE_MAX_SQL_LENGTH>SQLITE_MAX_LENGTH
 #error SQLITE_MAX_SQL_LENGTH must not be greater than SQLITE_MAX_LENGTH
 #endif
-#if SQLITE_MAX_COMPOUND_SELECT<2
-#error SQLITE_MAX_COMPOUND_SELECT must be at least 2
+#if SQL_MAX_COMPOUND_SELECT<2
+#error SQL_MAX_COMPOUND_SELECT must be at least 2
 #endif
 #if SQLITE_MAX_VDBE_OP<40
 #error SQLITE_MAX_VDBE_OP must be at least 40
@@ -1503,8 +1503,8 @@ sqlite3_limit(sqlite3 * db, int limitId, int newLimit)
 	assert(aHardLimit[SQLITE_LIMIT_SQL_LENGTH] == SQLITE_MAX_SQL_LENGTH);
 	assert(aHardLimit[SQLITE_LIMIT_COLUMN] == SQLITE_MAX_COLUMN);
 	assert(aHardLimit[SQLITE_LIMIT_EXPR_DEPTH] == SQLITE_MAX_EXPR_DEPTH);
-	assert(aHardLimit[SQLITE_LIMIT_COMPOUND_SELECT] ==
-	       SQLITE_MAX_COMPOUND_SELECT);
+	assert(aHardLimit[SQL_LIMIT_COMPOUND_SELECT] ==
+	       SQL_MAX_COMPOUND_SELECT);
 	assert(aHardLimit[SQLITE_LIMIT_VDBE_OP] == SQLITE_MAX_VDBE_OP);
 	assert(aHardLimit[SQLITE_LIMIT_FUNCTION_ARG] ==
 	       SQLITE_MAX_FUNCTION_ARG);
@@ -1856,6 +1856,7 @@ sql_init_db(sqlite3 **out_db)
 	assert(sizeof(db->aLimit) == sizeof(aHardLimit));
 	memcpy(db->aLimit, aHardLimit, sizeof(db->aLimit));
 	db->aLimit[SQLITE_LIMIT_WORKER_THREADS] = SQLITE_DEFAULT_WORKER_THREADS;
+	db->aLimit[SQL_LIMIT_COMPOUND_SELECT] = SQL_DEFAULT_COMPOUND_SELECT;
 	db->szMmap = sqlite3GlobalConfig.szMmap;
 	db->nMaxSorterMmap = 0x7FFFFFFF;
 
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index d8532d3..245f656 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -406,11 +406,11 @@ cmd ::= select(X).  {
 %destructor oneselect {sql_select_delete(pParse->db, $$);}
 
 %include {
-  /*
-  ** For a compound SELECT statement, make sure p->pPrior->pNext==p for
-  ** all elements in the list.  And make sure list length does not exceed
-  ** SQLITE_LIMIT_COMPOUND_SELECT.
-  */
+  /**
+   * For a compound SELECT statement, make sure
+   * p->pPrior->pNext==p for all elements in the list. And make
+   * sure list length does not exceed SQL_LIMIT_COMPOUND_SELECT.
+   */
   static void parserDoubleLinkSelect(Parse *pParse, Select *p){
     if( p->pPrior ){
       Select *pNext = 0, *pLoop;
@@ -420,10 +420,12 @@ cmd ::= select(X).  {
         pLoop->selFlags |= SF_Compound;
       }
       if( (p->selFlags & SF_MultiValue)==0 && 
-        (mxSelect = pParse->db->aLimit[SQLITE_LIMIT_COMPOUND_SELECT])>0 &&
+        (mxSelect = pParse->db->aLimit[SQL_LIMIT_COMPOUND_SELECT])>0 &&
         cnt>mxSelect
       ){
-        sqlite3ErrorMsg(pParse, "Too many UNION or EXCEPT or INTERSECT operations");
+        sqlite3ErrorMsg(pParse, "Too many UNION or EXCEPT or INTERSECT "
+                        "operations (limit %d is set)",
+                        pParse->db->aLimit[SQL_LIMIT_COMPOUND_SELECT]);
       }
     }
   }
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 9885071..060b02e 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -607,6 +607,17 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 		break;
 	}
 
+	case PragTyp_COMPOUND_SELECT_LIMIT: {
+		if (zRight != NULL) {
+			sqlite3_limit(db, SQL_LIMIT_COMPOUND_SELECT,
+				      sqlite3Atoi(zRight));
+		}
+		int retval =
+			sqlite3_limit(db, SQL_LIMIT_COMPOUND_SELECT, -1);
+		returnSingleInt(v, retval);
+		break;
+	}
+
 	/* *   PRAGMA busy_timeout *   PRAGMA busy_timeout = N *
 	 *
 	 * Call sqlite3_busy_timeout(db, N).  Return the current
diff --git a/src/box/sql/pragma.h b/src/box/sql/pragma.h
index ecc9ee8..e339160 100644
--- a/src/box/sql/pragma.h
+++ b/src/box/sql/pragma.h
@@ -16,6 +16,7 @@
 #define PragTyp_TABLE_INFO                    17
 #define PragTyp_PARSER_TRACE                  24
 #define PragTyp_DEFAULT_ENGINE                25
+#define PragTyp_COMPOUND_SELECT_LIMIT         26
 
 /* Property flags associated with various pragma. */
 #define PragFlg_NeedSchema 0x01	/* Force schema load before running */
@@ -221,6 +222,11 @@ static const PragmaName aPragmaName[] = {
 	 /* ColNames:  */ 0, 0,
 	 /* iArg:      */ SQLITE_ShortColNames},
 #endif
+	{ /* zName:     */ "sql_compound_select_limit",
+	/* ePragTyp:  */ PragTyp_COMPOUND_SELECT_LIMIT,
+	/* ePragFlg:  */ PragFlg_Result0,
+	/* ColNames:  */ 0, 0,
+	/* iArg:      */ 0},
 	{ /* zName:     */ "sql_default_engine",
 	 /* ePragTyp:  */ PragTyp_DEFAULT_ENGINE,
 	 /* ePragFlg:  */ PragFlg_Result0 | PragFlg_NoColumns1,
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 849c0f8..89c574c 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2434,21 +2434,25 @@ static int multiSelectOrderBy(Parse * pParse,	/* Parsing context */
 			      SelectDest * pDest	/* What to do with query results */
     );
 
-/*
- * Handle the special case of a compound-select that originates from a
- * VALUES clause.  By handling this as a special case, we avoid deep
- * recursion, and thus do not need to enforce the SQLITE_LIMIT_COMPOUND_SELECT
- * on a VALUES clause.
+/**
+ * Handle the special case of a compound-select that originates
+ * from a VALUES clause.  By handling this as a special case, we
+ * avoid deep recursion, and thus do not need to enforce the
+ * SQL_LIMIT_COMPOUND_SELECT on a VALUES clause.
  *
  * Because the Select object originates from a VALUES clause:
  *   (1) It has no LIMIT or OFFSET
  *   (2) All terms are UNION ALL
  *   (3) There is no ORDER BY clause
+ *
+ * @param pParse Parsing context.
+ * @param p The right-most of SELECTs to be coded.
+ * @param pDest What to do with query results.
+ * @retval 0 On success, not 0 elsewhere.
  */
 static int
-multiSelectValues(Parse * pParse,	/* Parsing context */
-		  Select * p,		/* The right-most of SELECTs to be coded */
-		  SelectDest * pDest)	/* What to do with query results */
+multiSelectValues(struct Parse *pParse, struct Select *p,
+		  struct SelectDest *pDest)
 {
 	Select *pPrior;
 	int nRow = 1;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 1d32c9a..09e8b49 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -357,7 +357,7 @@ struct sqlite3_vfs {
 #define SQLITE_LIMIT_SQL_LENGTH                1
 #define SQLITE_LIMIT_COLUMN                    2
 #define SQLITE_LIMIT_EXPR_DEPTH                3
-#define SQLITE_LIMIT_COMPOUND_SELECT           4
+#define SQL_LIMIT_COMPOUND_SELECT              4
 #define SQLITE_LIMIT_VDBE_OP                   5
 #define SQLITE_LIMIT_FUNCTION_ARG              6
 #define SQLITE_LIMIT_ATTACHED                  7
@@ -1031,6 +1031,17 @@ sqlite3_bind_parameter_lindex(sqlite3_stmt * pStmt, const char *zName,
 #define SQLITE_MAX_WORKER_THREADS SQLITE_DEFAULT_WORKER_THREADS
 #endif
 
+/**
+ * Default count of allowed compound selects.
+ *
+ * Tarantool: gh-2548, gh-3382: Fiber stack is 64KB by default,
+ * so maximum number of entities should be less than 30 or stack
+ * guard will be triggered (triggered with clang).
+*/
+#ifndef SQL_DEFAULT_COMPOUND_SELECT
+#define SQL_DEFAULT_COMPOUND_SELECT 30
+#endif
+
 /*
  * The default initial allocation for the pagecache when using separate
  * pagecaches for each database connection.  A positive number is the
diff --git a/src/box/sql/sqliteLimit.h b/src/box/sql/sqliteLimit.h
index b88c9c6..d9278bd 100644
--- a/src/box/sql/sqliteLimit.h
+++ b/src/box/sql/sqliteLimit.h
@@ -116,13 +116,9 @@ enum {
  * if the number of terms is too large.  In practice, most SQL
  * never has more than 3 or 4 terms.  Use a value of 0 to disable
  * any limit on the number of terms in a compount SELECT.
- *
- * Tarantool: gh-2548: Fiber stack is 64KB by default, so maximum
- * number of entities should be less than 50 or stack guard will be
- * triggered.
  */
-#ifndef SQLITE_MAX_COMPOUND_SELECT
-#define SQLITE_MAX_COMPOUND_SELECT 50
+#ifndef SQL_MAX_COMPOUND_SELECT
+#define SQL_MAX_COMPOUND_SELECT 50
 #endif
 
 /*
diff --git a/test/sql-tap/gh2548-select-compound-limit.test.lua b/test/sql-tap/gh2548-select-compound-limit.test.lua
index 9cfc066..5494a66 100755
--- a/test/sql-tap/gh2548-select-compound-limit.test.lua
+++ b/test/sql-tap/gh2548-select-compound-limit.test.lua
@@ -1,12 +1,14 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(8)
+test:plan(14)
 
 -- box.cfg{wal_mode='none'}
 
-table_count = 51
+table_count = 31
 
-for _, term in ipairs({'UNION', 'UNION ALL', 'INTERSECT', 'EXCEPT'}) do 
+select_string_last = ''
+
+for _, term in ipairs({'UNION', 'UNION ALL', 'INTERSECT', 'EXCEPT'}) do
     select_string = ''
     test:do_test("Positive COMPOUND "..term,
                  function()
@@ -39,6 +41,8 @@ for _, term in ipairs({'UNION', 'UNION ALL', 'INTERSECT', 'EXCEPT'}) do
                  end,
                  false)
 
+    select_string_last = select_string
+
 --    if not pcall(function() box.sql.execute(select_string) end) then
 --        print('not ok')
 --    end
@@ -49,4 +53,56 @@ for _, term in ipairs({'UNION', 'UNION ALL', 'INTERSECT', 'EXCEPT'}) do
 --    end
 end
 
+
+test:do_catchsql_test(
+    "gh2548-select-compound-limit-2",
+    select_string_last, {
+        -- <gh2548-select-compound-limit-2>
+        1, "Too many UNION or EXCEPT or INTERSECT operations (limit 30 is set)"
+        -- </gh2548-select-compound-limit-2>
+    })
+
+test:do_execsql_test(
+    "gh2548-select-compound-limit-3.1", [[
+        pragma sql_compound_select_limit
+    ]], {
+        -- <gh2548-select-compound-limit-3.1>
+        30
+        -- </gh2548-select-compound-limit-3.1>
+    })
+
+test:do_execsql_test(
+    "gh2548-select-compound-limit-3.2", [[
+        pragma sql_compound_select_limit=31
+    ]], {
+        -- <gh2548-select-compound-limit-3.2>
+        31
+        -- </gh2548-select-compound-limit-3.2>
+})
+
+test:do_execsql_test(
+    "gh2548-select-compound-limit-3.3",
+    select_string_last, {
+        -- <gh2548-select-compound-limit-3.3>
+        0, 1
+        -- </gh2548-select-compound-limit-3.3>
+    })
+
+test:do_execsql_test(
+    "gh2548-select-compound-limit-3.4", [[
+        pragma sql_compound_select_limit=0
+    ]], {
+        -- <gh2548-select-compound-limit-3.4>
+        0
+        -- </gh2548-select-compound-limit-3.4>
+    })
+
+test:do_execsql_test(
+    "gh2548-select-compound-limit-3.3",
+    select_string_last, {
+        -- <gh2548-select-compound-limit-3.3>
+        0, 1
+        -- </gh2548-select-compound-limit-3.3>
+    })
+
 test:finish_test()
diff --git a/test/sql-tap/select7.test.lua b/test/sql-tap/select7.test.lua
index 10e13e2..acdc123 100755
--- a/test/sql-tap/select7.test.lua
+++ b/test/sql-tap/select7.test.lua
@@ -169,17 +169,17 @@ test:do_catchsql_test(
 -- compound select statement.
 
 -- hardcoded define from src
--- 500 is default value
-local SQLITE_MAX_COMPOUND_SELECT = 500
+-- 30 is default value
+local SQL_MAX_COMPOUND_SELECT = 30
 sql = "SELECT 0"
-for i = 0, SQLITE_MAX_COMPOUND_SELECT + 1, 1 do
+for i = 0, SQL_MAX_COMPOUND_SELECT + 1, 1 do
     sql = sql .. " UNION ALL SELECT "..i..""
 end
 test:do_catchsql_test(
     "select7-6.2",
     sql, {
         -- <select7-6.2>
-        1, "Too many UNION or EXCEPT or INTERSECT operations"
+        1, "Too many UNION or EXCEPT or INTERSECT operations (limit 30 is set)"
         -- </select7-6.2>
     })
 
@@ -193,7 +193,7 @@ test:do_execsql_test(
         INSERT INTO t3 VALUES(56.0);
     ]], {
         -- <select7-7.1>
-        
+
         -- </select7-7.1>
     })
 
@@ -216,7 +216,7 @@ test:do_execsql_test(
         INSERT INTO t4 VALUES( 3.0 );
     ]], {
         -- <select7-7.3>
-        
+
         -- </select7-7.3>
     })
 
@@ -233,7 +233,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "select7-7.5",
     [[
-        SELECT a=0, typeof(a) FROM t4 
+        SELECT a=0, typeof(a) FROM t4
     ]], {
         -- <select7-7.5>
         0, "real", 0, "real"
@@ -243,7 +243,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "select7-7.6",
     [[
-        SELECT a=0, typeof(a) FROM t4 GROUP BY a 
+        SELECT a=0, typeof(a) FROM t4 GROUP BY a
     ]], {
         -- <select7-7.6>
         0, "real", 0, "real"
diff --git a/test/sql-tap/selectG.test.lua b/test/sql-tap/selectG.test.lua
index d83b790..c9ee7de 100755
--- a/test/sql-tap/selectG.test.lua
+++ b/test/sql-tap/selectG.test.lua
@@ -15,7 +15,7 @@ test:plan(1)
 -------------------------------------------------------------------------
 --
 -- This file verifies that INSERT operations with a very large number of
--- VALUE terms works and does not hit the SQLITE_LIMIT_COMPOUND_SELECT limit.
+-- VALUE terms works and does not hit the SQL_LIMIT_COMPOUND_SELECT limit.
 --
 -- ["set","testdir",[["file","dirname",["argv0"]]]]
 -- ["source",[["testdir"],"\/tester.tcl"]]
-- 
2.7.4