Re: Oddity in handling of cached plans for FDW queries

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> Exactly, for a rare scenario, should we be penalizing large number of plans
> or just continue to use a previously prepared plan when an optimal plan has
> become available because of changed condition. I would choose second over
> the first as it doesn't make things worse than they are.

You seem to be arguing from a premise that the worst consequence is use of
an inefficient plan.  This is false; the worst consequence is use of a
*wrong* plan, specifically one where a join has been pushed down but doing
so is no longer legal because of a user mapping change.  That is, it was
previously correct to access the two remote tables under the same remote
userID but now they should be accessed under different userIDs.  The
result will be that one or the other remote table is accessed under a
userID that is not what the current user mappings say should be used.
That could lead to either unexpected permissions failures (if the
actually-used userID lacks needed permissions) or security breakdowns
(if the actually-used userID has permissions to do something but the
query should not have been allowed to do it).

I do not think fixing this is optional.

I concur with Etsuro-san's dislike for hasForeignJoin; that flag is
underspecified and doesn't convey nearly enough information.  I do not
think a uses_user_mapping flag is much better.  ISTM what should happen is
that any time we decide to push down a foreign join, we should record the
identity of the common user mapping that made that pushdown possible in
the plan's invalItems list.  That would make it possible to invalidate
only the relevant plans when a user mapping is changed.

I believe what Ashutosh is focusing on is whether, when some user mapping
changes, we should invalidate all plans that could potentially now use a
pushed-down join but did not before.  I tend to agree that that's probably
not something we want to do, as it would be very hard to identify just the
plans likely to benefit.  So we would cause replanning of a lot of queries
that won't actually benefit, and in this direction it is indeed only an
optimization not a correctness matter.

Another way we could attack it would be to record the foreign server OID
as an invalItem for any query that has more than one foreign table
belonging to the same foreign server.  Then, invalidate whenever any user
mapping for that server changes.  This would take care of both the case
where a join pushdown becomes possible and where it stops becoming
possible.  But I'm not sure that the extra invalidations would be
worthwhile.

I'm not excited about Etsuro-san's proposal to record user mapping info
in the plan and skip execution-time mapping lookups altogether.  I think
(1) that's solving a problem that hasn't been proven to be a problem,
(2) it doesn't help unless the FDW code is changed to take advantage of
it, which is unlikely to happen for third-party FDWs, and (3) it opens
the door to a class of new bugs, as any failure to invalidate after a
mapping change would become a security fail even for non-join situations.
        regards, tom lane