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