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

On Thu, Jan 7, 2021 at 9:49 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> On 2021/01/05 16:56, Bharath Rupireddy wrote:
> > Attaching v8 patch set. Hopefully, cf bot will be happy with v8.
> >
> > Please consider the v8 patch set for further review.
> -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";

Yes we can. In that case, to use the new functions users have to
update postgres_fdw to 1.1, in that case, do we need to mention in the
documentation that to make use of the new functions, update
postgres_fdw to version 1.1?

With the above change, the contents of postgres_fdw--1.0.sql remain as
is and in postgres_fdw--1.0--1.1.sql we will have only the new
function statements:

/* contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql */

-- complain if script is sourced in psql, rather than via ALTER EXTENSION
\echo Use "ALTER EXTENSION postgres_fdw UPDATE TO '1.1'" to load this
file. \quit

CREATE FUNCTION postgres_fdw_get_connections ()
RETURNS text[]
AS 'MODULE_PATHNAME','postgres_fdw_get_connections'
LANGUAGE C STRICT PARALLEL RESTRICTED;

CREATE FUNCTION postgres_fdw_disconnect ()
RETURNS bool
AS 'MODULE_PATHNAME','postgres_fdw_disconnect'
LANGUAGE C STRICT PARALLEL RESTRICTED;

CREATE FUNCTION postgres_fdw_disconnect (text)
RETURNS bool
AS 'MODULE_PATHNAME','postgres_fdw_disconnect'
LANGUAGE C STRICT PARALLEL RESTRICTED;

> +<sect2>
> + <title>Functions</title>
>
> The document format for functions should be consistent with
> that in other contrib module like pgstattuple?

pgstattuple has so many columns to show up in output because of that
they have a table listing all the output columns and their types. The
new functions introduced here have only one or none input and an
output. I think, we don't need a table listing the input and output
names and types.

IMO, we can have something similar to what pg_visibility_map has for
functions, and also an example for each function showing how it can be
used. Thoughts?

> + 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?

Yes, connection's validity can be false only when the connection gets
invalidated and postgres_fdw_get_connections is called within an
explicit txn i.e. begin; commit;. In implicit txn, since the
invalidated connections get closed either during invalidation callback
or at the end of txn, postgres_fdw_get_connections will always show
valid connections. Having said that, I still feel we need the
true/false for valid/invalid in the output of the
postgres_fdw_get_connections, otherwise we might miss giving
connection validity information to the user in a very narrow use case
of explicit txn. If required, we can issue a warning whenever we see
an invalid connection saying "invalid connections connections are
discarded at the end of remote transaction". Thoughts?

> 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?

Yes, for postgres_fdw_get_connections we can return a set of records
of (server_name, valid). To do so, I can refer to dblink_get_pkey.
Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com