Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit - Mailing list pgsql-hackers
From | Fujii Masao |
---|---|
Subject | Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit |
Date | |
Msg-id | 82debcd7-b236-0fc5-5ff1-5e6b99c59d64@oss.nttdata.com Whole thread Raw |
In response to | Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>) |
Responses |
Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit
|
List | pgsql-hackers |
On 2021/01/07 17:21, Bharath Rupireddy wrote: > On Thu, Jan 7, 2021 at 9:49 AM Fujii Masao <masao.fujii@oss.nttdata.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
pgsql-hackers by date: