* [tarantool-patches] [PATCH] sql: analyze on VIEW lead to an assertion
@ 2018-05-02 13:25 N.Tatunov
2018-05-02 23:33 ` [tarantool-patches] " n.pettik
0 siblings, 1 reply; 6+ messages in thread
From: N.Tatunov @ 2018-05-02 13:25 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, 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
^ permalink raw reply [flat|nested] 6+ messages in thread* [tarantool-patches] Re: [PATCH] sql: analyze on VIEW lead to an assertion
2018-05-02 13:25 [tarantool-patches] [PATCH] sql: analyze on VIEW lead to an assertion N.Tatunov
@ 2018-05-02 23:33 ` n.pettik
2018-05-03 17:16 ` Hollow111
0 siblings, 1 reply; 6+ messages in thread
From: n.pettik @ 2018-05-02 23:33 UTC (permalink / raw)
To: tarantool-patches; +Cc: N. Tatunov
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’.
^ permalink raw reply [flat|nested] 6+ messages in thread* [tarantool-patches] Re: [PATCH] sql: analyze on VIEW lead to an assertion
2018-05-02 23:33 ` [tarantool-patches] " n.pettik
@ 2018-05-03 17:16 ` Hollow111
2018-05-03 17:19 ` Hollow111
0 siblings, 1 reply; 6+ messages in thread
From: Hollow111 @ 2018-05-03 17:16 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread* [tarantool-patches] Re: [PATCH] sql: analyze on VIEW lead to an assertion
2018-05-03 17:16 ` Hollow111
@ 2018-05-03 17:19 ` Hollow111
2018-05-03 18:28 ` n.pettik
0 siblings, 1 reply; 6+ messages in thread
From: Hollow111 @ 2018-05-03 17:19 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-15 15:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-02 13:25 [tarantool-patches] [PATCH] sql: analyze on VIEW lead to an assertion N.Tatunov
2018-05-02 23:33 ` [tarantool-patches] " n.pettik
2018-05-03 17:16 ` Hollow111
2018-05-03 17:19 ` Hollow111
2018-05-03 18:28 ` n.pettik
2018-05-15 15:56 ` Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox