| From: | Vaibhav Dalvi <vaibhav(dot)dalvi(at)enterprisedb(dot)com> |
|---|---|
| To: | Andrew Dunstan <andrew(at)dunslane(dot)net> |
| Cc: | Alexander Korotkov <aekorotkov(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, "dgrowleyml(at)gmail(dot)com" <dgrowleyml(at)gmail(dot)com> |
| Subject: | Re: gist_trgm_ops '=' operator: planner picks it over btree, ~300x slower |
| Date: | 2026-10-10 05:42:26 |
| Message-ID: | CA+vB=AGRVyUpku63vwteiZs_Wb=UGgSQK3pw70quCQQmBhTVEA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
I have added a CommitFest entry for this patch:
https://commitfest.postgresql.org/patch/7401
The CI run there showed that v1 fails on all the Meson-based jobs
(Autoconf was fine). I had updated the Makefile but forgot
contrib/pg_trgm/meson.build, so pg_trgm--1.6--1.7.sql was not
installed and CREATE EXTENSION pg_trgm failed with "no installation
script nor update path for version 1.7". The new pg_trgm_noeq test was
also not listed there.
PFA v2, which fixes this by adding both to meson.build. There is no
other changes. I have run the full Meson test suite locally and it
passes.
Please have a look.
Regards,
Vaibhav Dalvi
On Tue, Oct 6, 2026 at 4:32 PM Vaibhav Dalvi <vaibhav(dot)dalvi(at)enterprisedb(dot)com>
wrote:
> Hi Andrew,
>
> The same patch is applicable.
> I am re-attaching it for review and for the CF bot to pick up.
>
> Regards,
> Vaibhav
>
>
> On Tue, Oct 6, 2026 at 9:49 AM Vaibhav Dalvi <
> vaibhav(dot)dalvi(at)enterprisedb(dot)com> wrote:
>
>> Thank you, Andrew for the response and input.
>>
>> I will proceed as you suggested.
>>
>> Thanks,
>> Vaibhav
>>
>> On Sat, Oct 3, 2026 at 7:37 PM Andrew Dunstan <andrew(at)dunslane(dot)net>
>> wrote:
>>
>>>
>>> On 2026-09-02 We 9:29 AM, Andrew Dunstan wrote:
>>> >
>>> > On 2026-09-02 We 9:00 AM, Alexander Korotkov wrote:
>>> >> Hi, Vaibhav!
>>> >>
>>> >> On Wed, Sep 2, 2026 at 2:12 PM Vaibhav Dalvi
>>> >> <vaibhav(dot)dalvi(at)enterprisedb(dot)com> wrote:
>>> >>> I would like to raise an old issue again. David Rowley reported
>>> >>> this same problem back on 2024-09-17, in this[1] thread, but it did
>>> not
>>> >>> get any replies.
>>> >>>
>>> >>> I am hitting the same problem, so I am posting it again with three
>>> >>> different patches, since it can badly hurt anyone who has both a
>>> >>> btree and a gist_trgm_ops index on the same column.
>>> >>>
>>> >>> In short: once a table has both a btree index and a gist_trgm_ops
>>> >>> index on the same text column, the planner sometimes picks the GiST
>>> >>> index for a plain equality (=) query, and that GiST plan can be
>>> >>> hundreds of times slower than the btree plan for the exact same
>>> >>> query. Below is a small, self-contained test case:
>>> >>>
>>> >>> create extension if not exists pg_trgm;
>>> >>> create table t1 (a varchar(250), b varchar(250), c varchar(250));
>>> >>> create index t1_a_btree on t1 (a);
>>> >>> create index t1_a_gist on t1 using gist (a gist_trgm_ops);
>>> >>> insert into t1 select md5(a::text),md5(a::text),md5(a::text) from
>>> >>> generate_series(1,100000)a;
>>> >>> vacuum freeze analyze t1;
>>> >>>
>>> >>> explain (analyze, buffers) select * from t1 where a = '1234';
>>> >>> QUERY PLAN
>>> >>>
>>> --------------------------------------------------------------------------------------------------------------------
>>>
>>> >>>
>>> >>> Index Scan using t1_a_gist on t1 (cost=0.28..8.30 rows=1
>>> >>> width=99) (actual time=15.186..15.187 rows=0.00 loops=1)
>>> >>> Index Cond: ((a)::text = '1234'::text)
>>> >>> Buffers: shared hit=1583
>>> >>> Execution Time: 15.242 ms
>>> >>> (4 rows)
>>> >>>
>>> >>> -- disabling the GiST index makes the planner fall back to btree,
>>> >>> -- and the same query becomes ~355x faster:
>>> >>> update pg_index set indisvalid = false where
>>> >>> indexrelid='t1_a_gist'::regclass;
>>> >>> explain (analyze, buffers) select * from t1 where a = '1234';
>>> >>> QUERY PLAN
>>> >>>
>>> -------------------------------------------------------------------------------------------------------------------
>>>
>>> >>>
>>> >>> Index Scan using t1_a_btree on t1 (cost=0.42..8.44 rows=1
>>> >>> width=99) (actual time=0.022..0.022 rows=0.00 loops=1)
>>> >>> Index Cond: ((a)::text = '1234'::text)
>>> >>> Buffers: shared hit=3
>>> >>> Execution Time: 0.043 ms
>>> >>> (4 rows)
>>> >>>
>>> >>> The estimated cost of both plans is almost the same (8.30 vs 8.44),
>>> >>> but the real cost is not even close. The root cause: GiST's cost
>>> >>> estimate for '=' on gist_trgm_ops does not reflect the real cost of
>>> >>> the scan, so the planner can pick GiST over a much cheaper btree
>>> >>> index, even for people who kept both indexes only for other reasons
>>> >>> (LIKE queries, for example).
>>> >>>
>>> >>> I looked at three different ways to fix this:
>>> >>>
>>> >>> 1) v1-0001-Drop-equality-operator-from-gist_trgm_ops.patch
>>> >>> Stops gist_trgm_ops from offering '=' at all (gin_trgm_ops is
>>> >>> untouched, its cost estimator was already fixed for the same
>>> issue
>>> >>> in commit cd9479af2af). Small, but it changes the existing
>>> >>> opclass's behavior for anyone already relying on '=' through it.
>>> >>>
>>> >>> 2) v1-0001-gist-trgm-real-signature-stats.patch
>>> >>> Keeps '=' on gist_trgm_ops, and adds a new optional GiST support
>>> >>> function so the opclass can correct the planner's estimate using
>>> a
>>> >>> real measurement sampled from the index's own pages, instead of a
>>> >>> guess. It works, but it only helps "column = constant" conditions
>>> >>> (not joins), a small index still falls back to a pessimistic
>>> >>> guess, and it adds uncached I/O to every planning call. More
>>> >>> machinery than I'm comfortable with for this.
>>> >>>
>>> >>> 3) v1-0001-Add-gist_trgm_ops_noeq.patch
>>> >>> Adds a second, independently-named opclass, gist_trgm_ops_noeq,
>>> >>> identical to gist_trgm_ops except that it does not register '='
>>> >>> at all. gist_trgm_ops itself is completely untouched; anyone who
>>> >>> wants the safety creates new indexes with the new opclass, or
>>> >>> rebuilds an existing index onto it, and '=' simply cannot reach
>>> it
>>> >>> afterward, under any planner settings. No core or planner changes
>>> >>> at all, just a SQL/DDL addition.
>>> >>>
>>> >>> Of the three, (3) is the one I would prefer to take forward. It is
>>> >>> the smallest, safest change: nothing existing changes behavior, there
>>> >>> is no heuristic or cost-model logic to get wrong, and it is easy to
>>> >>> verify its correctness just by looking at the catalog entries. (1)
>>> >>> fixes the
>>> >>> regression but forces the choice on everyone using gist_trgm_ops for
>>> >>> '=' today, and (2) is real but has enough rough edges (documented in
>>> >>> that patch) that I would not want to see it committed as-is.
>>> >>>
>>> >>> Would appreciate the community's view on this, or any other approach
>>> >>> I may have missed.
>>> >>>
>>> >>> Thank you, @Andrew Dunstan for the offline inputs.
>>> >> I think 2 looks like the right direction. But I wonder about the cost
>>> >> of the cost estimate. Scanning 30 random index pages could be easily
>>> >> way more costly than the query execution. What about letting GiST
>>> >> opclass store custom statistics which would reflect index quality
>>> >> (like key overlapping degree per page level or something), then use it
>>> >> for fast scan cost estimation?
>>> >>
>>> >
>>> > Yes, and I think v2 has serious limitations in any case. The best way
>>> > forward might be for us to have a new AM hook that lets an opclass
>>> > walk its own pages during ANALYZE (bounded, since ANALYZE is already
>>> > expected to do I/O and is rate-limited by autovacuum, unlike planning)
>>> > and persist something like your per-level overlap measure through that
>>> > same path.
>>> >
>>> > But that's a large project.
>>> >
>>> > Meanwhile users encountering the problem can create the new
>>> > opfamily/opclass for themselves as in patch 3, and the recreate their
>>> > indexes using the new opclass. Or we could adopt patch 3 of we decide
>>> > not to go for a more thorough solution.
>>> >
>>> >
>>> >
>>>
>>> In the absence of further discussion, I suggest Vaibhav creates a CF
>>> entry for item 3. It will cost almost nothing and provide users a
>>> reasonable way forward.
>>>
>>>
>>> cheers
>>>
>>>
>>> andrew
>>>
>>> --
>>> Andrew Dunstan
>>> EDB: https://www.enterprisedb.com
>>>
>>>
| Attachment | Content-Type | Size |
|---|---|---|
| v2-Add-gist_trgm_ops_noeq.patch | application/octet-stream | 9.1 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Haibo Yan | 2026-10-10 06:10:48 | [PATCH] Fix grouping-set nulling of variable-free grouping expressions |
| Previous Message | Ayush Tiwari | 2026-10-10 03:44:26 | Re: Reset unlogged relations before syncing the data directory? |