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 6AD2326B60 for ; Wed, 18 Jul 2018 09:42:11 -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 Ip-c74Wqnw7N for ; Wed, 18 Jul 2018 09:42:11 -0400 (EDT) Received: from mail-lj1-f196.google.com (mail-lj1-f196.google.com [209.85.208.196]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DA09C20E3F for ; Wed, 18 Jul 2018 09:42:10 -0400 (EDT) Received: by mail-lj1-f196.google.com with SMTP id q5-v6so4125331ljh.12 for ; Wed, 18 Jul 2018 06:42:10 -0700 (PDT) MIME-Version: 1.0 References: <1530533500-9666-1-git-send-email-hollow653@gmail.com> <20180717135933.cc2g7rlbb2xl4kgo@tkn_work_nb> In-Reply-To: From: Nikita Tatunov Date: Wed, 18 Jul 2018 16:41:58 +0300 Message-ID: Subject: [tarantool-patches] Re: [PATCH] sql: assertion fail on nested select Content-Type: multipart/alternative; boundary="000000000000ab8e330571463adb" 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: alexander.turenko@tarantool.org Cc: tarantool-patches@freelists.org --000000000000ab8e330571463adb Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable =D1=81=D1=80, 18 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 15:34, Nikita Tatu= nov : > Hello, Alexander! Please consider changes to the patch: > > =D0=B2=D1=82, 17 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 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. > --000000000000ab8e330571463adb Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=D1=81= =D1=80, 18 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 15:34, Nikita Tatunov &l= t;hollow653@gmail.com>:
Hello, Alexander! Pleas= e consider changes to the patch:

=D0=B2=D1=82, 17 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 16:59, Al= exander 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 no= t 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<= br> > the issue).


Fixed:

To optimize the sel= ect-subquery tarantool uses
subquery flattening optimization whic= h is only used
with respect to some restrictions. When any of
subselects but the last one has an ORDER BY
clause we shou= ld raise an error. So subquery
components containing it are not a= ccepted
to be optimized.

Checking part w= as only checking if the last
subselect contains the clause which = led to an
assertion fail. With the patch applied
flatte= ning is not used in case any of
subselects has an ORDER BY.
=
=C2=A0
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 ha= s
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 te= st that problem on this sqlite3 version with -DSQLITE_DEBUG and assertion f= ails there.=C2=A0
Since flattening only makes another compound se= lect-stmt with ORDER BY clause I guess error was
occuring on flat= tened 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(
> +=C2=A0 =C2=A0 =C2=A0"subquery-9.3",
> +=C2=A0 =C2=A0 =C2=A0[[
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0SELECT * FROM (SELECT= * FROM table1 ORDER BY 1 UNION All
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0SELECT * FROM table1 UNION All > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 SELECT * FROM table1);

All -> ALL

Also fixed.=C2=A0
--000000000000ab8e330571463adb--