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

On Thu, Jan 21, 2016 at 12:47 AM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>> Well, we kind of need to get it right, not just be guessing.
>>
>> It looks to me as though GetConnection() only uses the user ID as a
>> cache lookup key. What it's trying to do is ensure that if user A and
>> user B need different user mapping options, we don't use the same
>> connection for both. So, actually, passing InvalidOid seems like it's
>> not really a problem here. It's arguably more correct than what we've
>> been doing up until now, since it means we cache the connection under
>> user OID whose options we used, rather than the user OID that asked
>> about the options.
>
> In that case, do we need GetUserMappingById changes in that patch and not
> pull it out. If we are keeping those changes, I need some clarification
> about your comment
>
>> 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?
>
> In pg_fdw_join_v2.patch, postgresBeginForeignScan() obtained user mapping
> using GetUserMappingById() instead of the earlier way of fetching it by
> userid and serverid. So even that change will remain, right?

In light of the investigation which led to the first comment you
quoted, I withdraw the second one (which I think I actually made
chronologically prior to the first one). I'm not exactly sure what
needs to happen here, but it seems to me that maybe the connection
cache should be changed to be keyed by user mapping OID. That seems
like it would address the problem you mention here:

http://www.postgresql.org/message-id/CAFjFpRf-LiD5bai4D6cSUseJh=xxJqipo_vN8mTnZG16TMWJ-w@mail.gmail.com

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company