Re: PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

On Fri, Jul 21, 2017 at 10:39 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> writes:
>> On Fri, Jul 21, 2017 at 10:55 AM, Kyotaro HORIGUCHI
>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> The attached patch differs only in this point.
>
>> +1. The patch looks good to me.
>
> Pushed with a couple additional changes: we'd all missed that the header
> comment for GetConnection was obsoleted by this change, and the arguments
> for GetSysCacheHashValue really need to be coerced to Datum. (I think
> OID to Datum is the same as what the compiler would do anyway, but best
> not to assume that.)

Thanks and sorry for not noticing the prologue.

>
> Back-patching was more exciting than I could wish. It seems that
> before 9.6, we did not have struct UserMapping storing the OID of the
> pg_user_mapping row it had been made from. I changed GetConnection to
> re-look-up that row and get the OID. But that's ugly, and there's a
> race condition: if user mappings are being added or deleted meanwhile,
> we might locate a per-user mapping when we're really using a PUBLIC
> mapping or vice versa, causing the ConnCacheEntry to be labeled with
> the wrong hash value so that it might not get invalidated properly later.
> Still, it's significantly better than it was, and that corner case seems
> unlikely to get hit in practice --- for one thing, you'd have to then
> revert the mapping addition/deletion before the ConnCacheEntry would be
> found and used again. I don't want to take the risk of modifying struct
> UserMapping in stable branches, so it's hard to see a way to make that
> completely bulletproof before 9.6.

+1.

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