On Thu, Jul 14, 2016 at 10:02 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> writes:
> > Exactly, for a rare scenario, should we be penalizing large number of
> > or just continue to use a previously prepared plan when an optimal plan
> > 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.
There is confusion here. The current situation is this:
If at the time of preparing a statement a join can be pushed down to
foreign server, we mark that plan as hasForeignJoin. Before execution, if
user mapping changes (add/modify/drop), all plans with hasForeignJoin are
invalidated. This means the situation you are describing above does not
exist in the current code. So, nothing needs to be fixed.
> 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.
This, as you have proposed, does not solve the problem either. Consider a
case when a public user mapping was associated with the foreign tables
being joined and the join was pushed down while preparing statement and
plan. Before execution, a user mapping was added which changed one of the
table's associated user mapping from public to the new one. Now the join is
not pushable, but the user mapping that was added/changed was not recorded
in invalItems, which contains the public user mapping. An improvement to
your proposal would be to invalidate plans related to a public user
mapping, when any user mapping is added/changed/dropped. But I guess, there
are also some problems there too. Adding/dropping/changing a user mapping
affects other user mappings as well, unlike say doing that to tables or
views. When implementing this, we debated whether it's worth to add that
much complexity. We favoured not adding the complexity that time. The code
as is not optimistic and might lead to using already created suboptimal
plans, but it doesn't have any known bugs there (assuming, I have explained
why the above situation doesn't lead to a bug).
> 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.
Right, that's my argument.
> 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
Yes. That wouldn't have the problem I described above. Again, I am not sure
whether it's worth making code complex for that case. User mappings are not
something added/dropped/modified very frequently.
> 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.
Yes, I agree.
The Postgres Database Company