Re: enable_incremental_sort changes query behavior

On Fri, Oct 2, 2020 at 10:32 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> On Fri, Oct 02, 2020 at 09:19:44AM -0400, James Coleman wrote:
> >
> > ...
> >
> >I've been able to confirm that the problem goes away if we stop adding
> >the gather merge paths in generate_useful_gather_paths().
> >
> >I'm not sure yet what conclusion that leads us to. It seems to be that
> >the biggest clue remains that all of this works correctly unless one
> >of the selected columns (which happens to be a pathkey at this point
> >because it's a DISTINCT query) contains a volatile expression.
> >
>
> Yeah. It seems to me this is a bug in get_useful_pathkeys_for_relation,
> which is calling find_em_expr_for_rel and is happy with anything it
> returns. But this was copied from postgres_fdw, which however does a bit
> more here:
>
>      if (pathkey_ec->ec_has_volatile ||
>          !(em_expr = find_em_expr_for_rel(pathkey_ec, rel)) ||
>          !is_foreign_expr(root, rel, em_expr))
>
> So not only does it explicitly check volatility of the pathkey, it also
> calls is_foreign_expr which checks the expression for mutable functions.
>
> The attached patch seems to fix this, but it only adds the check for
> mutable functions. Maybe it should check ec_has_volatile too ...

We actually discussed the volatility check in that function back on
the original thread [1], and we'd concluded that was specifically
necessary for the fdw code because the function would execute on two
different servers (and thus produce different results), but that in a
local server only scenario it should be fine.

My understanding (correct me if I'm wrong) is that the volatile
function should only be executed once (at the scan level?) to build
the tuple and from then on the datum isn't going to change, so I'm not
sure why the volatility would matter here.

James

1: https://www.postgresql.org/message-id/20200328025830.6v6ogkseohakc32q%40development