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

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

[Kirill Shcherbatov]

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.
---
Branch: http://github.com/tarantool/tarantool/tree/kshsh/gh-3382-select-compound-limit-fix
Issue: https://github.com/tarantool/tarantool/issues/3382

 src/box/sql/parse.y                                |  4 +++-
 src/box/sql/sqliteLimit.h                          |  8 ++++----
 test/sql-tap/gh2548-select-compound-limit.test.lua | 17 +++++++++++++++--
 test/sql-tap/select7.test.lua                      |  2 +-
 4 files changed, 23 insertions(+), 8 deletions(-)

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/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.
  *
- * 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 30
 #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..6546f1d 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(9)
 
 -- 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,13 @@ 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: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

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

[n.pettik]

> 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.

It is quite dubious statement without proofs. I guess you borrowed it
from comments in source code, but anyway lets drop it from commit message.
* It may be applied to SQLite which is considered as light db to be used
  on mobile devices etc *

> Fiber stack is 64KB by default, so maximum number of entities
> should be about 30 to stack guard will not be triggered.

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?

> 
> Closes #3382.
> ---
> Branch: http://github.com/tarantool/tarantool/tree/kshsh/gh-3382-select-compound-limit-fix
> Issue: https://github.com/tarantool/tarantool/issues/3382
> 
> src/box/sql/parse.y                                |  4 +++-
> src/box/sql/sqliteLimit.h                          |  8 ++++----
> test/sql-tap/gh2548-select-compound-limit.test.lua | 17 +++++++++++++++--
> test/sql-tap/select7.test.lua                      |  2 +-
> 4 files changed, 23 insertions(+), 8 deletions(-)
> 
> 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]);

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.

>       }
>     }
>   }
> 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. 

I found this (https://www.sqlite.org/lang_select.html):

‘''
The VALUES clause

The phrase "VALUES(expr-list)" means the same thing as "SELECT expr-list”.
The phrase "VALUES(expr-list-1),...,(expr-list-N)" means the same thing as
"SELECT expr-list-1 UNION ALL ... UNION ALL SELECT expr-list-N”.
Both forms are the same, except that the number of SELECT statements in
a compound is limited bySQLITE_LIMIT_COMPOUND_SELECT whereas
the number of rows in a VALUES clause has no arbitrary limit.
‘’'

Do we have tests on this case?

>  *
> - * 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.

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

[Kirill Shcherbatov]

> 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

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

[n.pettik]

Patch is almost OK, see several minor nitpicks.

>> 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

It doesn’t imply that the author of this commit did it in the right way.

> 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.

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).

>>>      }
>>>    }
>>>  }
>>> 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.

Ok. My question was “Is this statement true”?
Now I've checked and limit 0 really means “no limit”.

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

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

>>> *
>>> - * 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.

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

> 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

This pragma status is not displayed via simple ‘pragma’.

> ---
> 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
> 

> 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

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.

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

[Kirill Shcherbatov]

> 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

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

[n.pettik]

>> 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.

I don’t think so. It looks OK and it (default engine) seems to
be very useful for user. However, I don’t insist on this change,
so let it be as is.

> @@ -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
>     ]], {

Garbage diff.

Otherwise, patch LGTM.

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

[Kirill Yukhin]

Hello,
On 11 сен 14:47, Kirill Shcherbatov wrote:
> 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.
> ---
> Branch: http://github.com/tarantool/tarantool/tree/kshsh/gh-3382-select-compound-limit-fix
> Issue: https://github.com/tarantool/tarantool/issues/3382
I've checked your patch into 2.0 branch.

BTW, I've submitted feature request to relax the restriction:
https://github.com/tarantool/tarantool/issues/3700

--
Regards, Kirill Yukhin