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

On Tue, Apr 11, 2017 at 3:53 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> Apparently, postgres_fdw is trying to store RestrictInfos in the
>> fdw_private field of a ForeignScan node. That won't do; those aren't
>> supposed to be present in a finished plan tree, so there's no readfuncs.c
>> support for them (and we're not adding it).
>
>> Don't know if this is a new bug, or ancient but not previously reachable.
>> It seems to be nearly the inverse of the problem you found yesterday,
>> in which postgres_fdw was stripping RestrictInfos sooner than it really
>> ought to. Apparently that whole business needs a fresh look.
>
> Attached is a proposed patch that cleans up the mess here --- and it was
> a mess. The comments for PgFdwRelationInfo claimed that the remote_conds
> and local_conds fields contained bare clauses, but actually they could
> contain either bare clauses or RestrictInfos or both, depending on where
> the clauses had come from. And there was some seriously obscure and
> undocumented logic around how the fdw_recheck_quals got set up for a
> foreign join node. (BTW, said logic is assuming that no EPQ recheck is
> needed for a foreign join. I commented it to that effect, but I'm not
> very sure that I believe it. If there's a bug there, though, it's
> independent of the immediate problem.)
>
> Anybody want to review this, or shall I just push it?

+ * 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. Attached patch has those comments modified, please check
if that looks ok.

- extract_actual_join_clauses(extra->restrictlist, &joinclauses,
&otherclauses);
+ {
+ /* Grovel through the clauses to separate into two lists */
+ joinclauses = otherclauses = NIL;
+ foreach(lc, extra->restrictlist)
+ {
+ RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);
+
+ if (rinfo->is_pushed_down)
+ otherclauses = lappend(otherclauses, rinfo);
+ else
+ joinclauses = lappend(joinclauses, rinfo);
+ }
+ }
We are duplicating the logic to separate join and other clauses here. But then
we are already using is_pushed_down to separate the clauses at various places
e.g. compute_semi_anti_join_factors(), so probably this change is fine.

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.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company