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

Thanks for taking a look at the patches.

On Fri, Jan 1, 2021 at 9:35 PM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
> Happy new year.
>
> + appendStringInfo(&buf, "(%s, %s)", server->servername,
> + entry->invalidated ? "false" : "true");
>
> Is it better to use 'invalidated' than 'false' in the string ?

This point was earlier discussed in [1] and [2], but the agreement was
on having true/false [2] because of a simple reason specified in [1],
that is when some users have foreign server names as invalid or valid,
then the output is difficult to interpret which one is what. With
having true/false, it's easier. IMO, let's keep the true/false as is,
since it's also suggested in [2].

[1] - https://www.postgresql.org/message-id/CALj2ACUv%3DArQXs0U9PM3YXKCeSzJ1KxRokDY0g_0aGy--kDScA%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/6da38393-6ae5-4d87-2690-11c932123403%40oss.nttdata.com

> For the first if block of postgres_fdw_disconnect():
>
> + * Check if the connection associated with the given foreign server is
> + * in use i.e. entry->xact_depth > 0. Since we can not close it, so
> + * error out.
> + */
> + if (is_in_use)
> + ereport(WARNING,
>
> since is_in_use is only set in the if (server) block, I think the above warning can be moved into that block.

Modified that a bit. Since we error out when no server object is
found, then no need of keeping the code in else part. We could save on
some indentation

+ if (!server)
+ ereport(ERROR,
+ (errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
+ errmsg("foreign server \"%s\" does not exist",
servername)));
+
+ hashvalue = GetSysCacheHashValue1(FOREIGNSERVEROID,
+ ObjectIdGetDatum(server->serverid));
+ result = disconnect_cached_connections(hashvalue, false, &is_in_use);
+
+ /*
+ * Check if the connection associated with the given foreign server is
+ * in use i.e. entry->xact_depth > 0. Since we can not close it, so
+ * error out.
+ */
+ if (is_in_use)
+ ereport(WARNING,
+ (errmsg("cannot close the connection because it
is still in use")));

Attaching v6 patch set. Please have a look.

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