Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)

On Wed, Jan 20, 2016 at 4:58 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Mon, Jan 18, 2016 at 6:47 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> > Thanks Thom for bringing it to my notice quickly. Sorry for the same.
> >
> > Here are the patches.
> >
> > 1. pg_fdw_core_v2.patch: changes in core related to user mapping
> handling,
> > GUC
> > enable_foreignjoin
>
> I tried to whittle this patch down to something that I'd be
> comfortable committing and ended up with nothing left.
>
> First, I removed the enable_foreignjoin GUC. I still think an
> FDW-specific GUC is better, and I haven't heard anybody make a strong
> argument the other way. Your argument that this might be inconvenient
> if somebody is using a bunch of join-pushdown-enabled FDWs sounds like
> a strictly theoretical problem, considering how much difficulty we're
> having getting even one FDW to support join pushdown. And if it does
> happen, the user can script it. I'm willing to reconsider this point
> if there is a massive chorus of support for having this be a core
> option rather than an FDW option, but to me it seems that we've gone
> to a lot of trouble to make the system extensible and might as well
> get some benefit from it.
>

Ok. Removed.

>
> Second, I removed the documentation for GetForeignTable(). That
> function is already documented and doesn't need re-documenting.
>

Removed.

>
> Third, I removed GetUserMappingById(). As mentioned in the email to
> which I replied earlier, that doesn't actually produce the same result
> as the way we're doing it now, and might leave the user ID invalid.
>

The comment you quoted was my comment :). I never got a reply from
Hanada-san on that comment. A bit of investigation revealed this: A pushed
down foreign join which involves N foreign tables, might have different
effective userid for each of them.
userid = OidIsValid(rte->checkAsUser) ? rte->checkAsUser : GetUserId()
In such a case, AFAIU, the join will be pushed down only if none of those
have user mapping and there is public user mapping. Is that right? In such
a case, which userid should be stored in UserMapping structure?It might
look like setting GetUserId() as userid in UserMapping is wise, but not
really. All the foreign tables might have different effective userids, each
different from GetUserId() and what GetUserId() would return might have a
user mapping different from the effective userids. What userid should
UserMapping structure have in that case? I thought, that's why Hanada-san
used invalid userid there, so left as it is.

But that has an undesirable effect of setting it to invalid, even in case
of base relation or when all the joining relations have same effective
userid. We may cache "any" of the effective userids of joining relations if
the user mapping matches in build_join_rel() (we will need to add another
field userid in RelOptInfo along with umid). While creating the plan use
this userid to get user mapping. Does that sound good? This clubbed with
the plan cache invalidation should be full proof. We need a way to get user
mapping whether from umid (in which case public user mapping will have -1)
or serverid and some userid.

> Even if that were no issue, it doesn't seem to add anything. The only
> caller of the new function is postgresBeginForeignScan(), and that
> function already has a way of getting the user mapping. The new way
> doesn't seem to be better or faster, so why bother changing it?
>

> At this point, I was down to just the changes to store the user
> mapping ID (umid) in the RelOptInfo, and to consider join pushdown
> only if the user mapping IDs match. One observation I made is that if
> the code to initialize the FDW-related fields were lifted from
> get_relation_info() up to build_simple_rel(), we would not need to use
> planner_rt_fetch(), because the caller already has that information.
> That seems like it might be worth considering. But then I realized a
> more fundamental problem: making the plan depend on the user ID is a
> problem, because the user ID can be changed, and the plan might be
> cached. The same issue arises for RLS, but there is provision for
> that in RevalidateCachedQuery. This patch makes no similar provision.
>

Thanks for the catch. Please see attached patch for a quick fix in
RevalidateCachedQuery(). Are you thinking on similar lines? However, I am
not sure of planUserId - that field actually puzzles me. It's set when the
first time we create a plan and it never changes then. This seems to be a
problem, even for RLS, in following scene

prepare statement using RLS feature
execute statement -- now planUserId is set to say user1
set session role user2;
execute statement - RevalidateCachedQuery() detects that the user has
changed and invalidates the plan and recreates it (including possibly the
query analyze and rewrite). Note planUserId is unchanged here; it's still
user1
reset role; -- now GetUserId() returns user1
execute statement - RevalidateCachedQuery() detects that the planUserId and
GetUserId is same and doesn't invalidate the plan. Looks like it will
execute wrong plan. I am not very familiar with RLS feature, so this
analysis can be completely wrong.

If planUserId is not good for our purpose we can always have a different
field there, say foreignJoinUserId.

There might be another issue here. A user mapping can change and the plan
is cached. Public user mapping was used while planning but before the
(cached) plan was executed again, the user mapping for one of the effective
users involved was added with different credentials. We should expect the
new user mapping to be used for fetching data from foreign tables and thus
join becomes unsafe to be pushed down. If this issue exists we should add
code to invalidate the plan cache, at least the plans which have pushed
down joins.

>
> I think there are two ways forward here. One is to figure out a way
> for the plancache to invalidate queries using FDW join pushdown when
> the user ID changes.

I think this is better since it provides avenue for join pushdown even in
the changed conditions, thus giving better performance if permitted under
the changed conditions.

> The other is to recheck at execution time
> whether the user mapping IDs still match, and if not, fall back to
> using the "backup" plan that we need anyway for EvalPlanQual rechecks.
> This would of course mean that the backup plan would need to be
> something decently efficient, not just whatever we had nearest to
> hand. But that might not be too hard to manage.
>
>

In this case, we will need to create the backup plan even in those cases
where there is no EvalPlanQual involved. I am hesitant to do it that way,
unless we are left with no other option.

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