From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounce@freelists.org>
Received: from localhost (localhost [127.0.0.1])
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id E349B22473
	for <tarantool-patches@freelists.org>; Wed,  2 May 2018 19:34:31 -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 vB_ubO0g7F83 for <tarantool-patches@freelists.org>;
	Wed,  2 May 2018 19:34:31 -0400 (EDT)
Received: from smtp14.mail.ru (smtp14.mail.ru [94.100.181.95])
	(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 288CA22456
	for <tarantool-patches@freelists.org>; Wed,  2 May 2018 19:34:30 -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] sql: analyze on VIEW lead to an assertion
From: "n.pettik" <korablev@tarantool.org>
In-Reply-To: <1525267540-23152-1-git-send-email-hollow653@gmail.com>
Date: Thu, 3 May 2018 02:33:55 +0300
Content-Transfer-Encoding: quoted-printable
Message-Id: <1CAC631B-C9A4-4DF3-9C89-706865E4AB4E@tarantool.org>
References: <1525267540-23152-1-git-send-email-hollow653@gmail.com>
Sender: tarantool-patches-bounce@freelists.org
Errors-to: tarantool-patches-bounce@freelists.org
Reply-To: tarantool-patches@freelists.org
List-help: <mailto:ecartis@freelists.org?Subject=help>
List-unsubscribe: <tarantool-patches-request@freelists.org?Subject=unsubscribe>
List-software: Ecartis version 1.0.0
List-Id: tarantool-patches <tarantool-patches.freelists.org>
List-subscribe: <tarantool-patches-request@freelists.org?Subject=subscribe>
List-owner: <mailto:>
List-post: <mailto:tarantool-patches@freelists.org>
List-archive: <http://www.freelists.org/archives/tarantool-patches>
To: tarantool-patches@freelists.org
Cc: "N. Tatunov" <hollow653@gmail.com>

Generally, patch looks OK to me. I can provide only several minor
comments.

> On 2 May 2018, at 16:25, N.Tatunov <hollow653@gmail.com> wrote:
>=20
> Currently analyze directly on VIEW or
> analyze on db containing a VIEW leads to an
> assertion. This patch adds few appropriate
> check to fix this problem.


=46rom my point of view, sentence like =E2=80=98patch solves this =
problem'
doesn=E2=80=99t seem to be informative enough.
So I would better describe how you fixed problem, sort of:
"ANALYZE statement directly on VIEW raises error,
meanwhile ANALYZE on the whole database just skips views.=E2=80=9D
It would be great, if you rephrased somehow your last message.

Moreover, you are capable of using up to 72 chars in one line
for commit message body. Do not make lines be too short - it is not
elegant.

> -			if ((pTab =3D sqlite3LocateTable(pParse, 0, z)) =
!=3D 0) {
> -				analyzeTable(pParse, pTab, 0);
> +			if ((pTab =3D sqlite3LocateTable(pParse, 0, z))) =
{

We use explicit NULL checks: !=3D NULL.

> +				if (space_is_view(pTab))
> +					sqlite3ErrorMsg(pParse, "VIEWs =
aren't "
> +					"allowed to be analyzed=E2=80=9D);=


I would better use singular form: =E2=80=9CVIEW isn=E2=80=99t =E2=80=A6";

> +test:do_catchsql_test(
> +	"analyzeD-1.9",
> +	[[
> +		CREATE TABLE table1(id INT PRIMARY KEY, a INT);
> +		CREATE VIEW v AS SELECT * FROM table1;
> +		ANALYZE;
> +	]], {
> +		-- <analyzeD-1.9>
> +		0
> +		-- <analyzeD-1.9>
> +	})

Here I would also check that the name of view doesn=E2=80=99t
accidentally trap into stat spaces.

> +test:do_catchsql_test(
> +	"analyzeD-1.10",
> +	[[
> +		ANALYZE v;
> +	]], {
> +		-- <analyzeD-1.10>
> +		1, "VIEWs aren't allowed to be analyzed"
> +		-- <analyzeD-1.10>
> +	})

The same is here: also check that stat spaces don=E2=80=99t contain
statistics for table called=E2=80=98V=E2=80=99.