On Fri, Oct 2, 2020 at 10:53 AM James Coleman <jtc...@gmail.com> wrote: > > On Fri, Oct 2, 2020 at 10:32 AM Tomas Vondra > <tomas.von...@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
Oh, hmm, could what I said all be true, but there still be some rule that you shouldn't compare datums generated from volatile expressions in different backends (i.e., parallel query)? James
