[tarantool-patches] [PATCH] sql: analyze on VIEW lead to an assertion

[tarantool-patches] [PATCH] sql: analyze on VIEW lead to an assertion

[N.Tatunov]

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.

Closes #3324
---

Issue: https://github.com/tarantool/tarantool/issues/3324
Branch: https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3324-analyze-on-view

 src/box/sql/analyze.c          | 10 ++++++++--
 test/sql-tap/analyzeD.test.lua | 23 ++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index f0054c5..d95d534 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1123,6 +1123,8 @@ analyzeDatabase(Parse * pParse)
 	iTab = pParse->nTab;
 	for (k = sqliteHashFirst(&pSchema->tblHash); k; k = sqliteHashNext(k)) {
 		Table *pTab = (Table *) sqliteHashData(k);
+		if (space_is_view(pTab))
+			continue;
 		analyzeOneTable(pParse, pTab, 0, iStatCur, iMem, iTab);
 	}
 	loadAnalysis(pParse);
@@ -1179,8 +1181,12 @@ sqlite3Analyze(Parse * pParse, Token * pName)
 		/* Form 2:  Analyze table named */
 		z = sqlite3NameFromToken(db, pName);
 		if (z) {
-			if ((pTab = sqlite3LocateTable(pParse, 0, z)) != 0) {
-				analyzeTable(pParse, pTab, 0);
+			if ((pTab = sqlite3LocateTable(pParse, 0, z))) {
+				if (space_is_view(pTab))
+					sqlite3ErrorMsg(pParse, "VIEWs aren't "
+					"allowed to be analyzed");
+				else
+					analyzeTable(pParse, pTab, 0);
 			}
 		}
 		sqlite3DbFree(db, z);
diff --git a/test/sql-tap/analyzeD.test.lua b/test/sql-tap/analyzeD.test.lua
index 8fdadf5..5d25a1d 100755
--- a/test/sql-tap/analyzeD.test.lua
+++ b/test/sql-tap/analyzeD.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(9)
+test:plan(11)
 
 testprefix = "analyzeD"
 
@@ -139,5 +139,26 @@ test:do_execsql_test(
         -- </1.8>
    	})
 
+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>
+	})
+
+test:do_catchsql_test(
+	"analyzeD-1.10",
+	[[
+		ANALYZE v;
+	]], {
+		-- <analyzeD-1.10>
+		1, "VIEWs aren't allowed to be analyzed"
+		-- <analyzeD-1.10>
+	})
 
 test:finish_test()
-- 
2.7.4

[tarantool-patches] Re: [PATCH] sql: analyze on VIEW lead to an assertion

[n.pettik]

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:
> 
> 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.


