Re: Index-only scan returns incorrect results when using acomposite GIST index with a gist_trgm_ops column.

Thank you for looking this.

At Wed, 24 Jan 2018 00:13:51 +0300, Sergei Kornilov <sk@zsrv.org> wrote in <348951516742031@web54j.yandex.ru>
> Hello
> I tested this patch and think it can be commited to master. Is there a CF record? I can not find one.

Not yet. I'm thinking of creating an entry in the next CF after
waiting someone to pickup this.

> Should we also make backport to older versions? I test on REL_10_STABLE - patch builds and works ok, but "make check"
failson new testcase with error:
 
> >  CREATE INDEX ON t USING gist (a test_inet_ops, a inet_ops);
> >+ ERROR:  missing support function 4 for attribute 1 of index "t_a_a1_idx"
> and with different explain results.

Thank you for checking that. d3a4f89 allowed that and
inet_gist_decompress is removed at the same time. Unfortunately I
didn't find a type on master that has both decompress and fetch
methods so I prefer to split the regression patch among target
versions.

master has its own one. 9.6 and 10 share the same patch.  9.5 has
a different one since the signature of consistent method has
changed as of 9.6. This is not required for 9.4 and earlier since
they don't check canreturn on attribute basis. (And they don't
try index-only on GiST.)

On the other hand the fix patch applies to all affected versions.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 0e221b3396215d67167622cbc07d04b5185d6f66 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Wed, 24 Jan 2018 15:25:48 +0900
Subject: [PATCH] Regression test for the failure of check_index_only.

check_index_only forgets the case where two or more index columns on
the same table column but with different operator class.
---
 src/test/regress/expected/create_index.out | 42 ++++++++++++++++++++++++++++++
 src/test/regress/sql/create_index.sql      | 27 +++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 9c0f3e3..d95157c 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2913,6 +2913,48 @@ explain (costs off)
 (2 rows)
 
 --
+-- Check for duplicate indexkey with different opclasses
+--
+-- opclass that doesn't have function 9 (GIST_FETCH_PROC) for GiST
+CREATE OPERATOR CLASS test_inet_ops FOR TYPE inet USING gist AS
+        OPERATOR        3       &&,
+        FUNCTION        1       inet_gist_consistent (internal, inet, integer, oid, internal),
+        FUNCTION        2       inet_gist_union (internal, internal),
+        FUNCTION        3       inet_gist_compress (internal),
+        FUNCTION        4       inet_gist_decompress (internal),
+        FUNCTION        5       inet_gist_penalty (internal, internal, internal),
+        FUNCTION        6       inet_gist_picksplit (internal, internal),
+        FUNCTION        7       inet_gist_same (inet, inet, internal);
+CREATE TABLE t (a inet);
+CREATE INDEX ON t USING gist (a test_inet_ops, a inet_ops);
+INSERT INTO t VALUES ('192.168.0.1');
+VACUUM t;
+SELECT * FROM t WHERE a && '192.168.0.1'; -- should retuan a value
+      a      
+-------------
+ 192.168.0.1
+(1 row)
+
+-- enfoce GiST
+SET enable_seqscan TO false;
+SET enable_bitmapscan TO false;
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE a && '192.168.0.1'; -- shoudn't be index only scan
+                QUERY PLAN                
+------------------------------------------
+ Index Scan using t_a_a1_idx on t
+   Index Cond: (a && '192.168.0.1'::inet)
+(2 rows)
+
+SELECT * FROM t WHERE a && '192.168.0.1'; -- also should return a value
+      a      
+-------------
+ 192.168.0.1
+(1 row)
+
+-- cleanup
+DROP TABLE t;
+DROP OPERATOR CLASS test_inet_ops USING gist;
+--
 -- REINDEX (VERBOSE)
 --
 CREATE TABLE reindex_verbose(id integer primary key);
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index b199c23..95a5060 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -982,6 +982,33 @@ explain (costs off)
   select * from tenk1 where (thousand, tenthous) in ((1,1001), (null,null));
 
 --
