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?
--
Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
