Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs) - Mailing list pgsql-hackers
| From | Ashutosh Bapat |
|---|---|
| Subject | Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs) |
| Date | |
| Msg-id | CAFjFpRd+xfod6NeqoVB=cWu2dqM4nNOpjU8dAqnHi6sJThZ=+g@mail.gmail.com Whole thread Raw |
| In response to | Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs) (Robert Haas <robertmhaas@gmail.com>) |
| Responses |
Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)
(Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>)
Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs) (Robert Haas <robertmhaas@gmail.com>) |
| List | pgsql-hackers |
On Wed, Jan 20, 2016 at 4:58 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 18, 2016 at 6:47 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.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()
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.
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
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
pgsql-hackers by date: