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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> Why did you choose 30? AFAIU previous ’50’ also was taken as
> 'stack guard will not be triggered’ number.
> Is this just typical ‘less than 50 but not too much’?:)
> Have you checked this number on different compilers/OSs?
I've done everything same way than

commit 974bce9aeb169b3f8c08924d8afb95bedc4a6c53
Author: Kirill Yukhin <kirill.yukhin@gmail.com>
Date:   Thu Jun 29 21:07:41 2017 +0300

    sql: Limit number of terms in SELECT compoind stmts
    
    Right now SQL query compiler is run on top of fiber's stack,
    which is limited to 64KB by default, so maximum
    number of entities in compound SELECT statement should be less than
    50 (verified by experiment) or stack guard will be triggered.
    
    In future we'll introduce heuristic which should understand that
    query is 'complex' and run compilation in separate thread with larger
    stack. This will also allow us not to block Tarantool while compiling
    the query.
    
    Closes #2548

I've also carried experiment on with gcc(no proble even with 50) and on FreeBSD.

> Mb it is worth to add pragma in order to adjust number of max nested selects?
> I ask this because error message gives an illusion of such opportunity
> ("if limit is set, then I can change it”). Or change message to simple:
> (limit is %d). It is only request, I like this additional information btw.
> Also, another cool option would be <pragma options> or <pragma settings>.
> It would display all current settings and options: max number of nested selects,
> depth of recursion in triggers etc.
I don't mind pragma. Let's introduce it. I am going to return old hard limit 500(that was before Kirill's patch),
but would set 30 on initializing.

> 
>>       }
>>     }
>>   }
>> diff --git a/src/box/sql/sqliteLimit.h b/src/box/sql/sqliteLimit.h
>> index b88c9c6..0b8fc43 100644
>> --- a/src/box/sql/sqliteLimit.h
>> +++ b/src/box/sql/sqliteLimit.h
>> @@ -117,12 +117,12 @@ enum {
>>  * 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.
> 
> Is this comment relevant? I know that it is trapped here accidentally,
> but lets remove it in case it is outdated. 
It is not outdated with introducing new pragma.

> Do we have tests on this case?
It doesn't matter for this patch. Ticket is definitely not about it.

> 
>>  *
>> - * 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.
>> + * 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.
> 
> It is so ridiculous :)) Never say never
¯\_(ツ)_/¯

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

Decreased the maximum number of terms in a compound SELECT
statement. The code generator for compound SELECT statements
does one level of recursion for each term.  A stack overflow can
result if the number of terms is too large.  In practice, most
SQL never has more than 3 or 4 terms.
Fiber stack is 64KB by default, so maximum number of entities
should be about 30 to stack guard will not be triggered.

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                                 |  2 +
 src/box/sql/parse.y                                |  4 +-
 src/box/sql/pragma.c                               | 11 ++++++
 src/box/sql/pragma.h                               |  6 +++
 src/box/sql/sqliteInt.h                            |  5 +++
 src/box/sql/sqliteLimit.h                          |  8 ++--
 test/sql-tap/gh2548-select-compound-limit.test.lua | 43 +++++++++++++++++++++-
 test/sql-tap/select7.test.lua                      |  2 +-
 8 files changed, 73 insertions(+), 8 deletions(-)

diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 69b2fec..d30d6af 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1856,6 +1856,8 @@ 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[SQLITE_LIMIT_COMPOUND_SELECT] =
+		SQLITE_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..c5e90a7 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -423,7 +423,9 @@ cmd ::= select(X).  {
         (mxSelect = pParse->db->aLimit[SQLITE_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[SQLITE_LIMIT_COMPOUND_SELECT]);
       }
     }
   }
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 9885071..d382c59 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, SQLITE_LIMIT_COMPOUND_SELECT,
+				      sqlite3Atoi(zRight));
+		}
+		int retval =
+			sqlite3_limit(db, SQLITE_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/sqliteInt.h b/src/box/sql/sqliteInt.h
index 1d32c9a..a6cdab6 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1031,6 +1031,11 @@ 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. */
+#ifndef SQLITE_DEFAULT_COMPOUND_SELECT
+#define SQLITE_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..43b95c8 100644
--- a/src/box/sql/sqliteLimit.h
+++ b/src/box/sql/sqliteLimit.h
@@ -117,12 +117,12 @@ enum {
  * 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.
+ * 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.
  */
 #ifndef SQLITE_MAX_COMPOUND_SELECT
-#define SQLITE_MAX_COMPOUND_SELECT 50
+#define SQLITE_MAX_COMPOUND_SELECT 500
 #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..f9f43a1 100755
--- a/test/sql-tap/gh2548-select-compound-limit.test.lua
+++ b/test/sql-tap/gh2548-select-compound-limit.test.lua
@@ -1,10 +1,12 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(8)
+test:plan(12)
 
 -- box.cfg{wal_mode='none'}
 
-table_count = 51
+table_count = 31
+
+select_string_last = ''
 
 for _, term in ipairs({'UNION', 'UNION ALL', 'INTERSECT', 'EXCEPT'}) do 
     select_string = ''
@@ -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,39 @@ 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:finish_test()
diff --git a/test/sql-tap/select7.test.lua b/test/sql-tap/select7.test.lua
index 10e13e2..59465f0 100755
--- a/test/sql-tap/select7.test.lua
+++ b/test/sql-tap/select7.test.lua
@@ -179,7 +179,7 @@ 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>
     })
 
-- 
2.7.4