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

On 2021/01/07 17:21, Bharath Rupireddy wrote:
> 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?

But since postgres_fdw.control indicates that the default version is 1.1,
"CREATE EXTENSION postgres_fdw" installs v1.1. So basically the users
don't need to update postgres_fdw from v1.0 to v1.1. Only the users of
v1.0 need to update that to v1.1 to use new functions. No?

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

Yes.

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

Sounds good.

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

Understood. I withdraw my suggestion and am fine to display
valid/invalid information.

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

IMO it's overkill to emit such warinng message because that
situation is normal one. OTOH, it seems worth documenting that.

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

Yes.

Regards,

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