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

On Wed, 9 Apr 2025 at 03:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Bisecting fingers this commit:
>
> 1349d2790bf48a4de072931c722f39337e72055e is the first bad commit
> commit 1349d2790bf48a4de072931c722f39337e72055e
> Author: David Rowley <drowley@postgresql.org>
> Date:   Tue Aug 2 23:11:45 2022 +1200
>
>     Improve performance of ORDER BY / DISTINCT aggregates

:-(

> Fortunately, that commit didn't actually rip out the old code path.
> The simplest fix I can think of is to disable the presorted-agg
> optimization if (1) there's a FILTER clause and (2) the proposed
> sort key is anything more complex than a Var.  There might be
> some wiggle room in (2) -- for instance, RelabelType(Var) should
> be safe -- but we don't have a lot of intelligence about which
> expression types are guaranteed error-free.

Yeah, I can't think of any other fix.

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.  That commit does assume it's
always better to sort first when we can, which in many cases, that's
going to be true, e.g. when an index provides presorted input or when
the presorted sort order serves multiple aggregates. The non-presorted
path has to perform multiple identical sorts for the latter.

I've attached an experimental patch to fix the reported bug which
works by first checking for Vars and Consts, then falls back on
passing the Expr through strip_implicit_coercions() and trying again
to see if Vars and Consts were found. Simple tests show this seems to
work ok, but it does cause the expected results to change with the
sqljson tests.  The new results seem fine as the reason that query no
longer uses the presorted aggregate is because of the RowExpr in
"JSON_ARRAYAGG(foo ORDER BY bar) FILTER (WHERE bar > 2) as
row_filtered_agg", which causes no presortable Aggrefs to be found,
which results in the Aggref having to perform the sort.  That means
all the other non ORDER BY aggrefs give the results as per the order
of their inputs.

I wonder if we could work harder for RowExprs.
strip_implicit_coercions() seems to not want to handle them and even
mentions that fact in a comment. I don't have a grasp on why that is
or if it can be done by doing more work looking at each RowExpr.arg.

David