On Wed, Nov 25, 2020 at 2:43 AM Alexey Kondratov <a.kondra...@postgrespro.ru>
wrote:
> On 2020-11-24 06:52, Bharath Rupireddy wrote:
> > Thanks for the review comments.
> >
> > On Mon, Nov 23, 2020 at 9:57 PM Alexey Kondratov
> > <a.kondra...@postgrespro.ru> wrote:
> >>
> >> > v1-0001-postgres_fdw-function-to-discard-cached-connections.patch
> >>
> >> This patch looks pretty straightforward for me, but there are some
> >> things to be addressed IMO:
> >>
> >> + server = GetForeignServerByName(servername, true);
> >> +
> >> + if (server != NULL)
> >> + {
> >>
> >> Yes, you return a false if no server was found, but for me it worth
> >> throwing an error in this case as, for example, dblink does in the
> >> dblink_disconnect().
> >>
> >
> > dblink_disconnect() "Returns status, which is always OK (since any
> > error causes the function to throw an error instead of returning)."
> > This behaviour doesn't seem okay to me.
> >
> > Since we throw true/false, I would prefer to throw a warning(with a
> > reason) while returning false over an error.
> >
>
> I thought about something a bit more sophisticated:
>
> 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.
>
> That looks like a semantically correct behavior, but let us wait for any
> other opinion.
>
> >
> >>
> >> + result = disconnect_cached_connections(FOREIGNSERVEROID,
> >> + hashvalue,
> >> + false);
> >>
> >> + if (all || (!all && cacheid == FOREIGNSERVEROID &&
> >> + entry->server_hashvalue == hashvalue))
> >> + {
> >> + if (entry->conn != NULL &&
> >> + !all && cacheid == FOREIGNSERVEROID &&
> >> + entry->server_hashvalue == hashvalue)
> >>
> >> These conditions look bulky for me. First, you pass FOREIGNSERVEROID
> >> to
> >> disconnect_cached_connections(), but actually it just duplicates 'all'
> >> flag, since when it is 'FOREIGNSERVEROID', then 'all == false'; when
> >> it
> >> is '-1', then 'all == true'. That is all, there are only two calls of
> >> disconnect_cached_connections(). That way, it seems that we should
> >> keep
> >> only 'all' flag at least for now, doesn't it?
> >>
> >
> > I added cachid as an argument to disconnect_cached_connections() for
> > reusability. Say, someone wants to use it with a user mapping then
> > they can pass cacheid USERMAPPINGOID, hash value of user mapping. The
> > cacheid == USERMAPPINGOID && entry->mapping_hashvalue == hashvalue can
> > be added to disconnect_cached_connections().
> >
>
> Yeah, I have got your point and motivation to add this argument, but how
> we can use it? To disconnect all connections belonging to some specific
> user mapping? But any user mapping is hard bound to some foreign server,
> AFAIK, so we can pass serverid-based hash in this case.
>
> 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.
>
> >
> > v1-0003-postgres_fdw-server-level-option-keep_connection.patch
> > This patch adds a new server level option, keep_connection, default
> > being on, when set to off, the local session doesn't cache the
> > connections associated with the foreign server.
> >
>
> This patch looks good to me, except one note:
>
> (entry->used_in_current_xact &&
> - !keep_connections))
> + (!keep_connections || !entry->keep_connection)))
> {
>
> 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?
>
> >
> >
> v1-0004-postgres_fdw-connection-cache-discard-tests-and-documentation.patch
> > This patch adds the tests and documentation related to this feature.
> >
>
> I have not read all texts thoroughly, but what caught my eye:
>
> + A GUC, <varname>postgres_fdw.keep_connections</varname>, default
> being
> + <literal>on</literal>, when set to <literal>off</literal>, the local
> session
>
> 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...
>
>
A quick thought here.
Would it make sense to add a hook in the DISCARD ALL implementation that
postgres_fdw can register for?
There's precedent here, since DISCARD ALL already has the same effect as
SELECT pg_advisory_unlock_all(); amongst other things.
