| From: | Greg Burd <greg(at)burd(dot)me> |
|---|---|
| To: | Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Chris Cleveland <ccleveland(at)dieselpoint(dot)com> |
| Subject: | Re: Let an ordering index scan hand its ORDER BY value to the target list |
| Date: | 2026-10-05 16:46:52 |
| Message-ID: | VKFOpl5I4rGZS4ouIc98EAp4AYSVlG7SHpAaV8XHBnr7qA4dTqkeFxMTVBFJ5Nx1V3QTZ-QaykYiPxfg_STJ0QbYtlnZTBiu3BdO7gNkLEA=@burd.me |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Monday, October 5th, 2026 at 10:39 AM, Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com> wrote:
> On Thu, 1 Oct 2026, 21:10 Greg Burd, <greg(at)burd(dot)me> wrote:
> >
> > Hello hackers,
> >
> > I'd like to see core improve the full text search index provided in
> > core at some point which these days means including BM25 (and likely
> > other) algorithm(s). To learn about that space I've been working on
> > an extension [1].
> >
> > While working on that I stumbled onto something that seemed odd and
> > worth investing time in. It turns out that an index scan followed by
> > an order by doesn't do what I'd expected, it doesn't use the
> > orderbyvals returned by the index when ordering results.
> >
> > SELECT id, body <=> 'search terms' AS score
> > FROM docs
> > ORDER BY body <=> 'search terms'
> > LIMIT 10;
> >
> > In that query the executor will get the ranked set of results from
> > the index scan and ignore it and then turn around and re-compute a
> > rank value for each document in isolation.
> >
> > BM25 ranking is over a corpus, not a single document so this was a
> > head scratcher because to me this looked like doing double the work
> > only to get incorrect results.
>
> If it's getting incorrect results then that's probably an issue in
> your extension's definitions.
>
> > I did some digging [2] and I find [3] I'm not alone [4]. And that I
> > agreed with Chris Cleveland's [5] statement:
> >
> > > It would also be nice if the orderbyval could be made available in
> > > the projection. That way we could report the score() in the
> > > result set.
> >
> > Only to find later that Tom explained that this was a more subtle
> > issue that it seemed to be on the surface.
> >
> > > An index ordering operator is an optimization that the planner may
> > > or may not choose to employ. If you've designed your code on the
> > > assumption that that's the only possible plan, it will break for
> > > any but the most trivial queries. [6]
> >
> > Reading into Tom's objection what I take away is that one should not
> > make the operator's own value index-dependent, so that the same SQL
> > expression means different things under different plans, stop me if
> > I'm wrong.
>
> Correct. An index's presence (or being selected in planning) must not
> affect queries' row-data output, unless the query has sufficiently
> incomplete ordering, or the query introspects the catalogs that
> describe that index's existence (but that is besides the point). If
> your operator relies on the whole table's corpus for weights, then
> you'll have to figure out how the operator gets access to the whole
> table's corpus' weights even outside index scan plans -- post-filter
> ordering is a valid plan approach and sometimes the best plan option
> available, even when there is a bm25/vector/etc. index available.
I get your point and I led with the wrong motivation. The BM25 case is
an extension problem: the operator has to find the corpus statistics
itself so that Seq Scan + Sort and Index Scan agree. I shouldn't have
framed any part of this as a way to display an index-derived number that
the operator can't reproduce.
Please read the patch as the thing you agreed with at the end of your
mail: avoiding the double (triple, with xs_recheckorderby) evaluation
of an ORDER BY operator whose value the index already computed.
> > I propose that when the executor chooses an Index Scan and the AM
> > reports its value as exact (`xs_recheckorderby = false`), the
> > executor may substitute that value for a target-list occurrence of
> > the identical ORDER BY expression. It may not substitute anything
> > else.
>
> What "value" are you talking about? Table and Index scans produce
> their results in TupleSlots, and these don't contain the projected
> order-by's expression.
IndexScanDesc.xs_orderbyvals / xs_orderbynulls, which the AM fills via
index_store_float8_orderby_distances() and which IndexNextWithReorder
already uses to sort. The patch copies those (or, for a rechecked row,
the executor's recomputed values) into a one-row virtual slot and lets
the projection read them through an INNER_VAR instead of re-evaluating
the expression.
> Also note that it is not strictly necessary for indexes to know the
> exact output of an order-by expression to know if the ordering it
> outputs needs rechecking: E.g., a query that orders by distance from
> a timestamp, but stores only the week number of the timestamps, can
> still give you a correct ordering that doesn't need rechecking as long
> as the index only has one value per week. The index won't necessarily
> be able to produce the exact ordering value of the output tuple, but
> it will produce data that is ordered in the same order as the
> expression would be.
>
> > - For an AM whose value differs from the operator's (pgvector's
> > squared L2, every BM25 AM), the substitution is observable.
>
> Then isn't every one of those indexes inherently breaking the AM contract?
>
> > The patch mirrors what Index Only Scan already does for indexed
> > columns. With IoS, setrefs rewrites target-list expressions that
> > match an index column into `INDEX_VAR` references. Here, it rewrites
> > target-list expressions that match an ORDER BY expression into
> > references to the scan's ORDER BY values.
>
> It's not quite mirroring the IOS definitions, because it's missing an
> equivalent for amroutine->amcanreturn(). See also my comment above
> about truncated knowledge giving correct results in certain
> situations: I think it's useful to be able to decouple "can return
> tuples in the correct order, with fallback" from "will provide
> complete and correct orderable values for IOS output, with fallback".
Yes. This is the real hole in v1/v2. gist.sgml says that with
recheck = false the distance function's values need only order the
same as the operator, so xs_recheckorderby = false promises ordering,
not value, and a per-AM bool can't express a per-opclass property.
> > Okay, that's more than enough to see if anyone agrees that this is an
> > important things to fix and an approach that is making good
> > trade-offs or not.
>
> I do agree that we should try fixing the double (or even triple, when
> xs_recheckorderby) evaluation of order-by-operator expressions.
For v3 I intend to replace amorderbyvalsexact with a callback shaped
like amcanreturn: amcanreturnorderby(Relation, int attno), NULL by
default. GiST would answer true for an opclass whose distance
function is documented as returning the ordering operator's result
(in core, the point opclass; box/circle/polygon set recheck and never
reach this), false otherwise; SP-GiST likewise for the quad/kd point
opclasses. plancat.c already collects canreturn[] per column, and
indexorderbycols tells setrefs which column each ORDER BY key is on,
so the rewrite is gated per key, and an opclass that orders by a
truncated value is never asked to display one. That also decouples
the two things you listed: "can order correctly, with fallback" stays
xs_recheckorderby; "will return the exact ordering value" becomes the
new callback.
Does that shape look right to you before I write it?
> Kind regards,
>
> Matthias van de Meent
> Databricks (https://www.databricks.com)
>
>
> PS.
>
> > [5] https://pg.ddx.io/m/pgsql-hackers/CABSN6VfLK5msEDSR8wPeMh_h3xNyr2Xs5NJK8+uOZp0bVxw7Hg(at)mail(dot)gmail(dot)com/
>
> Was this mail not available in the project's own archives?
It is; sorry, that was my own mirror site https://pg.ddx.io and I have a
Tampermonkey script that converts links to it and this time I copy/pasted
the link and didn't normalize it to the one more likely to be available
in the future.
[5] https://postgr.es/m/CABSN6VfLK5msEDSR8wPeMh_h3xNyr2Xs5NJK8+uOZp0bVxw7Hg@mail.gmail.com
PS: Have you tried the search at https://pg.ddx.io/search or used the MCP
server at https://pg.ddx.io/mcp ?
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Robert Haas | 2026-10-05 17:01:07 | Re: pg_*_advice: tsv load failure, etc. |
| Previous Message | shihao zhong | 2026-10-05 16:34:18 | [PG19] plpgsql: SELECT INTO sets FOUND wrongly after a function becomes a SRF |