Re: Postgres: Queries are too slow after upgrading to PG17 from PG15

On Mon, Aug 4, 2025 at 6:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I took a very quick look at this

Thanks for the review.

> * I don't love the fact that _bt_presort_const_array shares no code
> with _bt_sort_array_elements.  Maybe there's nothing to be done
> about it given that they're working on different representations.
> But the comments could at least point out that the two functions
> had better give equivalent results.

I'll try to improve matters here for the next revision. But it
probably won't look all that different.

> * I really don't like the fact that SK_PRESORTED gets set so far
> away from the code that's actually doing the work to make that
> valid (not to mention far from the code that consults that flag).
> Even if it's bug-free today, that seems like an invitation to
> maintenance problems.  Can we factor that differently?

It sounds like you're not so much complaining about SK_PRESORTED
itself as you are about the v1 patch's assumption that all scans of
amsearcharray-index-AM indexes with a Const arg must already be
presorted. In other words, it seems as if this complaint is closely
related to (if not exactly the same as) your later complaint about
modularity issues.

If there *was* a separate "presorted" SAOP property/struct field, then
you might not find it odd to see a SK_PRESORTED flag, used to
represent that presortedness property at the scan key level. It'd be
fairly similar to the way that we already don't pass a
ScalarArrayOpExpr down to nbtree directly (we just pass down an
ArrayType and set the SK_SEARCHARRAY scan key flag).

Note that SK_SEARCHARRAY is set in precisely one place on the master
branch, which is also the only place where the new SK_PRESORTED flag
gets set with the patch applied: within ExecIndexBuildScanKeys.

> > I'm unsure of how to assess my approach from a modularity perspective.
> > The patch simply assumes that any amsearcharray index AM is either
> > nbtree, or some other amsearcharray index AM that is okay with having
> > its array deduplicated based on nbtree rules/a B-Tree ORDER proc.
>
> Well, it's worse than that: it's assuming that support procedure 1
> is in fact a btree ordering proc in any opclass of an amsearcharray
> AM.  I don't think that's too safe.

Agreed that that's not safe (and wouldn't be even if it was fine to
make aggressive assumptions about amsearcharray index AMs being
nbtree-like).

> We have our hands on enough
> info about the index that we could skip trying to do the sort if
> it's not a btree, and I think we should do so, at least for today.

I think that this boils down to the following (correct me if I'm wrong):

There is a need to remember that we've presorted, rather than making
any kind of hardcoded assumption that that has happened. In practice
we'll always presort with a Const-arg'd ScalarArrayOp nbtree index
qual, but ExecIndexBuildScanKeys has no business knowing about any of
that. This can be achieved by limiting application of the optimization
to nbtree from within createplan.c -- probably by testing
"IndexOptInfo.relam == BTREE_AM_OID" (much like
match_rowcompare_to_indexcol does it already).

Does that summary sound about right? And if it does, do you have any
thoughts on the best place for the new "presorted" struct field?
Should it go in the ScalarArrayOpExpr struct itself?

--
Peter Geoghegan