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

On Fri, Jul 21, 2017 at 10:55 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> At Thu, 20 Jul 2017 18:15:42 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <18927.1500588942@sss.pgh.pa.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