[tarantool-patches] [PATCH] sql: assertion fail on nested select

[tarantool-patches] [PATCH] sql: assertion fail on nested select

[N.Tatunov]

To optimize the select-subquery tarantool uses
subquery flattening function which is only used
with respect to some restrictions. One of them
was implemented improperly.

Closes: #3353
---

Issue: https://github.com/tarantool/tarantool/issues/3353
Branch: https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3353-assertion-on-nested-select

 src/box/sql/select.c           |  6 +++---
 test/sql-tap/subquery.test.lua | 48 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 54f78a9..c035691 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -3893,14 +3893,14 @@ flattenSubquery(Parse * pParse,		/* Parsing context */
 	 * queries.
 	 */
 	if (pSub->pPrior) {
-		if (pSub->pOrderBy) {
-			return 0;	/* Restriction 20 */
-		}
 		if (isAgg || (p->selFlags & SF_Distinct) != 0
 		    || pSrc->nSrc != 1) {
 			return 0;
 		}
 		for (pSub1 = pSub; pSub1; pSub1 = pSub1->pPrior) {
+			if (pSub1->pOrderBy) {
+				return 0;	/* Restriction 20 */
+			}
 			testcase((pSub1->
 				  selFlags & (SF_Distinct | SF_Aggregate)) ==
 				 SF_Distinct);
diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua
index 06631c1..d1e17b3 100755
--- a/test/sql-tap/subquery.test.lua
+++ b/test/sql-tap/subquery.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(69)
+test:plan(73)
 
 --!./tcltestrunner.lua
 -- 2005 January 19
@@ -926,6 +926,52 @@ test:do_execsql_test(
         -- </subquery-8.1>
     })
 
+test:do_catchsql_test(
+	"subquery-9.0",
+	[[
+		DROP TABLE IF EXISTS table1;
+		CREATE TABLE table1 (id VARCHAR(100) PRIMARY KEY);
+		INSERT INTO table1 VALUES ('abc'), ('abd');
+	]], {
+		-- <subquery-9.0>
+		0
+		-- <subquery-9.0>
+	})
+
+test:do_catchsql_test(
+	"subquery-9.1",
+	[[
+		SELECT * FROM (SELECT * FROM table1 UNION ALL
+                               SELECT * FROM table1 ORDER BY 1 UNION ALL
+			       SELECT * FROM table1);
+	]], {
+		-- <subquery-9.1>
+		1, 'ORDER BY clause should come after UNION ALL not before'
+		-- <subquery-9.1>
+	})
+
+test:do_execsql_test(
+	"subquery-9.2",
+	[[
+		SELECT 'abc' FROM (SELECT * FROM table1 UNION ALL
+                                   SELECT * FROM table1 UNION ALL
+				   SELECT * FROM table1 ORDER BY 1);
+	]], {
+		-- <subquery-9.2>
+		'abc','abc','abc','abc','abc','abc'
+		-- <subquery-9.2>
+	})
 
+test:do_catchsql_test(
+	"subquery-9.3",
+	[[
+		SELECT * FROM (SELECT * FROM table1 ORDER BY 1 UNION All
+                               SELECT * FROM table1 UNION All
+			       SELECT * FROM table1);
+	]], {
+             -- <subquery-9.3>
+             1, 'ORDER BY clause should come after UNION ALL not before'
+             -- <subquery-9.3>
+	})
 
 test:finish_test()
-- 
2.7.4

[tarantool-patches] Re: [PATCH] sql: assertion fail on nested select

[Alexander Turenko]

Hi Nikita!

Please, consider comments below.

WBR, Alexander Turenko.

On Mon, Jul 02, 2018 at 03:11:40PM +0300, N.Tatunov wrote:
> To optimize the select-subquery tarantool uses
> subquery flattening function which is only used
> with respect to some restrictions. One of them
> was implemented improperly.
> 

Be certain: 'some restriction' and just 'improperly' are not good
problem descriptions.

I would use wording like the following.

> When a subquery is a compound select the subquery flattening
> optimization verify whether the ORDER BY clause presents, but only for
> the last subquery component. The commit fixes that to verify all
> subquery components.

> In case of ORDER BY in non-last component of a compound select the
> error should be raised. Accepting of such subqueries for the
> flattening optimization prevents the error to be raised afterwards.
> Instead it stuck into assertion violation inside flattenSubquery (see
> the issue).

I checked sqlite3 check-in 8000d230 (it is immediate predecessor of
c9104b59 where the flattening optimization seems to be changed, but
tarantool didn't updated with that check-in). It has the assert, but has
no a change like yours, and correctly shows the following error:

Error: ORDER BY clause should come after UNION ALL not before

Why it is so?

> +test:do_catchsql_test(
> +	"subquery-9.3",
> +	[[
> +		SELECT * FROM (SELECT * FROM table1 ORDER BY 1 UNION All
> +                               SELECT * FROM table1 UNION All
> +			       SELECT * FROM table1);

All -> ALL

[tarantool-patches] Re: [PATCH] sql: assertion fail on nested select

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 2392 bytes --]

Hello, Alexander! Please consider changes to the patch:

вт, 17 июл. 2018 г. в 16:59, Alexander Turenko <
alexander.turenko@tarantool.org>:

> Hi Nikita!
>
> Please, consider comments below.
>
> WBR, Alexander Turenko.
>
> On Mon, Jul 02, 2018 at 03:11:40PM +0300, N.Tatunov wrote:
> > To optimize the select-subquery tarantool uses
> > subquery flattening function which is only used
> > with respect to some restrictions. One of them
> > was implemented improperly.
> >
>
> Be certain: 'some restriction' and just 'improperly' are not good
> problem descriptions.
>
> I would use wording like the following.
>
> > When a subquery is a compound select the subquery flattening
> > optimization verify whether the ORDER BY clause presents, but only for
> > the last subquery component. The commit fixes that to verify all
> > subquery components.
>
> > In case of ORDER BY in non-last component of a compound select the
> > error should be raised. Accepting of such subqueries for the
> > flattening optimization prevents the error to be raised afterwards.
> > Instead it stuck into assertion violation inside flattenSubquery (see
> > the issue).
>
>
Fixed:

To optimize the select-subquery tarantool uses
subquery flattening optimization which is only used
with respect to some restrictions. When any of
subselects but the last one has an ORDER BY
clause we should raise an error. So subquery
components containing it are not accepted
to be optimized.

Checking part was only checking if the last
subselect contains the clause which led to an
assertion fail. With the patch applied
flattening is not used in case any of
subselects has an ORDER BY.


> I checked sqlite3 check-in 8000d230 (it is immediate predecessor of
> c9104b59 where the flattening optimization seems to be changed, but
> tarantool didn't updated with that check-in). It has the assert, but has
> no a change like yours, and correctly shows the following error:
>
> Error: ORDER BY clause should come after UNION ALL not before
>
> Why it is so?
>
> > +test:do_catchsql_test(
> > +     "subquery-9.3",
> > +     [[
> > +             SELECT * FROM (SELECT * FROM table1 ORDER BY 1 UNION All
> > +                               SELECT * FROM table1 UNION All
> > +                            SELECT * FROM table1);
>
> All -> ALL
>

Also fixed.

[-- Attachment #2: Type: text/html, Size: 3202 bytes --]

[tarantool-patches] Re: [PATCH] sql: assertion fail on nested select

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 3046 bytes --]

ср, 18 июл. 2018 г. в 15:34, Nikita Tatunov <hollow653@gmail.com>:

> Hello, Alexander! Please consider changes to the patch:
>
> вт, 17 июл. 2018 г. в 16:59, Alexander Turenko <
> alexander.turenko@tarantool.org>:
>
>> Hi Nikita!
>>
>> Please, consider comments below.
>>
>> WBR, Alexander Turenko.
>>
>> On Mon, Jul 02, 2018 at 03:11:40PM +0300, N.Tatunov wrote:
>> > To optimize the select-subquery tarantool uses
>> > subquery flattening function which is only used
>> > with respect to some restrictions. One of them
>> > was implemented improperly.
>> >
>>
>> Be certain: 'some restriction' and just 'improperly' are not good
>> problem descriptions.
>>
>> I would use wording like the following.
>>
>> > When a subquery is a compound select the subquery flattening
>> > optimization verify whether the ORDER BY clause presents, but only for
>> > the last subquery component. The commit fixes that to verify all
>> > subquery components.
>>
>> > In case of ORDER BY in non-last component of a compound select the
>> > error should be raised. Accepting of such subqueries for the
>> > flattening optimization prevents the error to be raised afterwards.
>> > Instead it stuck into assertion violation inside flattenSubquery (see
>> > the issue).
>>
>>
> Fixed:
>
> To optimize the select-subquery tarantool uses
> subquery flattening optimization which is only used
> with respect to some restrictions. When any of
> subselects but the last one has an ORDER BY
> clause we should raise an error. So subquery
> components containing it are not accepted
> to be optimized.
>
> Checking part was only checking if the last
> subselect contains the clause which led to an
> assertion fail. With the patch applied
> flattening is not used in case any of
> subselects has an ORDER BY.
>
>
>> I checked sqlite3 check-in 8000d230 (it is immediate predecessor of
>> c9104b59 where the flattening optimization seems to be changed, but
>> tarantool didn't updated with that check-in). It has the assert, but has
>> no a change like yours, and correctly shows the following error:
>>
>> Error: ORDER BY clause should come after UNION ALL not before
>>
>> Why it is so?
>>
>>
I tried to test that problem on this sqlite3 version with -DSQLITE_DEBUG
and assertion fails there.
Since flattening only makes another compound select-stmt with ORDER BY
clause I guess error was
occuring on flattened version of it. Moreover in latter versions of sqlite3
this assert was deleted and the
restriction check wasn't fixed. So I assume sqlite3 developers just don't
know about it or don't aim on
fixing it as the error anyways occurs but on flattened subquery.

> +test:do_catchsql_test(
>> > +     "subquery-9.3",
>> > +     [[
>> > +             SELECT * FROM (SELECT * FROM table1 ORDER BY 1 UNION All
>> > +                               SELECT * FROM table1 UNION All
>> > +                            SELECT * FROM table1);
>>
>> All -> ALL
>>
>
> Also fixed.
>

[-- Attachment #2: Type: text/html, Size: 4408 bytes --]

[tarantool-patches] Re: [PATCH] sql: assertion fail on nested select

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

I woudln't consider it as an actual 'fix' though.

ср, 18 июл. 2018 г. в 18:17, n.pettik@corp.mail.ru <n.pettik@corp.mail.ru>:

>
> I tried to test that problem on this sqlite3 version with -DSQLITE_DEBUG
> and assertion fails there.
> Since flattening only makes another compound select-stmt with ORDER BY
> clause I guess error was
> occuring on flattened version of it. Moreover in latter versions of
> sqlite3 this assert was deleted and the
> restriction check wasn't fixed. So I assume sqlite3 developers just don't
> know about it or don't aim on
> fixing it as the error anyways occurs but on flattened subquery.
>
>
> Here is original fix of this assertion fail (JFYI):
> https://www.sqlite.org/src/info/823779d31eb09cda
>
>

[-- Attachment #2: Type: text/html, Size: 1573 bytes --]

[tarantool-patches] Re: [PATCH] sql: assertion fail on nested select

[Alexander Turenko]

The patch LGTM. Nikita P., what do you think?

WBR, Alexander Turenko.

On Wed, Jul 18, 2018 at 06:37:00PM +0300, Nikita Tatunov wrote:
>    I woudln't consider it as an actual 'fix' though.
> 
>    ср, 18 июл. 2018 г. в 18:17, [1]n.pettik@corp.mail.ru
>    <[2]n.pettik@corp.mail.ru>:
> 
>    I tried to test that problem on this sqlite3 version with
>    -DSQLITE_DEBUG and assertion fails there.
>    Since flattening only makes another compound select-stmt with ORDER BY
>    clause I guess error was
>    occuring on flattened version of it. Moreover in latter versions of
>    sqlite3 this assert was deleted and the
>    restriction check wasn't fixed. So I assume sqlite3 developers just
>    don't know about it or don't aim on
>    fixing it as the error anyways occurs but on flattened subquery.
> 
>    Here is original fix of this assertion fail (JFYI):
>    [3]https://www.sqlite.org/src/info/823779d31eb09cda
> 
> References
> 
>    1. mailto:n.pettik@corp.mail.ru
>    2. mailto:n.pettik@corp.mail.ru
>    3. https://www.sqlite.org/src/info/823779d31eb09cda

[tarantool-patches] Re: [PATCH] sql: assertion fail on nested select

[n.pettik]

> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 54f78a9..c035691 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -3893,14 +3893,14 @@ flattenSubquery(Parse * pParse,		/* Parsing context */
> 	 * queries.
> 	 */
> 	if (pSub->pPrior) {
> -		if (pSub->pOrderBy) {
> -			return 0;	/* Restriction 20 */
> -		}
> 		if (isAgg || (p->selFlags & SF_Distinct) != 0
> 		    || pSrc->nSrc != 1) {
> 			return 0;
> 		}
> 		for (pSub1 = pSub; pSub1; pSub1 = pSub1->pPrior) {
> +			if (pSub1->pOrderBy) {
> +				return 0;	/* Restriction 20 */
> +			}

According to our code style:
- use explicit != NULL comparison;
- don’t put bracers around one-line if statement;
- put comment above the code to be commented;

>diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua
>--- a/test/sql-tap/subquery.test.lua
>+++ b/test/sql-tap/subquery.test.lua
>+test:do_execsql_test(
>+       "subquery-9.2",
>+       [[
>+               SELECT 'abc' FROM (SELECT * FROM table1 UNION ALL
>+                                   SELECT * FROM table1 UNION ALL
>+                                  SELECT * FROM table1 ORDER BY 1);
>+       ]], {
>+               -- <subquery-9.2>
>+               'abc','abc','abc','abc','abc','abc'
>+               -- <subquery-9.2>
>+       })

Why did you use here SELECT ‘abc’ and in other tests - SELECT *?
It is nitpicking, but just tests look a little weird..

[tarantool-patches] Re: [PATCH] sql: assertion fail on nested select

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 4268 bytes --]

ср, 18 июл. 2018 г. в 23:36, n.pettik <korablev@tarantool.org>:

>
> > diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> > index 54f78a9..c035691 100644
> > --- a/src/box/sql/select.c
> > +++ b/src/box/sql/select.c
> > @@ -3893,14 +3893,14 @@ flattenSubquery(Parse * pParse,               /*
> Parsing context */
> >        * queries.
> >        */
> >       if (pSub->pPrior) {
> > -             if (pSub->pOrderBy) {
> > -                     return 0;       /* Restriction 20 */
> > -             }
> >               if (isAgg || (p->selFlags & SF_Distinct) != 0
> >                   || pSrc->nSrc != 1) {
> >                       return 0;
> >               }
> >               for (pSub1 = pSub; pSub1; pSub1 = pSub1->pPrior) {
> > +                     if (pSub1->pOrderBy) {
> > +                             return 0;       /* Restriction 20 */
> > +                     }
>
> According to our code style:
> - use explicit != NULL comparison;
> - don’t put bracers around one-line if statement;
> - put comment above the code to be commented;
>
>
Fixed.


> >diff --git a/test/sql-tap/subquery.test.lua
> b/test/sql-tap/subquery.test.lua
> >--- a/test/sql-tap/subquery.test.lua
> >+++ b/test/sql-tap/subquery.test.lua
> >+test:do_execsql_test(
> >+       "subquery-9.2",
> >+       [[
> >+               SELECT 'abc' FROM (SELECT * FROM table1 UNION ALL
> >+                                   SELECT * FROM table1 UNION ALL
> >+                                  SELECT * FROM table1 ORDER BY 1);
> >+       ]], {
> >+               -- <subquery-9.2>
> >+               'abc','abc','abc','abc','abc','abc'
> >+               -- <subquery-9.2>
> >+       })
>
> Why did you use here SELECT ‘abc’ and in other tests - SELECT *?
> It is nitpicking, but just tests look a little weird..


Yes, kind of weird. Why not though?
Fixed, anyways.

Diff:

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 54f78a9..1a6b034 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -3893,14 +3893,14 @@ flattenSubquery(Parse * pParse, /* Parsing context
*/
  * queries.
  */
  if (pSub->pPrior) {
- if (pSub->pOrderBy) {
- return 0; /* Restriction 20 */
- }
  if (isAgg || (p->selFlags & SF_Distinct) != 0
      || pSrc->nSrc != 1) {
  return 0;
  }
  for (pSub1 = pSub; pSub1; pSub1 = pSub1->pPrior) {
+ /* Restriction 20 */
+ if (pSub1->pOrderBy != NULL)
+ return 0;
  testcase((pSub1->
    selFlags & (SF_Distinct | SF_Aggregate)) ==
  SF_Distinct);
diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua
index 06631c1..8fde62d 100755
--- a/test/sql-tap/subquery.test.lua
+++ b/test/sql-tap/subquery.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(69)
+test:plan(73)

 --!./tcltestrunner.lua
 -- 2005 January 19
@@ -926,6 +926,52 @@ test:do_execsql_test(
         -- </subquery-8.1>
     })

+test:do_catchsql_test(
+ "subquery-9.0",
+ [[
+ DROP TABLE IF EXISTS table1;
+ CREATE TABLE table1 (id VARCHAR(100) PRIMARY KEY);
+ INSERT INTO table1 VALUES ('abc'), ('abd');
+ ]], {
+ -- <subquery-9.0>
+ 0
+ -- <subquery-9.0>
+ })
+
+test:do_catchsql_test(
+ "subquery-9.1",
+ [[
+ SELECT * FROM (SELECT * FROM table1 UNION ALL
+                               SELECT * FROM table1 ORDER BY 1 UNION ALL
+        SELECT * FROM table1);
+ ]], {
+ -- <subquery-9.1>
+ 1, 'ORDER BY clause should come after UNION ALL not before'
+ -- <subquery-9.1>
+ })
+
+test:do_execsql_test(
+ "subquery-9.2",
+ [[
+ SELECT * FROM (SELECT * FROM table1 UNION ALL
+                                   SELECT * FROM table1 UNION ALL
+    SELECT * FROM table1 ORDER BY 1);
+ ]], {
+ -- <subquery-9.2>
+ 'abc', 'abc', 'abc', 'abd', 'abd', 'abd'
+ -- <subquery-9.2>
+ })

+test:do_catchsql_test(
+ "subquery-9.3",
+ [[
+ SELECT * FROM (SELECT * FROM table1 ORDER BY 1 UNION ALL
+                               SELECT * FROM table1 UNION ALL
+        SELECT * FROM table1);
+ ]], {
+             -- <subquery-9.3>
+             1, 'ORDER BY clause should come after UNION ALL not before'
+             -- <subquery-9.3>
+ })

 test:finish_test()

[-- Attachment #2: Type: text/html, Size: 8390 bytes --]

[tarantool-patches] Re: [PATCH] sql: assertion fail on nested select

[n.pettik]

Now LGTM.

[tarantool-patches] Re: [PATCH] sql: assertion fail on nested select

[Vladislav Shpilevoy]

LGTM as well.

On 19/07/2018 16:09, n.pettik wrote:
> Now LGTM.
> 

[tarantool-patches] Re: [PATCH] sql: assertion fail on nested select

[Kirill Yukhin]

Hello,
On 02 июл 15:11, N.Tatunov wrote:
> To optimize the select-subquery tarantool uses
> subquery flattening function which is only used
> with respect to some restrictions. One of them
> was implemented improperly.
> 
> Closes: #3353
> ---
> 
> Issue: https://github.com/tarantool/tarantool/issues/3353
> Branch: https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3353-assertion-on-nested-select
I've checke the patch into 2.0 branch.

--
Regards, Kirill Yukhin