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

I missed the example plan cache revalidation patch in the previous mail.
Sorry. Here it is.

On Wed, Jan 20, 2016 at 7:20 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

>
>
> 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
>

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