Thanks for taking a look at the patches. On Fri, Jan 1, 2021 at 9:35 PM Zhihong Yu <z...@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
v6-0001-postgres_fdw-function-to-discard-cached-connectio.patch
Description: Binary data
v6-0002-postgres_fdw-add-keep_connections-GUC-to-not-cach.patch
Description: Binary data
v6-0003-postgres_fdw-server-level-option-keep_connection-.patch
Description: Binary data
