Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> On Tue, Apr 11, 2017 at 3:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> + * there is no need for EPQ recheck at a join (and Vars or Aggrefs in
> + * the qual might not be available locally anyway).
> I am not sure whether EPQ checks make sense for an upper relation, esp. a
> grouped relation. So mentioning Aggref can be confusing here. For joins, we
> execute EPQ by executing the (so called) outer plan created from fdw_outerpath.
> For this, we fetch whole-row references for the joining relations and build the
> output row by executing the local outer plan attached to the ForeignScanPlan.
> This whole-row references has values for all Vars, so even though Vars are not
> available, corresponding column values are available. So mentioning Vars is
> also confusing here.
Well, my first attempt at fixing this merged remote_conds and remote_exprs
together, which in the previous version of the code resulted in always
passing the remote conditions as fdw_recheck_quals too. And what happened
was that I got "variable not found in subplan target list" errors for Vars
used inside Aggrefs in pushed-to-the-remote HAVING clauses. Which is
unsurprising -- it'd be impossible to return such a Var if the grouping is
being done remotely. So I think it's important for this comment to
explain that we *can't* put upperrel quals into fdw_recheck_quals, not just
that "there's no need to". But pointing out that at a join, there's a
different mechanism that's responsible for EPQ checks is good. I'll
reword this again.
> We can club the code to separate other and join clauses, checking that all join
> clauses are shippable and separating other clauses into local and remote
> clauses in a single list traversal as done in the attached patch.
OK. I was trying to be noninvasive, but this does look more sensible.
I think this might read better if we inverted the test (so that
the outer-join-joinclause-must-be-pushable case would come first).
If we're going for a more-than-minimal patch, I'm inclined to also
move the first loop in postgresGetForeignPlan into the "else" branch,
so that it doesn't get executed in the join/upperrel case. I realize
that it's going to iterate zero times in that case, but it's just
confusing to have it there when we don't expect it to do anything
and we would only throw away the results if it did do anything.
regards, tom lane
