Re: BUG #15613: Bug in PG Planner for Foreign Data Wrappers

(2019/02/07 4:15), Tom Lane wrote:
> Etsuro Fujita<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
>>>> We could either split the function into two or three functions, or add
>>>> still more overhead to it to notice what kind of relation has been
>>>> passed and adjust its behavior for that. I'm not really thrilled with
>>>> the latter: the fact that it's called create_foreignSCAN_path means,
>>>> to me, that it's not supposed to be used for anything but baserel
>>>> cases.
>
>>> I don't have any strong opinion on that.
>
>> On second thoughts, I think it would be a good idea to split that
>> function, because we can minimize the parameters list passed to each
>> function, making it easy to call that function; as you mentioned,
>> 'required_outer' isn't required for upperrels, and 'fdw_outerpath' isn't
>> required for baserels and upperrels. Not sure we should do that for PG12.
>
> Yeah, I agree. Attached is a draft of that. I've not thought very hard
> about how we would want to manage parameterization of foreign joins, so
> for now create_foreign_join_path doesn't support that. We'll have to
> change its API whenever we do want to support that, which is a good reason
> why it should be separate from create_foreignscan_path: that way we don't
> break API for FDWs that are only implementing plain scans.
>
> I propose that we apply this or something much like it to HEAD, and leave
> the problem of actually working out parameterized foreign joins for later.

Agreed. One thing I noticed is this bit in create_foreign_join_path:

+ /*
+ * Since the path's required_outer should always include all the rel's
+ * lateral_relids, forcibly add those if necessary. This is a bit of a
+ * hack, but up till early 2019 the contrib FDWs failed to ensure that,
+ * and it's likely that the same error has propagated into many external
+ * FDWs. Don't risk modifying the passed-in relid set here.
+ */
+ if (rel->lateral_relids && !bms_is_subset(rel->lateral_relids,
+ required_outer))
+ required_outer = bms_union(required_outer, rel->lateral_relids);

I think this would waste cycles. Do we really need this? How about
just throwing an error instead of doing this, if the joinrel has lateral
references?

Another thing is that it might be better to add regression test cases.
Other than that, the patch looks good to me.

> Not sure whether we should do the same in the back branches. It might be
> fine to decide that we're never going to support parameterized foreign
> joins in the back branches, in which case I think just adding the Assert
> to create_foreignscan_path would be enough there.

I might be missing something, but I don't see any need to support that
in the back branches in future.

Thank you for taking the time to create the patch!

Best regards,
Etsuro Fujita