On Tue, Apr 11, 2017 at 8:11 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> writes: >> On Tue, Apr 11, 2017 at 3:53 AM, Tom Lane <t...@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".
The comments in the committed versions are good. > 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). Ok. In fact, thinking more about it, we should probably test JOIN_INNER explicitly instead of !IS_OUTER_JOIN() since SEMI_JOINS are not considered as outer joins and I am not sure how would we be treating the quals for those. > > 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. > Committed version looks quite clear. I noticed that you committed the patch, while I was writing this mail. Sending it anyway. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
