On 2020/11/27 11:12, Bharath Rupireddy wrote:
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.
Regarding the 0001 patch, we should add the function that returns
the information of cached connections like dblink_get_connections(),
together with 0001 patch? Otherwise it's not easy for users to
see how many cached connections are and determine whether to
disconnect them or not. Sorry if this was already discussed before.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
