From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id ACF46297E1 for ; Wed, 19 Sep 2018 17:29:22 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id kJbkWhB1UqTg for ; Wed, 19 Sep 2018 17:29:22 -0400 (EDT) Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id F1BF3294D5 for ; Wed, 19 Sep 2018 17:29:21 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: decrease SELECT_COMPOUND_LIMIT threshold From: "n.pettik" In-Reply-To: <53ae8b41-f859-4296-71e5-c424e5f285fa@tarantool.org> Date: Thu, 20 Sep 2018 00:29:13 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <79E2AE74-BDE1-4E03-9345-F7DD863F811B@tarantool.org> References: <53ae8b41-f859-4296-71e5-c424e5f285fa@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov Patch is almost OK, see several minor nitpicks. >> Why did you choose 30? AFAIU previous =E2=80=9950=E2=80=99 also was = taken as >> 'stack guard will not be triggered=E2=80=99 number. >> Is this just typical =E2=80=98less than 50 but not too much=E2=80=99?:)= >> Have you checked this number on different compilers/OSs? > I've done everything same way than >=20 > commit 974bce9aeb169b3f8c08924d8afb95bedc4a6c53 > Author: Kirill Yukhin > Date: Thu Jun 29 21:07:41 2017 +0300 >=20 > sql: Limit number of terms in SELECT compoind stmts >=20 > 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. >=20 > 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. >=20 > Closes #2548 It doesn=E2=80=99t 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. >=20 >> 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=E2=80=9D). Or change message = to simple: >> (limit is %d). It is only request, I like this additional information = btw. >> Also, another cool option would be or . >> 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. >>=20 >> Is this comment relevant? I know that it is trapped here = accidentally, >> but lets remove it in case it is outdated.=20 > It is not outdated with introducing new pragma. Ok. My question was =E2=80=9CIs this statement true=E2=80=9D? Now I've checked and limit 0 really means =E2=80=9Cno limit=E2=80=9D. >=20 >> 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. >>=20 >> It is so ridiculous :)) Never say never > =C2=AF\_(=E3=83=84)_/=C2=AF >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D >=20 > 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. >=20 > Closes #3382. >=20 > @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=3D20 This pragma status is not displayed via simple =E2=80=98pragma=E2=80=99. > --- > 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(-) >=20 > diff --git a/src/box/sql/main.c b/src/box/sql/main.c > index 69b2fec..d30d6af 100644 > --- a/src/box/sql/main.c >=20 > 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 >=20 > +/** 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.