[Tarantool-patches] [PATCH] sql: fix index consideration with INDEXED BY clause

[Tarantool-patches] [PATCH] sql: fix index consideration with INDEXED BY clause

[Nikita Pettik]

Accidentally, number of indexes to be considered during query planning
in presence of INDEXED BY is calculated wrong. Instead of one (INDEXED
BY is not a hint but requirement) index to be used (which is indicated
in INDEXED BY clause), all space indexes take part in query planning.
There are not so many tests checking this feature, so unfortunately this
bug was hidden. Let's fix it and force only one index to be used in QP
in case of INDEXED BY clause.
---
Branch: https://github.com/tarantool/tarantool/tree/np/sql-indexed-by-fix

Bug is found by one of Tarantool's customers in MailRu.

 src/box/sql/where.c       |  7 +++++--
 test/sql-tap/eqp.test.lua | 19 ++++++++++++++++++-
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index ed507bf4d..7ec43e184 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -2867,8 +2867,11 @@ tnt_error:
 	 * If there was an INDEXED BY clause, then only that one
 	 * index is considered.
 	 */
-	uint32_t idx_count = fake_index == NULL || pSrc->pIBIndex != NULL ?
-			     space->index_count : 1;
+	uint32_t idx_count = 0;
+	if (pSrc->pIBIndex != NULL || fake_index != NULL)
+		idx_count = 1;
+	else
+		idx_count = space->index_count;
 	for (uint32_t i = 0; i < idx_count; iSortIdx++, i++) {
 		if (i > 0)
 			probe = space->index[i]->def;
diff --git a/test/sql-tap/eqp.test.lua b/test/sql-tap/eqp.test.lua
index b8c3c6607..83ef947bb 100755
--- a/test/sql-tap/eqp.test.lua
+++ b/test/sql-tap/eqp.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(68)
+test:plan(70)
 
 --!./tcltestrunner.lua
 -- 2010 November 6
@@ -772,6 +772,9 @@ test:do_execsql_test(
     [[
         CREATE TABLE t1(a INT , b INT , c INT , PRIMARY KEY(b, c));
         CREATE TABLE t2(id  INT primary key, a INT , b INT , c INT );
+        CREATE TABLE t(id INT PRIMARY KEY, r_d TEXT, s INTEGER);
+        CREATE INDEX i1 ON t (r_d, s);
+        CREATE INDEX i2 ON t (s);
     ]])
 
 test:do_eqp_test("8.1.1", "SELECT * FROM t2", {
@@ -796,5 +799,19 @@ test:do_eqp_test("8.2.4", "SELECT count(*) FROM t1", {
     {0, 0, 0, "B+tree count T1"},
 })
 
+-- Verify that INDEXED BY clause forces specified index.
+-- Test case (in simplified form) is taken from customer.
+--
+test:do_eqp_test(
+    "8.2.5.1",
+    [[ SELECT r_d, s FROM t INDEXED BY i1 WHERE r_d > '10' LIMIT 10; ]], {
+    {0, 0, 0, "SEARCH TABLE T USING COVERING INDEX I1 (R_D>?) (~262144 rows)"},
+})
+
+test:do_eqp_test(
+    "8.2.5.2",
+    [[ SELECT r_d, s FROM t INDEXED BY i1 WHERE r_d > '10' AND s = 0 LIMIT 10; ]], {
+    { 0, 0, 0, "SEARCH TABLE T USING COVERING INDEX I1 (R_D>?) (~245760 rows)" },
+})
 
 test:finish_test()
-- 
2.15.1

Re: [Tarantool-patches] [PATCH] sql: fix index consideration with INDEXED BY clause

[Vladislav Shpilevoy]

Hi! Thanks for the patch! LGTM.

On 26/12/2019 01:48, Nikita Pettik wrote:
> Accidentally, number of indexes to be considered during query planning
> in presence of INDEXED BY is calculated wrong. Instead of one (INDEXED
> BY is not a hint but requirement) index to be used (which is indicated
> in INDEXED BY clause), all space indexes take part in query planning.
> There are not so many tests checking this feature, so unfortunately this
> bug was hidden. Let's fix it and force only one index to be used in QP
> in case of INDEXED BY clause.
> ---
> Branch: https://github.com/tarantool/tarantool/tree/np/sql-indexed-by-fix

Re: [Tarantool-patches] [PATCH] sql: fix index consideration with INDEXED BY clause

[Nikita Pettik]

On 27 Dec 14:31, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch! LGTM.

Pushed to master.
 
> On 26/12/2019 01:48, Nikita Pettik wrote:
> > Accidentally, number of indexes to be considered during query planning
> > in presence of INDEXED BY is calculated wrong. Instead of one (INDEXED
> > BY is not a hint but requirement) index to be used (which is indicated
> > in INDEXED BY clause), all space indexes take part in query planning.
> > There are not so many tests checking this feature, so unfortunately this
> > bug was hidden. Let's fix it and force only one index to be used in QP
> > in case of INDEXED BY clause.
> > ---
> > Branch: https://github.com/tarantool/tarantool/tree/np/sql-indexed-by-fix

Re: [Tarantool-patches] [PATCH] sql: fix index consideration with INDEXED BY clause

[Nikita Pettik]

On 27 Dec 13:48, Nikita Pettik wrote:
> On 27 Dec 14:31, Vladislav Shpilevoy wrote:
> > Hi! Thanks for the patch! LGTM.
> 
> Pushed to master.

I've also backported to 2.2 (since without this fix INDEXED BY is
almost useless).
  
> > On 26/12/2019 01:48, Nikita Pettik wrote:
> > > Accidentally, number of indexes to be considered during query planning
> > > in presence of INDEXED BY is calculated wrong. Instead of one (INDEXED
> > > BY is not a hint but requirement) index to be used (which is indicated
> > > in INDEXED BY clause), all space indexes take part in query planning.
> > > There are not so many tests checking this feature, so unfortunately this
> > > bug was hidden. Let's fix it and force only one index to be used in QP
> > > in case of INDEXED BY clause.
> > > ---
> > > Branch: https://github.com/tarantool/tarantool/tree/np/sql-indexed-by-fix