Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTERUSER MAPPING

On Fri, Jul 21, 2017 at 10:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>> On Fri, Jul 21, 2017 at 10:55 AM, Kyotaro HORIGUCHI
>> <horiguchi.kyotaro@lab.ntt.co.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