postgres_fdw: misplaced? comments in connection.c
Hi,
The comments for pgfdw_get_cleanup_result() say this:
* It's not a huge problem if we throw an ERROR here, but if we get into error
* recursion trouble, we'll end up slamming the connection shut, which will
* necessitate failing the entire toplevel transaction even if subtransactions
* were used. Try to use WARNING where we can.
But we don’t use WARNING anywhere in that function. The right place
for this is pgfdw_exec_cleanup_query()?
Best regards,
Etsuro Fujita
On Mon, Oct 11, 2021 at 5:05 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
The comments for pgfdw_get_cleanup_result() say this:
* It's not a huge problem if we throw an ERROR here, but if we get into error
* recursion trouble, we'll end up slamming the connection shut, which will
* necessitate failing the entire toplevel transaction even if subtransactions
* were used. Try to use WARNING where we can.But we don’t use WARNING anywhere in that function. The right place
for this is pgfdw_exec_cleanup_query()?
I noticed that pgfdw_cancel_query(), which is called during (sub)abort
cleanup if necessary, also uses WARNING, instead of ERROR, to avoid
the error-recursion-trouble issue. So I think it would be good to
move this to pgfdw_cancel_query() as well as
pgfdw_exec_cleanup_query(). Attached is a patch for that.
Best regards,
Etsuro Fujita
Attachments:
move-misplaced-comments.patchapplication/octet-stream; name=move-misplaced-comments.patchDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index ce0164e9d2..4aff315b7c 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -1137,6 +1137,11 @@ pgfdw_reject_incomplete_xact_state_change(ConnCacheEntry *entry)
* and ignore the result. Returns true if we successfully cancel the query
* and discard any pending result, and false if not.
*
+ * It's not a huge problem if we throw an ERROR here, but if we get into error
+ * recursion trouble, we'll end up slamming the connection shut, which will
+ * necessitate failing the entire toplevel transaction even if subtransactions
+ * were used. Try to use WARNING where we can.
+ *
* XXX: if the query was one sent by fetch_more_data_begin(), we could get the
* query text from the pendingAreq saved in the per-connection state, then
* report the query using it.
@@ -1187,6 +1192,11 @@ pgfdw_cancel_query(PGconn *conn)
* If the query is executed successfully but returns an error, the return
* value is true if and only if ignore_errors is set. If the query can't be
* sent or times out, the return value is false.
+ *
+ * It's not a huge problem if we throw an ERROR here, but if we get into error
+ * recursion trouble, we'll end up slamming the connection shut, which will
+ * necessitate failing the entire toplevel transaction even if subtransactions
+ * were used. Try to use WARNING where we can.
*/
static bool
pgfdw_exec_cleanup_query(PGconn *conn, const char *query, bool ignore_errors)
@@ -1233,11 +1243,6 @@ pgfdw_exec_cleanup_query(PGconn *conn, const char *query, bool ignore_errors)
* be a query that was initiated as part of transaction abort to get the remote
* side back to the appropriate state.
*
- * It's not a huge problem if we throw an ERROR here, but if we get into error
- * recursion trouble, we'll end up slamming the connection shut, which will
- * necessitate failing the entire toplevel transaction even if subtransactions
- * were used. Try to use WARNING where we can.
- *
* endtime is the time at which we should give up and assume the remote
* side is dead. Returns true if the timeout expired, otherwise false.
* Sets *result except in case of a timeout.
On Tue, Oct 12, 2021 at 1:33 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
On Mon, Oct 11, 2021 at 5:05 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
The comments for pgfdw_get_cleanup_result() say this:
* It's not a huge problem if we throw an ERROR here, but if we get into error
* recursion trouble, we'll end up slamming the connection shut, which will
* necessitate failing the entire toplevel transaction even if subtransactions
* were used. Try to use WARNING where we can.But we don’t use WARNING anywhere in that function. The right place
for this is pgfdw_exec_cleanup_query()?I noticed that pgfdw_cancel_query(), which is called during (sub)abort
cleanup if necessary, also uses WARNING, instead of ERROR, to avoid
the error-recursion-trouble issue. So I think it would be good to
move this to pgfdw_cancel_query() as well as
pgfdw_exec_cleanup_query(). Attached is a patch for that.
There seems to be no objections, so I have applied the patch.
Best regards,
Etsuro Fujita