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

Hello
I can reproduce on actual 9.6.6, 10.1 and fresh master build (9c7d06d60680c7f00d931233873dee81fdb311c6 commit). I did not check earlier versions

set enable_indexonlyscan to off ;
postgres=# SELECT w FROM words WHERE w LIKE '%e%';
w
-------
Lorem
Index scan result is correct. Affected only index only scan,

PS: i find GIST(w gist_trgm_ops, w); some strange idea, but result is incorrect in any case.

best regards, Sergei

Hello,

Gist imposes the ninth strategy to perform index only scan but
planner is not considering that.

At Wed, 17 Jan 2018 22:26:15 +0300, Sergei Kornilov <sk@zsrv.org>
wrote in <412861516217175@web38o.yandex.ru>

Hello
I can reproduce on actual 9.6.6, 10.1 and fresh master build
(9c7d06d60680c7f00d931233873dee81fdb311c6 commit). I did not check
earlier versions

set enable_indexonlyscan to off ;
postgres=# SELECT w FROM words WHERE w LIKE '%e%';
w
-------
Lorem
Index scan result is correct. Affected only index only scan,

PS: i find GIST(w gist_trgm_ops, w); some strange idea, but result
is incorrect in any case.

The cause is that gist_trgm_ops lacks "fetch" method but planner
is failing to find that.

https://www.postgresql.org/docs/10/static/gist-extensibility.html

The optional ninth method fetch is needed if the operator class
wishes to support index-only scans.

Index only scan is not usable in the case since the first index
column cannot be rechecked but check_index_only makes wrong
decision by the second occurance of "w'. There may be a chance
that recheck is not required but we cannot predict that until
actually acquire a tuple during execution.

Please find the attached patch.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello!

18 янв. 2018 г., в 10:48, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> написал(а):

Gist imposes the ninth strategy to perform index only scan but
planner is not considering that
....
Please find the attached patch.

I agree with you that current behavior is a bug and your patch seems correct.
I'm a bit worried about ninth strategy words: fetch is not necessary now, if opclass lacks compress methods - index only scan is possible. See https://github.com/postgres/postgres/commit/d3a4f89d8a3e500bd7c0b7a8a8a5ce1b47859128 for details.
Though there are tests in cube and seg for that, if your patch passes check-world, than this behavior is not affected.

Thank you for the patch!

Best regards, Andrey Borodin.

On Thu, Jan 18, 2018 at 12:57:38PM +0500, Andrey Borodin wrote:

18 янв. 2018 г., в 10:48, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> написал(а):

Gist imposes the ninth strategy to perform index only scan but
planner is not considering that
....
Please find the attached patch.

I agree with you that current behavior is a bug and your patch seems correct.
I'm a bit worried about ninth strategy words: fetch is not necessary

now, if opclass lacks compress methods - index only scan is
possible. See
https://github.com/postgres/postgres/commit/d3a4f89d8a3e500bd7c0b7a8a8a5ce1b47859128
for details.

Though there are tests in cube and seg for that, if your patch passes
check-world, than this behavior is not affected.

The proposed patch has no regression tests. If the current set is not
enough to stress the problem, you surely should add some (haven't
checked the patch in detail, sorry ;p ).
--
Michael

At Thu, 18 Jan 2018 12:57:38 +0500, Andrey Borodin <x4mmm@yandex-team.ru> wrote in <62C2B9A0-51BC-40FF-9BCA-F203784CB9C1@yandex-team.ru>

Hello!

18 янв. 2018 г., в 10:48, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> написал(а):

Gist imposes the ninth strategy to perform index only scan but
planner is not considering that
....
Please find the attached patch.

I agree with you that current behavior is a bug and your patch seems correct.
I'm a bit worried about ninth strategy words: fetch is not necessary now, if opclass lacks compress methods - index only scan is possible. See https://github.com/postgres/postgres/commit/d3a4f89d8a3e500bd7c0b7a8a8a5ce1b47859128 for details.

It is true. I don't think that fetch for trigm is feasible but it
is workable if created.

Though there are tests in cube and seg for that, if your patch passes check-world, than this behavior is not affected.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello.

At Thu, 18 Jan 2018 17:25:05 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in <20180118082505.GA84508@paquier.xyz>

On Thu, Jan 18, 2018 at 12:57:38PM +0500, Andrey Borodin wrote:

Please find the attached patch.

I agree with you that current behavior is a bug and your patch seems correct.
I'm a bit worried about ninth strategy words: fetch is not necessary

now, if opclass lacks compress methods - index only scan is
possible. See
https://github.com/postgres/postgres/commit/d3a4f89d8a3e500bd7c0b7a8a8a5ce1b47859128
for details.

Though there are tests in cube and seg for that, if your patch passes
check-world, than this behavior is not affected.

The proposed patch has no regression tests. If the current set is not
enough to stress the problem, you surely should add some (haven't
checked the patch in detail, sorry ;p ).

Uggg.. I'm beaten again.. You're definitely right!

It was a bit hard to find the way to cause the failure without
extension but the first attached file is that.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Jan 18, 2018 at 8:48 AM, Kyotaro HORIGUCHI <
horiguchi.kyotaro@lab.ntt.co.jp> wrote:

At Wed, 17 Jan 2018 22:26:15 +0300, Sergei Kornilov <sk@zsrv.org>
wrote in <412861516217175@web38o.yandex.ru>

Hello
I can reproduce on actual 9.6.6, 10.1 and fresh master build
(9c7d06d60680c7f00d931233873dee81fdb311c6 commit). I did not check
earlier versions

set enable_indexonlyscan to off ;
postgres=# SELECT w FROM words WHERE w LIKE '%e%';
w
-------
Lorem
Index scan result is correct. Affected only index only scan,

