On 2016/04/26 21:45, Etsuro Fujita wrote:
While re-reviewing the fix, I noticed that since PQcancel we added to
pgfdw_xact_callback to cancel a DML pushdown query isn't followed by a
ROLLBACK, the connection to the remote server will be discarded at the
end of the while loop in that function, which will cause a FATAL error
of "connection to client lost". Probably, that was proposed by me in
the first version of the patch, but I don't think that's a good idea.
Shouldn't we execute ROLLBACK after that PQcancel?
Another thing I noticed is, ISTM that we miss the case where DML
pushdown queries are performed in subtransactions. I think cancellation
logic would also need to be added to pgfdw_subxact_callback.
Attached is a patch for that.
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/connection.c
--- b/contrib/postgres_fdw/connection.c
***************
*** 677,684 **** pgfdw_xact_callback(XactEvent event, void *arg)
* using an asynchronous execution function, the command
* might not have yet completed. Check to see if a command
* is still being processed by the remote server, and if so,
! * request cancellation of the command; if not, abort
! * gracefully.
*/
if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
{
--- 677,683 ----
* using an asynchronous execution function, the command
* might not have yet completed. Check to see if a command
* is still being processed by the remote server, and if so,
! * request cancellation of the command.
*/
if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
{
***************
*** 694,700 **** pgfdw_xact_callback(XactEvent event, void *arg)
errbuf)));
PQfreeCancel(cancel);
}
- break;
}
/* If we're aborting, abort all remote transactions too */
--- 693,698 ----
***************
*** 798,803 **** pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid,
--- 796,825 ----
{
/* Assume we might have lost track of prepared statements */
entry->have_error = true;
+
+ /*
+ * If a command has been submitted to the remote server by using an
+ * asynchronous execution function, the command might not have yet
+ * completed. Check to see if a command is still being processed by
+ * the remote server, and if so, request cancellation of the
+ * command.
+ */
+ if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
+ {
+ PGcancel *cancel;
+ char errbuf[256];
+
+ if ((cancel = PQgetCancel(entry->conn)))
+ {
+ if (!PQcancel(cancel, errbuf, sizeof(errbuf)))
+ ereport(WARNING,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not send cancel request: %s",
+ errbuf)));
+ PQfreeCancel(cancel);
+ }
+ }
+
/* Rollback all remote subtransactions during abort */
snprintf(sql, sizeof(sql),
"ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
