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