On 2020/12/24 15:42, Bharath Rupireddy wrote:
On Thu, Dec 24, 2020 at 7:21 AM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
On 2020/12/23 23:40, Bharath Rupireddy wrote:
On Wed, Dec 23, 2020 at 7:31 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
I agree to make pgfdw_xact_callback() close the connection when
entry->invalidated == true. But I think that it's better to get rid of
have_invalid_connections flag and make pgfdw_inval_callback() close
the connection immediately if entry->xact_depth == 0, to avoid unnecessary
scan of the hashtable during COMMIT of transaction not accessing to
foreign servers. Attached is the POC patch that I'm thinking. Thought?
We could do that way as well. It seems okay to me. Now the disconnect
code is spread in pgfdw_inval_callback() and pgfdw_xact_callback().
There's also less burden on pgfdw_xact_callback() to close a lot of
connections on a single commit. The behaviour is like this - If
entry->xact_depth == 0, then the entries wouldn't have got any
connection in the current txn so they can be immediately closed in
pgfdw_inval_callback() and pgfdw_xact_callback() can exit immediately
as xact_got_connection is false. If entry->xact_depth > 0 which means
that probably pgfdw_inval_callback() came from a sub txn, we would
have got a connection in the txn i.e. xact_got_connection becomes true
due to which it can get invalidated in pgfdw_inval_callback() and get
closed in pgfdw_xact_callback() at the end of the txn.
And there's no chance of entry->xact_depth > 0 and xact_got_connection false.
I think we need to change the comment before pgfdw_inval_callback() a bit:
* After a change to a pg_foreign_server or pg_user_mapping catalog entry,
* mark connections depending on that entry as needing to be remade.
* We can't immediately destroy them, since they might be in the midst of
* a transaction, but we'll remake them at the next opportunity.
to
* After a change to a pg_foreign_server or pg_user_mapping catalog entry,
* try to close the cached connections associated with them if they
are not in the
* midst of a transaction otherwise mark them as invalidated. We will
destroy the
* invalidated connections in pgfdw_xact_callback() at the end of the main
xact.
* Closed connections will be remade at the next opportunity.
Yes, I agree that we need to update that comment.
Any thoughts on the earlier point in [1] related to a test case(which
becomes unnecessary with this patch) coverage?
ISTM that we can leave that test as it is because it's still useful to test
the case where the cached connection is discarded in pgfdw_inval_callback().
Thought?
Yes, that test case covers the code this patch adds i.e. closing the
connection in pgfdw_inval_callback() while committing alter foreign
server stmt.
By applying the patch, probably we can get rid of the code to discard
the invalidated cached connection in GetConnection(). But at least for
the back branches, I'd like to leave the code as it is so that we can make
sure that the invalidated cached connection doesn't exist when getting
new connection. Maybe we can improve that in the master, but I'm not
sure if it's really worth doing that against the gain. Thought?
+1 to keep that code as is even after this patch is applied(at least
it works as an assertion that we don't have any leftover invalid
connections). I'm not quite sure, we may need that in some cases, say
if we don't hit disconnect_pg_server() code in pgfdw_xact_callback()
and pgfdw_inval_callback() due to some errors in between. I can not
think of an exact use case though.
Attaching v2 patch that adds the comments and the other test case that
covers disconnecting code in pgfdw_xact_callback. Please review it.
Thanks for updating the patch! It basically looks good to me except
the following minor things.
+ * After a change to a pg_foreign_server or pg_user_mapping catalog entry, try
+ * to close the cached connections associated with them if they are not in the
+ * midst of a transaction otherwise mark them as invalid. We will destroy the
+ * invalidated connections in pgfdw_xact_callback() at the end of the main
+ * xact. Closed connections will be remade at the next opportunity.
Even when we're in the midst of transaction, if that transaction has not used
the cached connections yet, we close them immediately. So, to make the
comment more precise, what about updating the comment as follows?
---------------------
After a change to a pg_foreign_server or pg_user_mapping catalog entry,
close connections depending on that entry immediately if current
transaction has not used those connections yet. Otherwise, mark those
connections as invalid and then make pgfdw_xact_callback() close them
at the end of current transaction, since they cannot be closed in the midst
of a transaction using them. Closed connections will be remade at the next
opportunity if necessary.
---------------------
+ /*
+ * Close the connection if it's not in midst of a xact.
Otherwise
+ * mark it as invalid so that it can be disconnected at
the end of
+ * main xact in pgfdw_xact_callback().
+ */
Because of the same reason as the above, what about updating this comment
as follows?
---------------------
Close the connection immediately if it's not used yet in this transaction.
Otherwise mark it as invalid so that pgfdw_xact_callback() can close it
at the end of this transaction.
---------------------
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
