On Wed, Nov 25, 2020 at 12:13 AM Alexey Kondratov <a.kondra...@postgrespro.ru> wrote: > > 1) Return 'true' if there were open connections and we successfully > closed them. > 2) Return 'false' in the no-op case, i.e. there were no open > connections. > 3) Rise an error if something went wrong. And non-existing server case > belongs to this last category, IMO. >
Done this way. > > I am not sure, but I think, that instead of adding this additional flag > into ConnCacheEntry structure we can look on entry->xact_depth and use > local: > > bool used_in_current_xact = entry->xact_depth > 0; > > for exactly the same purpose. Since we set entry->xact_depth to zero at > the end of xact, then it was used if it is not zero. It is set to 1 by > begin_remote_xact() called by GetConnection(), so everything seems to be > fine. > Done. > > In the case of pgfdw_inval_callback() this argument makes sense, since > syscache callbacks work that way, but here I can hardly imagine a case > where we can use it. Thus, it still looks as a preliminary complication > for me, since we do not have plans to use it, do we? Anyway, everything > seems to be working fine, so it is up to you to keep this additional > argument. > Removed the cacheid variable. > > Following this logic: > > 1) If keep_connections == true, then per-server keep_connection has a > *higher* priority, so one can disable caching of a single foreign > server. > > 2) But if keep_connections == false, then it works like a global switch > off indifferently of per-server keep_connection's, i.e. they have a > *lower* priority. > > It looks fine for me, at least I cannot propose anything better, but > maybe it should be documented in 0004? > Done. > > I think that GUC acronym is used widely only in the source code and > Postgres docs tend to do not use it at all, except from acronyms list > and a couple of 'GUC parameters' collocation usage. And it never used in > a singular form there, so I think that it should be rather: > > A configuration parameter, > <varname>postgres_fdw.keep_connections</varname>, default being... > Done. > > The whole paragraph is really difficult to follow. It could be something > like that: > > <para> > Note that setting <varname>postgres_fdw.keep_connections</varname> > to > off does not discard any previously made and still open > connections immediately. > They will be closed only at the end of a future transaction, which > operated on them. > > To close all connections immediately use > <function>postgres_fdw_disconnect</function> function. > </para> > Done. Attaching the v2 patch set. Please review it further. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
v2-0001-postgres_fdw-connection-cache-disconnect-function.patch
Description: Binary data
v2-0004-postgre_fdw-connection-cache-discard-tests-and-doc.patch
Description: Binary data
v2-0002-postgres_fdw-add-keep_connections-GUC-to-not-cache.patch
Description: Binary data
v2-0003-postgres_fdw-server-level-option-keep_connection.patch
Description: Binary data
