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

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.

- if (!HeapTupleIsValid(tup))
- elog(ERROR, "cache lookup failed for user mapping %u", entry->key);
- umform = (Form_pg_user_mapping) GETSTRUCT(tup);
- server = GetForeignServer(umform->umserver);
- ReleaseSysCache(tup);
+ server = GetForeignServer(entry->serverid);

What about applying only the change about serverid, as a separate patch at
first? This change itself is helpful to get rid of error "cache lookup failed"
in pgfdw_reject_incomplete_xact_state_change(). Patch attached.

+ server = GetForeignServerExtended(entry->serverid, true);

Since the type of second argument in GetForeignServerExtended() is bits16,
it's invalid to specify "true" there?

+ 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.

+ * 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.

+ 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.

+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.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION