Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

On 2021/01/29 19:45, Bharath Rupireddy wrote:
> On Fri, Jan 29, 2021 at 1:24 PM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>>
>> On Fri, Jan 29, 2021 at 1:17 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>>>> But if the issue is only the inconsistency of test results,
>>>>> we can go with the option (2)? Even with (2), we can make the test
>>>>> stable by removing "valid" column and executing
>>>>> postgres_fdw_get_connections() within the transaction?
>>>>
>>>> Hmmm, and we should have the tests at the start of the file
>>>> postgres_fdw.sql before even we make any foreign server connections.
>>>
>>> We don't need to move the test if we always call postgres_fdw_disconnect_all() just before starting new transaction and calling postgres_fdw_get_connections() as follows?
>>>
>>> SELECT 1 FROM postgres_fdw_disconnect_all();
>>> BEGIN;
>>> ...
>>> SELECT * FROM postgres_fdw_get_connections();
>>> ...
>>
>> Yes, that works, but we cannot show true/false for the
>> postgres_fdw_disconnect_all output.
>>
>> I will post the patch soon. Thanks a lot.
>
> Attaching a patch that has following changes: 1) Now,
> postgres_fdw_get_connections will only return set of active
> connections server names not their valid state 2) The functions
> postgres_fdw_get_connections, postgres_fdw_disconnect and
> postgres_fdw_disconnect_all are now being tested within an explicit
> xact block, this way the tests are more stable even with clobber cache
> always builds.
>
> I tested the patch here on my development system with
> -DCLOBBER_CACHE_ALWAYS configuration, the tests look consistent.
>
> Please review the patch.

Thanks for the patch!

--- Return false as loopback2 connectin is closed already.
-SELECT postgres_fdw_disconnect('loopback2');
- postgres_fdw_disconnect
--------------------------
- f
-(1 row)
-
--- Return an error as there is no foreign server with given name.
-SELECT postgres_fdw_disconnect('unknownserver');
-ERROR: server "unknownserver" does not exist

Why do we need to remove these? These seem to work fine even in
CLOBBER_CACHE_ALWAYS.

+ /*
+ * It doesn't make sense to show this entry in the output with a
+ * NULL server_name as it will be closed at the xact end.
+ */
+ continue;

-1 with this change because I still think that it's more useful to list
all the open connections.

This makes me think that more discussion would be necessary before
changing the interface of postgres_fdw_get_connections(). On the other
hand, we should address the issue ASAP to make the buildfarm member fine.
So at first I'd like to push only the change of regression test.
Patch attached. I tested it both with CLOBBER_CACHE_ALWAYS set and unset,
and the results were stable.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION