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

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.

> I think this is actually a bug fix, and should not wait for the next
> commitfest.

Agreed.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center