Always throw errors on failure on critical connection in router executor

[marcocitus]

DESCRIPTION: Fixes a bug that could cause transactions to incorrectly proceed after failure

Fixes #2214

Citus currently doesn't throw an error when a failure occurs in the router executor on a connection that was marked as critical. This can cause transactions to incorrectly proceed after a failure on a connection that's supposed to complete a 2PC due to commands earlier in the transaction. The commit handler is not ready for that situation and panics.

This PR fixes it by checking whether the connection is critical when an error response is returned, or throwing an error immediately if a connection failure occurs.

As a side-effect, this also caused some error messages in queries with CTEs to become more meaningful.

I've also defensively moved the call to CheckTransactionHealth out of CoordinatedRemoteTransactionsCommit because that created a code path where we could throw an error in the commit handler.

[codecov]

Codecov Report

Merging #2215 into master will increase coverage by 0.07%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #2215      +/-   ##
==========================================
+ Coverage   93.64%   93.71%   +0.07%     
==========================================
  Files         103      102       -1     
  Lines       26385    26286      -99     
==========================================
- Hits        24707    24634      -73     
+ Misses       1678     1652      -26

[onderkalaci]

Just FYI: This seems like a good candidate to add a test using the new failure testing framework. We have not checked in the failure testing framework, but, after we merge this PR, it should be easy to add a test via a new PR as #2212

[pykello]

I checked different code paths which called StoreQueryResult() and ConsumeQueryResult() with failOnError = false to see if raising errors in these cases can be problematic.

I couldn't find any logical problems, but I think this hurts the readability a bit, since reader expects no errors to be thrown when this flag is false, but errors can be thrown.

[pykello]

For later: this should be an Assert(). Based on the documentation, PQsetSingleRowMode() should always return 1 if called immediately after PQsendQuery(), otherwise it is a programming error and returns 0.

[pykello]

failOnError is misnamed here, since there are cases that this can be false but we still fail. We should at least update the comments for StoreQueryResult() and ConsumeQueryResult().

[marcocitus]

Note: I did some renaming based on Hadi's feedback, but only the first 3 commits should be backported.