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

Thanks for taking a look at the patches.

On Fri, Jan 1, 2021 at 9:35 PM Zhihong Yu <zyu@yugabyte.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