Re: [HACKERS] [sqlsmith] ERROR: badly formatted node string "RESTRICTINFO...

On Tue, Apr 11, 2017 at 8:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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".

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