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

On Fri, Jul 21, 2017 at 10:55 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> At Thu, 20 Jul 2017 18:15:42 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in <18927(dot)1500588942(at)sss(dot)pgh(dot)pa(dot)us>
>> This seems like overkill. We can test it reasonably easily within the
>> existing framework, as shown in the attached patch. I'm also fairly
>
> It checks for a disconnection caused in a single session. I
> thought that its inter-process characteristics is important
> (since I had forgot that in the previous version), but it is
> reasonable enough if we can rely on the fact that it surely works
> through invalidation mechanism.
>
> In shoft, I agree to the test in your patch.
>
>> concerned that what you're showing here would be unstable in the buildfarm
>> as a result of race conditions between the multiple sessions.
>
> Sure. It is what I meant by 'fragile'.
>
>> I made some cosmetic updates to the code patch, as well.
>
> Thank you for leaving the hashvalue staff and revising the comment.
>
> By the way I mistakenly had left the following code in the
> previous patch.
>
> + /* hashvalue == 0 means a cache reset, must clear all state */
> + if (hashvalue == 0)
> + entry->invalidated = true;
> + else if ((cacheid == FOREIGNSERVEROID &&
> + entry->server_hashvalue == hashvalue) ||
> + (cacheid == USERMAPPINGOID &&
> + entry->mapping_hashvalue == hashvalue))
> + entry->invalidated = true;
>
> The reason for the redundancy was that it had used switch-case in
> the else block just before. However, it is no longer
> reasonable. I'd like to change here as the follows.
>
> + /* hashvalue == 0 means a cache reset, must clear all state */
> + if ((hashvalue == 0) ||
> + ((cacheid == FOREIGNSERVEROID &&
> + entry->server_hashvalue == hashvalue) ||
> + (cacheid == USERMAPPINGOID &&
> + entry->mapping_hashvalue == hashvalue)))
> + entry->invalidated = true;
>
> The attached patch differs only in this point.
>

+1. The patch looks good to me.

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