From my point of view, sentence like ‘patch solves this problem'
doesn’t 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.”
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 = sqlite3LocateTable(pParse, 0, z)) != 0) {
> -				analyzeTable(pParse, pTab, 0);
> +			if ((pTab = sqlite3LocateTable(pParse, 0, z))) {

We use explicit NULL checks: != NULL.

> +				if (space_is_view(pTab))
> +					sqlite3ErrorMsg(pParse, "VIEWs aren't "
> +					"allowed to be analyzed”);

I would better use singular form: “VIEW isn’t …";

> +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’t
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’t contain
statistics for table called‘V’.

[tarantool-patches] Re: [PATCH] sql: analyze on VIEW lead to an assertion

[Hollow111]

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

Diff for the newer version of patch after fixing remarks:


diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index f0054c5..d95d534 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1123,6 +1123,8 @@ analyzeDatabase(Parse * pParse)
  iTab = pParse->nTab;
  for (k = sqliteHashFirst(&pSchema->tblHash); k; k = sqliteHashNext(k)) {
  Table *pTab = (Table *) sqliteHashData(k);
+ if (space_is_view(pTab))
+ continue;
  analyzeOneTable(pParse, pTab, 0, iStatCur, iMem, iTab);
  }
  loadAnalysis(pParse);
@@ -1179,8 +1181,12 @@ sqlite3Analyze(Parse * pParse, Token * pName)
  /* Form 2:  Analyze table named */
  z = sqlite3NameFromToken(db, pName);
  if (z) {
- if ((pTab = sqlite3LocateTable(pParse, 0, z)) != 0) {
- analyzeTable(pParse, pTab, 0);
+ if ((pTab = sqlite3LocateTable(pParse, 0, z))) {
+ if (space_is_view(pTab))
+ sqlite3ErrorMsg(pParse, "VIEWs aren't "
+ "allowed to be analyzed");
+ else
+ analyzeTable(pParse, pTab, 0);
  }
  }
  sqlite3DbFree(db, z);
diff --git a/test/sql-tap/analyzeD.test.lua b/test/sql-tap/analyzeD.test.lua
index 8fdadf5..5d25a1d 100755
--- a/test/sql-tap/analyzeD.test.lua
+++ b/test/sql-tap/analyzeD.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(9)
+test:plan(11)

 testprefix = "analyzeD"

@@ -139,5 +139,26 @@ test:do_execsql_test(
         -- </1.8>
    })

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

 test:finish_test()

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

[tarantool-patches] Re: [PATCH] sql: analyze on VIEW lead to an assertion

[Hollow111]

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

I'm sorry it was wrong diff if the patch

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index f0054c5..6ac035b 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1123,6 +1123,8 @@ analyzeDatabase(Parse * pParse)
  iTab = pParse->nTab;
  for (k = sqliteHashFirst(&pSchema->tblHash); k; k = sqliteHashNext(k)) {
  Table *pTab = (Table *) sqliteHashData(k);
+ if (space_is_view(pTab))
+ continue;
  analyzeOneTable(pParse, pTab, 0, iStatCur, iMem, iTab);
  }
  loadAnalysis(pParse);
@@ -1179,8 +1181,12 @@ sqlite3Analyze(Parse * pParse, Token * pName)
  /* Form 2:  Analyze table named */
  z = sqlite3NameFromToken(db, pName);
  if (z) {
- if ((pTab = sqlite3LocateTable(pParse, 0, z)) != 0) {
- analyzeTable(pParse, pTab, 0);
+ if ((pTab = sqlite3LocateTable(pParse, 0, z)) != NULL) {
+ if (space_is_view(pTab))
+ sqlite3ErrorMsg(pParse, "VIEW isn't "
+ "allowed to be analyzed");
+ else
+ analyzeTable(pParse, pTab, 0);
  }
  }
  sqlite3DbFree(db, z);
diff --git a/test/sql-tap/analyzeD.test.lua b/test/sql-tap/analyzeD.test.lua
index 8fdadf5..ef6aced 100755
--- a/test/sql-tap/analyzeD.test.lua
+++ b/test/sql-tap/analyzeD.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(9)
+test:plan(15)

 testprefix = "analyzeD"

@@ -139,5 +139,66 @@ test:do_execsql_test(
         -- </1.8>
    })

+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>
+ })
+
+test:do_execsql_test(
+ "analyzeD-1.10",
+ [[
+ SELECT * FROM "_sql_stat4" WHERE "tbl" = 'v';
+ ]], {
+ -- <analyzeD-1.10>
+
+ -- <analyzeD-1.10>
+ })
+
+test:do_execsql_test(
+ "analyzeD-1.11",
+ [[
+ SELECT * FROM "_sql_stat1" WHERE "tbl" = 'v';
+ ]], {
+ -- <analyzeD-1.11>
+
+ -- <analyzeD-1.11>
+ })
+
+test:do_catchsql_test(
+ "analyzeD-1.12",
+ [[
+ ANALYZE v;
+ ]], {
+ -- <analyzeD-1.12>
+ 1, "VIEW isn't allowed to be analyzed"
+ -- <analyzeD-1.12>
+ })
+
+test:do_execsql_test(
+ "analyzeD-1.13",
+ [[
+ SELECT * FROM "_sql_stat4" WHERE "tbl" = 'v';
+ ]], {
+ -- <analyzeD-1.13>
+
+ -- <analyzeD-1.13>
+ })
+
+test:do_execsql_test(
+ "analyzeD-1.14",
+ [[
+ SELECT * FROM "_sql_stat1" WHERE "tbl" = 'v';
+ ]], {
+ -- <analyzeD-1.14>
+
+ -- <analyzeD-1.14>
+ })

 test:finish_test()

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

[tarantool-patches] Re: [PATCH] sql: analyze on VIEW lead to an assertion

[n.pettik]

LGTM.

[tarantool-patches] Re: [PATCH] sql: analyze on VIEW lead to an assertion

[Kirill Yukhin]

Hello,
On 03 мая 21:28, n.pettik wrote:
> LGTM.

I've checked your patch into 2.0

--
Regards, Kirill Yukhin