On Thu, Jan 7, 2021 at 9:49 AM Fujii Masao <masao.fu...@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? 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
