Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

On Thu, Jan 14, 2021 at 3:52 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> On 2021/01/09 10:12, Bharath Rupireddy wrote:
> > On Fri, Jan 8, 2021 at 9:55 AM Bharath Rupireddy
> > <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >> I will make the changes and post a new patch set soon.
> >
> > Attaching v9 patch set that has addressed the review comments on the
> > disconnect function returning setof records, documentation changes,
> > and postgres_fdw--1.0-1.1.sql changes.
> >
> > Please consider the v9 patch set for further review.
>
> Thanks for updating the patch! I reviewed only 0001 patch.
>
> + /*
> + * Quick exit if the cache has been destroyed in
> + * disconnect_cached_connections.
> + */
> + if (!ConnectionHash)
> + return;
>
> This code is not necessary at least in pgfdw_xact_callback() and
> pgfdw_subxact_callback()? Because those functions check
> "if (!xact_got_connection)" before checking the above condition.

Yes, if xact_got_connection is true, then ConnectionHash wouldn't have
been cleaned up in disconnect_cached_connections. +1 to remove that in
pgfdw_xact_callback and pgfdw_subxact_callback. But we need that check
in pgfdw_inval_callback, because we may reach there after
ConnectionHash is destroyed and set to NULL in
disconnect_cached_connections.

> + server = GetForeignServerExtended(entry->serverid, true);
>
> Since the type of second argument in GetForeignServerExtended() is bits16,
> it's invalid to specify "true" there?

Yeah. I will change it to be something like below:
bits16 flags = FSV_MISSING_OK;
server = GetForeignServerExtended(entry->serverid, flags);

> + if (no_server_conn_cnt > 0)
> + {
> + ereport(WARNING,
> + (errmsg_plural("found an active connection for which the foreign server would have been dropped",
> + "found some active connections for which the foreign servers would have been dropped",
> + no_server_conn_cnt),
> + no_server_conn_cnt > 1 ?
> + errdetail("Such connections are discarded at the end of remote transaction.")
> + : errdetail("Such connection is discarded at the end of remote transaction.")));
>
> At least for me, I like returning such connections with "NULL" in server_name
> column and "false" in valid column, rather than emitting a warning. Because
> which would enable us to count the number of actual foreign connections
> easily by using SQL, for example.

+1. I was also of the similar opinion about this initially. I will change this.

> + * During the first call, we initialize the function context, get the list
> + * of active connections using get_connections and store this in the
> + * function's memory context so that it can live multiple calls.
> + */
> + if (SRF_IS_FIRSTCALL())
>
> I guess that you used value-per-call mode to make the function return
> a set result since you refered to dblink_get_pkey(). But isn't it better to
> use materialize mode like dblink_get_notify() does rather than
> value-per-call because this function returns not so many records? ISTM
> that we can simplify postgres_fdw_get_connections() by using materialize mode.

Yeah. +1 I will change it to use materialize mode.

> + hash_destroy(ConnectionHash);
> + ConnectionHash = NULL;
>
> If GetConnection() is called after ConnectionHash is destroyed,
> it initialize the hashtable and registers some callback functions again
> even though the same function have already been registered. This causes
> same function to be registered as a callback more than once. This is
> a bug.

Yeah, we will register the same callbacks many times. I'm thinking to
have something like below:

static bool conn_cache_destroyed = false;

if (!active_conn_exists)
{
hash_destroy(ConnectionHash);
ConnectionHash = NULL;
conn_cache_destroyed = true;
}

/*
* Register callback functions that manage connection cleanup. This
* should be done just once in each backend. We don't register the
* callbacks again, if the connection cache is destroyed at least once
* in the backend.
*/
if (!conn_cache_destroyed)
{
RegisterXactCallback(pgfdw_xact_callback, NULL);
RegisterSubXactCallback(pgfdw_subxact_callback, NULL);
CacheRegisterSyscacheCallback(FOREIGNSERVEROID,
pgfdw_inval_callback, (Datum) 0);
CacheRegisterSyscacheCallback(USERMAPPINGOID,
pgfdw_inval_callback, (Datum) 0);
}

Thoughts?

> +CREATE FUNCTION postgres_fdw_disconnect ()
>
> Do we really want postgres_fdw_disconnect() with no argument?
> IMO postgres_fdw_disconnect() with the server name specified is enough.
> But I'd like to hear the opinion about that.

IMO, we should have that. Though a bit impractical use case, if we
have many connections which are not being used and want to disconnect
them at once, this function will be useful.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com