+-- Check for duplicate indexkey with different opclasses
+--
+-- opclass that doesn't have function 9 (GIST_FETCH_PROC) for GiST
+CREATE OPERATOR CLASS test_inet_ops FOR TYPE inet USING gist AS
+        OPERATOR        3       &&,
+        FUNCTION        1       inet_gist_consistent (internal, inet, integer, oid, internal),
+        FUNCTION        2       inet_gist_union (internal, internal),
+        FUNCTION        3       inet_gist_compress (internal),
+        FUNCTION        4       inet_gist_decompress (internal),
+        FUNCTION        5       inet_gist_penalty (internal, internal, internal),
+        FUNCTION        6       inet_gist_picksplit (internal, internal),
+        FUNCTION        7       inet_gist_same (inet, inet, internal);
+CREATE TABLE t (a inet);
+CREATE INDEX ON t USING gist (a test_inet_ops, a inet_ops);
+INSERT INTO t VALUES ('192.168.0.1');
+VACUUM t;
+SELECT * FROM t WHERE a && '192.168.0.1'; -- should retuan a value
+-- enfoce GiST
+SET enable_seqscan TO false;
+SET enable_bitmapscan TO false;
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE a && '192.168.0.1'; -- shoudn't be index only scan
+SELECT * FROM t WHERE a && '192.168.0.1'; -- also should return a value
+-- cleanup
+DROP TABLE t;
+DROP OPERATOR CLASS test_inet_ops USING gist;
+
+--
 -- REINDEX (VERBOSE)
 --
 CREATE TABLE reindex_verbose(id integer primary key);
-- 
2.9.2

From 0b734a1950077aee1f43b3d3141bde8e8eeb2778 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Wed, 24 Jan 2018 14:54:56 +0900
Subject: [PATCH] Regression test for the failure of check_index_only.

check_index_only forgets the case where two or more index columns on
the same table column but with different operator class.
---
 src/test/regress/expected/create_index.out | 42 ++++++++++++++++++++++++++++++
 src/test/regress/sql/create_index.sql      | 27 +++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 064adb4..483522e 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2990,6 +2990,48 @@ explain (costs off)
 (4 rows)
 
 --
+-- Check for duplicate indexkey with different opclasses
+--
+-- opclass that doesn't have function 9 (GIST_FETCH_PROC) for GiST
+CREATE OPERATOR CLASS test_inet_ops FOR TYPE inet USING gist AS
+        OPERATOR        3       &&,
+        FUNCTION        1       inet_gist_consistent (internal, inet, smallint, oid, internal),
+        FUNCTION        2       inet_gist_union (internal, internal),
+        FUNCTION        3       inet_gist_compress (internal),
+        FUNCTION        4       inet_gist_decompress (internal),
+        FUNCTION        5       inet_gist_penalty (internal, internal, internal),
+        FUNCTION        6       inet_gist_picksplit (internal, internal),
+        FUNCTION        7       inet_gist_same (inet, inet, internal);
+CREATE TABLE t (a inet);
+CREATE INDEX ON t USING gist (a test_inet_ops, a inet_ops);
+INSERT INTO t VALUES ('192.168.0.1');
+VACUUM t;
+SELECT * FROM t WHERE a && '192.168.0.1'; -- should retuan a value
+      a      
+-------------
+ 192.168.0.1
+(1 row)
+
+-- enfoce GiST
+SET enable_seqscan TO false;
+SET enable_bitmapscan TO false;
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE a && '192.168.0.1'; -- shoudn't be index only scan
+                QUERY PLAN                
+------------------------------------------
+ Index Scan using t_a_a1_idx on t
+   Index Cond: (a && '192.168.0.1'::inet)
+(2 rows)
+
+SELECT * FROM t WHERE a && '192.168.0.1'; -- also should return a value
+      a      
+-------------
+ 192.168.0.1
+(1 row)
+
+-- cleanup
+DROP TABLE t;
+DROP OPERATOR CLASS test_inet_ops USING gist;
+--
 -- REINDEX (VERBOSE)
 --
 CREATE TABLE reindex_verbose(id integer primary key);
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 67470db..4d58ef0 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1026,6 +1026,33 @@ explain (costs off)
   select * from boolindex where not b order by i limit 10;
 
 --
