pgsql icon indicating copy to clipboard operation
pgsql copied to clipboard

Fix function_clause on set_succeeded_or_within_failed_transaction

Open nadsat opened this issue 8 years ago • 1 comments

clause {error, closed} added in function set_succeeded_or_within_failed_transaction

nadsat avatar Sep 19 '17 16:09 nadsat

This code was introduced with commit 94cc7b334cd33ea20e2d3478e5d4fbfc76c715c6. set_succeeded_or_within_failed_transaction/1 was meant to be used in a defensive programming approach to make sure that set timeout and reset timeout statements succeeded, especially in the context of the specific error code for failures within transactions. See the converations in #16 . This is why this function is typed as returning a boolean() and a badmatch occurs if it returns false.

The test for {error, closed} could be done in pgsql_simple_query and pgsql_extended_query instead, before calling set_succeeded_or_within_failed_transaction/1. If the initial set timeout query fails with {error, closed}, this probably should be reported to caller and there is no need to perform the actual query, and if the timeout reset query fails with {error, closed}, the result of the actual query should be returned to caller instead.

However, ignoring {error, closed} as suggested by the proposed change achieves the same result. If the error occurs when setting the timeout, proceeding with the actual query as the socket is already closed will just return {error, closed} as well, which will be returned to caller. If the socket is closed during the query, resetting the timeout will also fail and {error, closed} will be returned to caller. And if the socket is closed after the query, the actual query result will be returned and the state of the socket will be ignored. There already is a comment with set_succeeded_or_within_failed_transaction/1, but judging from this very conversation it needs to be reworked to explain this logic.

Finally, I believe this is reproduceable and could be implemented in a non-regression test where the actual query is long (e.g. pg_sleep()) and the backend is abruptly terminated from another process (with pg_terminate_backend()). This probably triggers a TCP close event and the reset timeout query will fail.

pguyot avatar Sep 19 '17 21:09 pguyot