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

On 2021/01/05 16:56, Bharath Rupireddy wrote:
> On Sat, Jan 2, 2021 at 11:19 AM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>> I'm sorry for the mess. I missed adding the new files into the v6-0001
>> patch. Please ignore the v6 patch set and consder the v7 patch set for
>> further review. Note that 0002 and 0003 patches have no difference
>> from v5 patch set.
>
> It seems like cf bot was failing on v7 patches. On Linux, it fails
> while building documentation in 0001 patch, I corrected that. On
> FreeBSD, it fails in one of the test cases I added, since it was
> unstable, I corrected it now.
>
> Attaching v8 patch set. Hopefully, cf bot will be happy with v8.
>
> Please consider the v8 patch set for further review.

Thanks for the patch!

-DATA = postgres_fdw--1.0.sql
+DATA = postgres_fdw--1.1.sql postgres_fdw--1.0--1.1.sql

Shouldn't we leave 1.0.sql as it is and create 1.0--1.1.sql so that
we can run the followings?

CREATE EXTENSION postgres_fdw VERSION "1.0";
ALTER EXTENSION postgres_fdw UPDATE TO "1.1";

+<sect2>
+ <title>Functions</title>

The document format for functions should be consistent with
that in other contrib module like pgstattuple?

+ When called in the local session, it returns an array with each element as a
+ pair of the foreign server names of all the open connections that are
+ previously made to the foreign servers and <literal>true</literal> or
+ <literal>false</literal> to show whether or not the connection is valid.

We thought that the information about whether the connection is valid or
not was useful to, for example, identify and close explicitly the long-living
invalid connections because they were useless. But thanks to the recent
bug fix for connection leak issue, that information would be no longer
so helpful for us? False is returned only when the connection is used in
this local transaction but it's marked as invalidated. In this case that
connection cannot be explicitly closed because it's used in this transaction.
It will be closed at the end of transaction. Thought?

I guess that you made postgres_fdw_get_connections() return the array
because the similar function dblink_get_connections() does that. But
isn't it more convenient to make that return the set of records?

Regards,

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