+-- Check for duplicate indexkey with different opclasses
+--
+-- opclass that doesn't have function 9 (GIST_FETCH_PROC) for GiST
+CREATE OPERATOR CLASS test_inet_ops FOR TYPE inet USING gist AS
+        OPERATOR        3       &&,
+        FUNCTION        1       inet_gist_consistent (internal, inet, smallint, oid, internal),
+        FUNCTION        2       inet_gist_union (internal, internal),
+        FUNCTION        3       inet_gist_compress (internal),
+        FUNCTION        4       inet_gist_decompress (internal),
+        FUNCTION        5       inet_gist_penalty (internal, internal, internal),
+        FUNCTION        6       inet_gist_picksplit (internal, internal),
+        FUNCTION        7       inet_gist_same (inet, inet, internal);
+CREATE TABLE t (a inet);
+CREATE INDEX ON t USING gist (a test_inet_ops, a inet_ops);
+INSERT INTO t VALUES ('192.168.0.1');
+VACUUM t;
+SELECT * FROM t WHERE a && '192.168.0.1'; -- should retuan a value
+-- enfoce GiST
+SET enable_seqscan TO false;
+SET enable_bitmapscan TO false;
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE a && '192.168.0.1'; -- shoudn't be index only scan
+SELECT * FROM t WHERE a && '192.168.0.1'; -- also should return a value
+-- cleanup
+DROP TABLE t;
+DROP OPERATOR CLASS test_inet_ops USING gist;
+
+--
 -- REINDEX (VERBOSE)
 --
 CREATE TABLE reindex_verbose(id integer primary key);
-- 
2.9.2

From 4bbd78bc8d3219c2be7d388553e6c9f23b242a05 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 18 Jan 2018 20:57:13 +0900
Subject: [PATCH 1/2] Regression test for the failure of check_index_only.

check_index_only forgets the case where two or more index columns on
the same table column but with different operator class.
---
 src/test/regress/expected/create_index.out | 41 ++++++++++++++++++++++++++++++
 src/test/regress/sql/create_index.sql      | 26 +++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 031a0bc..065aad4 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2990,6 +2990,47 @@ explain (costs off)
 (4 rows)
 
 --
+-- Check for duplicate indexkey with different opclasses
+--
+-- opclass that doesn't have function 9 (GIST_FETCH_PROC) for GiST
+CREATE OPERATOR CLASS test_inet_ops FOR TYPE inet USING gist AS
+        OPERATOR        3       &&,
+        FUNCTION        1       inet_gist_consistent (internal, inet, smallint, oid, internal),
+        FUNCTION        2       inet_gist_union (internal, internal),
+        FUNCTION        3       inet_gist_compress (internal),
+        FUNCTION        5       inet_gist_penalty (internal, internal, internal),
+        FUNCTION        6       inet_gist_picksplit (internal, internal),
+        FUNCTION        7       inet_gist_same (inet, inet, internal);
+CREATE TABLE t (a inet);
+CREATE INDEX ON t USING gist (a test_inet_ops, a inet_ops);
+INSERT INTO t VALUES ('192.168.0.1');
+VACUUM t;
+SELECT * FROM t WHERE a && '192.168.0.1'; -- should retuan a value
+      a      
+-------------
+ 192.168.0.1
+(1 row)
+
+-- enfoce GiST
+SET enable_seqscan TO false;
+SET enable_bitmapscan TO false;
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE a && '192.168.0.1'; -- shoudn't be index only scan
+                QUERY PLAN                
+------------------------------------------
+ Index Scan using t_a_a1_idx on t
+   Index Cond: (a && '192.168.0.1'::inet)
+(2 rows)
+
+SELECT * FROM t WHERE a && '192.168.0.1'; -- also should return a value
+      a      
+-------------
+ 192.168.0.1
+(1 row)
+
+-- cleanup
+DROP TABLE t;
+DROP OPERATOR CLASS test_inet_ops USING gist;
+--
 -- REINDEX (VERBOSE)
 --
 CREATE TABLE reindex_verbose(id integer primary key);
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index a45e8eb..642ac33 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1026,6 +1026,32 @@ explain (costs off)
   select * from boolindex where not b order by i limit 10;
 
 --
+-- Check for duplicate indexkey with different opclasses
+--
+-- opclass that doesn't have function 9 (GIST_FETCH_PROC) for GiST
+CREATE OPERATOR CLASS test_inet_ops FOR TYPE inet USING gist AS
+        OPERATOR        3       &&,
+        FUNCTION        1       inet_gist_consistent (internal, inet, smallint, oid, internal),
+        FUNCTION        2       inet_gist_union (internal, internal),
+        FUNCTION        3       inet_gist_compress (internal),
+        FUNCTION        5       inet_gist_penalty (internal, internal, internal),
+        FUNCTION        6       inet_gist_picksplit (internal, internal),
+        FUNCTION        7       inet_gist_same (inet, inet, internal);
+CREATE TABLE t (a inet);
+CREATE INDEX ON t USING gist (a test_inet_ops, a inet_ops);
+INSERT INTO t VALUES ('192.168.0.1');
+VACUUM t;
+SELECT * FROM t WHERE a && '192.168.0.1'; -- should retuan a value
+-- enfoce GiST
+SET enable_seqscan TO false;
+SET enable_bitmapscan TO false;
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE a && '192.168.0.1'; -- shoudn't be index only scan
+SELECT * FROM t WHERE a && '192.168.0.1'; -- also should return a value
+-- cleanup
+DROP TABLE t;
+DROP OPERATOR CLASS test_inet_ops USING gist;
+
+--
 -- REINDEX (VERBOSE)
 --
 CREATE TABLE reindex_verbose(id integer primary key);
-- 
2.9.2

From fec0283e4b8ea5b692c82a2abeca782b64e3d5ad Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 18 Jan 2018 21:04:18 +0900
Subject: [PATCH 2/2] Fix check_index_only for the case of duplicate keycolumn

If there is an index that have multiple keys on the same attribute but
using different operator class, only the last key on the same
attribute affects the availability of the attribute. This causes
failure of rechecking index tuples. An attribute should be considered
as unavailable if any key on it cannot return the attribute.
---
 src/backend/optimizer/path/indxpath.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 7fc7080..2fede1d 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -1866,6 +1866,7 @@ check_index_only(RelOptInfo *rel, IndexOptInfo *index)
     bool        result;
     Bitmapset  *attrs_used = NULL;
     Bitmapset  *index_canreturn_attrs = NULL;
+    Bitmapset  *index_cannotreturn_attrs = NULL;
     ListCell   *lc;
     int            i;
 
@@ -1905,7 +1906,8 @@ check_index_only(RelOptInfo *rel, IndexOptInfo *index)
 
     /*
      * Construct a bitmapset of columns that the index can return back in an
-     * index-only scan.
+     * index-only scan. We must have a value for all occurances of the same
+     * attribute since it can be used for rechecking.
      */
     for (i = 0; i < index->ncolumns; i++)
     {
@@ -1922,11 +1924,19 @@ check_index_only(RelOptInfo *rel, IndexOptInfo *index)
             index_canreturn_attrs =
                 bms_add_member(index_canreturn_attrs,
                                attno - FirstLowInvalidHeapAttributeNumber);
+        else
+            index_cannotreturn_attrs =
+                bms_add_member(index_cannotreturn_attrs,
+                               attno - FirstLowInvalidHeapAttributeNumber);
     }
 
+    index_canreturn_attrs = bms_del_members(index_canreturn_attrs,
+                                            index_cannotreturn_attrs);
+
     /* Do we have all the necessary attributes? */
     result = bms_is_subset(attrs_used, index_canreturn_attrs);
 
+
     bms_free(attrs_used);
     bms_free(index_canreturn_attrs);
 
-- 
2.9.2