On Wed, Jan 20, 2021 at 6:58 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
+ * It checks if the cache has a connection for the given foreign server that's
+ * not being used within current transaction, then returns true. If the
+ * connection is in use, then it emits a warning and returns false.
The comment also should mention the case where no open connection
for the given server is found? What about rewriting this to the following?
---------------------
If the cached connection for the given foreign server is found and has not
been used within current transaction yet, close the connection and return
true. Even when it's found, if it's already used, keep the connection, emit
a warning and return false. If it's not found, return false.
---------------------
Done.
+ * It returns true, if it closes at least one connection, otherwise false.
+ *
+ * It returns false, if the cache doesn't exit.
The above second comment looks redundant.
Yes. "otherwise false" means it.
+ if (ConnectionHash)
+ result = disconnect_cached_connections(0, true);
Isn't it smarter to make disconnect_cached_connections() check
ConnectionHash and return false if it's NULL? If we do that, we can
simplify the code of postgres_fdw_disconnect() and _all().
Done.
+ * current transaction are disconnected. Otherwise, the unused entries with the
+ * given hashvalue are disconnected.
In the above second comment, a singular form should be used instead?
Because there must be no multiple entries with the given hashvalue.
Rephrased the function comment a bit. Mentioned the _disconnect and
_disconnect_all in comments because we have said enough what each of
those two functions do.
+/*
+ * Workhorse to disconnect cached connections.
+ *
+ * This function disconnects either all unused connections when called from
+ * postgres_fdw_disconnect_all or a given foreign server unused connection when
+ * called from postgres_fdw_disconnect.
+ *
+ * This function returns true if at least one connection is disconnected,
+ * otherwise false.
+ */
+ server = GetForeignServer(entry->serverid);
This seems to cause an error "cache lookup failed"
if postgres_fdw_disconnect_all() is called when there is
a connection in use but its server is dropped. To avoid this error,
GetForeignServerExtended() with FSV_MISSING_OK should be used
instead, like postgres_fdw_get_connections() does?
+1. So, I changed it to GetForeignServerExtended, added an assertion
for invalidation just like postgres_fdw_get_connections. I also added
a test case for this, we now emit a slightly different warning for
this case alone that is (errmsg("cannot close dropped server
connection because it is still in use")));. This warning looks okay as
we cannot show any other server name in the ouput and we know that
this rare case can exist when someone drops the server in an explicit
transaction.
+ if (entry->server_hashvalue == hashvalue &&
+ (entry->xact_depth > 0 || result))
+ {
+ hash_seq_term(&scan);
+ break;
entry->server_hashvalue can be 0? If yes, since postgres_fdw_disconnect_all()
specifies 0 as hashvalue, ISTM that the above condition can be true
unexpectedly. Can we replace this condition with just "if (!all)"?
I don't think so entry->server_hashvalue can be zero, because
GetSysCacheHashValue1/CatalogCacheComputeHashValue will not return 0
as hash value. I have not seen someone comparing hashvalue with an
expectation that it has 0 value, for instance see if (hashvalue == 0
|| riinfo->oidHashValue == hashvalue).
Having if(!all) something like below there doesn't suffice because we
might call hash_seq_term, when some connection other than the given
foreign server connection is in use.
