Re: Error when using array_agg with filter where clause in pg16 and pg17

On Wed, 9 Apr 2025 at 12:25, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> > Unfortunately, the situation is a little worse than what you
> > highlighted, as I think I didn't consider FILTER at all, and this
> > means I didn't consider the costing differences between filtering then
> > sorting vs sorting then filtering.
>
> Oooh. If the FILTER clause is selective, that could easily mean that
> the "optimization" loses big from having to sort many more tuples.
> I wonder if we should just not apply it when there's a FILTER,
> full stop.

Well, technically, that might have been a safer option if we'd caught
this before I committed that patch or before we released that version,
but it might only be a problem if we have to perform a Sort. If the
presorted-ness comes from an Index Scan, then we've not spent any
extra effort sorting tuples that'll be filtered. If we were to switch
the optimisation off for FILTER now, we could cause performance
regressions in the back branches for people who are getting benefits
from Index Scans with a FILTER clause.

The only way I can see to fix that properly is to cost it in during
aggregate planning. IIRC, there's no costing for the implicit sorts in
Aggref. We could add some of those and put a flag in AggPath which
gets propagated to Agg to specify if aggpresorted should be ignored or
not for the given Agg node. We'd have to add_path() for both versions
of the AggPath and let the cheapest Path win.

I suspect we should just leave this for v18 and maybe come back and
improve for v19. There is still SET enable_presorted_aggregate = 0; if
someone stumbles upon this.

David