PS: i find GIST(w gist_trgm_ops, w); some strange idea, but result
is incorrect in any case.

The cause is that gist_trgm_ops lacks "fetch" method but planner
is failing to find that.

https://www.postgresql.org/docs/10/static/gist-extensibility.html

The optional ninth method fetch is needed if the operator class
wishes to support index-only scans.

Index only scan is not usable in the case since the first index
column cannot be rechecked but check_index_only makes wrong
decision by the second occurance of "w'. There may be a chance
that recheck is not required but we cannot predict that until
actually acquire a tuple during execution.

I didn't get this point. Same column is indexed twice using two
different opclasses. The first opclass doesn't support index-only scan,
while the second opclass does support it. So, if the first opclass needs
recheck then it can be rechecked using the value got from the second
opclass. Thus, I think we can potentially support index-only scan
in this case.

Another thing is that it's probably hard to do in our current
optimizer/executor/am
infrastructure. And assuming that use-case is quite narrow, and it looks
OK to just disable such feature as bug fix.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hello.

At Fri, 19 Jan 2018 01:16:56 +0300, Alexander Korotkov <a.korotkov@postgrespro.ru> wrote in <CAPpHfdvJeYrnAsXhKRfO_NMDUaWQyK+wyhcv4zOdRzTdfNCkkw@mail.gmail.com>

On Thu, Jan 18, 2018 at 8:48 AM, Kyotaro HORIGUCHI <
horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Index only scan is not usable in the case since the first index
column cannot be rechecked but check_index_only makes wrong
decision by the second occurance of "w'. There may be a chance
that recheck is not required but we cannot predict that until
actually acquire a tuple during execution.

I didn't get this point. Same column is indexed twice using two
different opclasses. The first opclass doesn't support index-only scan,
while the second opclass does support it. So, if the first opclass needs
recheck then it can be rechecked using the value got from the second
opclass. Thus, I think we can potentially support index-only scan
in this case.
Another thing is that it's probably hard to do in our current
optimizer/executor/am
infrastructure. And assuming that use-case is quite narrow, and it looks

To be exact, you're right. My description was abridged by
omitting exactly what you mentioned above. The complexity needed
to allow that doesn't seem worth adding.

OK to just disable such feature as bug fix.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello
I tested this patch and think it can be commited to master. Is there a CF record? I can not find one.

Should we also make backport to older versions? I test on REL_10_STABLE - patch builds and works ok, but "make check" fails on 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.

regards, Sergei

Hi!

24 янв. 2018 г., в 2:13, Sergei Kornilov <sk@zsrv.org> написал(а):

Should we also make backport to older versions? I test on REL_10_STABLE - patch builds and works ok, but "make check" fails on 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.

To be usable for 10 the patch must use full-featured opclass in tests: there is no decompress GiST function in test_inet_ops which was required before 11.
I think, explain results will be identical.

Best regards, Andrey Borodin.

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" fails on 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

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:

At Wed, 24 Jan 2018 00:13:51 +0300, Sergei Kornilov <sk@zsrv.org> wrote in <348951516742031@web54j.yandex.ru>

Should we also make backport to older versions? I test on REL_10_STABLE - patch builds and works ok, but "make check" fails on 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.

I pushed this fix with minor adjustments. I did not like the proposed
regression test at all: it was overcomplicated and the need for different
versions for different back branches wasn't fun either. After some poking
around I found that the bug could be exhibited using just btree_gist's
gist_inet_ops, since the core inet_ops class indexes the same datatype and
it does have a fetch function. So I added a test case in btree_gist.

regards, tom lane

At Thu, 01 Mar 2018 15:39:18 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <22609.1519936758@sss.pgh.pa.us>

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:

At Wed, 24 Jan 2018 00:13:51 +0300, Sergei Kornilov <sk@zsrv.org> wrote in <348951516742031@web54j.yandex.ru>

Should we also make backport to older versions? I test on REL_10_STABLE - patch builds and works ok, but "make check" fails on 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.

I pushed this fix with minor adjustments. I did not like the proposed

Thank you.

regression test at all: it was overcomplicated and the need for different
versions for different back branches wasn't fun either. After some poking
around I found that the bug could be exhibited using just btree_gist's
gist_inet_ops, since the core inet_ops class indexes the same datatype and
it does have a fetch function. So I added a test case in btree_gist.

Ah, It wasn't in my sight to test core in contrib. Thanks for
improving it.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:

At Thu, 01 Mar 2018 15:39:18 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <22609.1519936758@sss.pgh.pa.us>

... After some poking
around I found that the bug could be exhibited using just btree_gist's
gist_inet_ops, since the core inet_ops class indexes the same datatype and
it does have a fetch function. So I added a test case in btree_gist.

Ah, It wasn't in my sight to test core in contrib. Thanks for
improving it.

I've just noticed that this new test case is sometimes failing on
CLOBBER_CACHE_ALWAYS buildfarm animals, eg here:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jaguarundi&amp;dt=2018-10-14%2022%3A11%3A00

(I've seen some similar failures before but hadn't looked into the
reason for them.)

I can duplicate the plan choice shown here if I stick in "ANALYZE
inettmp", so presumably what is happening is that sometimes a
background auto-analyze is managing to run and change the rowcount
estimate.

We don't really care about bitmap indexscan vs regular here, rather
about index-only vs regular, so there's not anything much wrong with
using the post-ANALYZE behavior. So I think what we should do to
make this test case more stable is to change the VACUUM to VACUUM
ANALYZE and accept the ensuing change in expected plan.

I doubt this test failure would ever occur in normal builds, so I'm
not going to risk touching it immediately before a release wrap.
But I'll make the change tomorrow or so.

regards, tom lane