Horton

Thread

Commits

  1. Improve stability of recently-added regression test case.

  2. Fix IOS planning when only some index columns can return an attribute.

  3. Ability to advance replication slots

  4. Allow no-op GiST support functions to be omitted.

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

    David Pereiro Lagares <david@nlpgo.com> — 2018-01-17T17:34:54Z

    Hi,
I was playing a bit with different types of indexes when I noticed that
I was getting an incorrect result for composite GIST indexes if the
first column of the index uses pg_trgm options and the execution plan is
an index only scan.

Here are the (verbose) steps to reproduce the problem in an empty
database: 

Setup:

        root=# SELECT version();
        
        version                                              
        ---------------------------------------------------------------
        9.6.4 on x86_64-pc-linux-gnu, compiled by gcc (Debian 6.3.0-18)
        6.3.0 20170516, 64-bit
        (1 fila)
        
        
        CREATE EXTENSION IF NOT EXISTS pg_trgm;
        CREATE EXTENSION IF NOT EXISTS btree_gist;
        CREATE TABLE words ( id SERIAL PRIMARY KEY, w VARCHAR );
        INSERT INTO words (w) VALUES ('Lorem'), ('ipsum');
        
        
Queries that make a seq scan yield correct results:
        
        root=# SELECT w FROM words WHERE w LIKE '%e%'; 
           w   
        -------
         Lorem
        (1 fila)
        
        root=# EXPLAIN ANALYZE SELECT w FROM words WHERE w LIKE '%e%';
                                                  QUERY PLAN          
        --------------------------------------------------------------
        Seq Scan on words  (cost=0.00..1.02 rows=2 width=6) (actual
        time=0.018..0.020 rows=1 loops=1)
           Filter: ((w)::text ~~ '%e%'::text)
           Rows Removed by Filter: 1
         Planning time: 0.112 ms
         Execution time: 0.040 ms
        (5 filas)
        
Index scan with simple index works fine also:

        root=# SET enable_seqscan = OFF;
        SET
        root=# CREATE INDEX ON words USING GIST(w gist_trgm_ops);
        CREATE INDEX
        root=# SELECT w FROM words WHERE w LIKE '%e%';
           w   
        -------
         Lorem
        (1 fila)
        
        root=# EXPLAIN ANALYZE SELECT w FROM words WHERE w LIKE '%e%';
                                                             QUERY
        PLAN   
        ------------------------------------------------------------------
        Index Scan using words_w_idx on words  (cost=0.13..8.16 rows=2
        width=32) (actual time=0.053..0.054 rows=1 loops=1)
           Index Cond: ((w)::text ~~ '%e%'::text)
           Rows Removed by Index Recheck: 1
         Planning time: 0.101 ms
         Execution time: 0.114 ms
        (5 filas)
        
Queries that use the index only scan return no results:

        root=# CREATE INDEX ON words USING GIST(w gist_trgm_ops, w);
        CREATE INDEX
        root=# VACUUM ANALYZE words;
        VACUUM
        root=# SELECT w FROM words WHERE w LIKE '%e%';
         w 
        ---
        (0 filas)
        
        root=# EXPLAIN ANALYZE SELECT w FROM words WHERE w LIKE '%e%';
                                                                QUERY
        PLAN  
        -------------------------------------------------------------------- Index Only Scan using words_w_w1_idx on words  (cost=0.13..4.16 rows=2 width=6) (actual time=0.043..0.043 rows=0 loops=1)
           Index Cond: (w ~~ '%e%'::text)
           Rows Removed by Index Recheck: 2
           Heap Fetches: 0
         Planning time: 0.114 ms
         Execution time: 0.103 ms
        (6 filas)
        
Thank you for your help.
Regards.

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

    Sergei Kornilov <sk@zsrv.org> — 2018-01-17T19:26:15Z

    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

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

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-01-18T05:48:51Z

    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

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

    x4mmm@yandex-team.ru — 2018-01-18T07:57:38Z

    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.

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

    Michael Paquier <michael.paquier@gmail.com> — 2018-01-18T08:25:05Z

    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

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

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-01-18T10:20:09Z

    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

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

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-01-18T12:14:49Z

    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

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

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2018-01-18T22:16:56Z

    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

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

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-01-19T01:00:36Z

    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

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

    Sergei Kornilov <sk@zsrv.org> — 2018-01-23T21:13:51Z

    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

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

    x4mmm@yandex-team.ru — 2018-01-24T06:34:18Z

    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.

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

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-01-24T07:11:53Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-01T20:39:18Z

    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

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

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-03-07T00:26:38Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-15T18:48:24Z

    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&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