Fix function_clause on set_succeeded_or_within_failed_transaction

[nadsat]

clause {error, closed} added in function set_succeeded_or_within_failed_transaction

[waisbrot]

Why is the result true instead of pgsql_error:is_in_failed_sql_transaction(Error)?

[nadsat]

According to the definition of is_in_failed_sql_transaction in pgsql_error.erl:
is_in_failed_sql_transaction({pgsql_error, Fields}) ->
{code, <<"25P02">>} =:= lists:keyfind(code, 1, Fields).
{error, closed} wouldn't match. Also the comment on the function set_succeeded_or_within_failed_transaction states that it should always return true.

[waisbrot]

But in the definition of pgsql_error:is_in_failed_sql_transaction/1 we can see that it might not return true... (Also, if set_succeeded_or_within_failed_transaction/1 should always return true then it's spec ought to say that instead of boolean())

But anyhow, I think I see what you're saying: if we're in an aborted transaction then the result of this function is true. You're suggesting that having the connection drop is analogous to the transaction aborting. That makes sense to me; the comment

% If set failed because the transaction was aborted, the query will fail

seems like it should apply just as well to the connection dropping.

I feel convinced that this solution is reasonable. :) Were you able to test this code?

[nadsat]

For the moment I haven't been able to reproduce the exact scenario where it happens

[pguyot]

This code was introduced with commit 94cc7b3.
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.