On Jan21, 2011, at 10:16 , Simon Riggs wrote:
> On Fri, 2011-01-21 at 13:49 +0900, Tatsuo Ishii wrote:
>>
>>> I'm pretty well convinced that we should NOT be issuing
>>> ERRCODE_ADMIN_SHUTDOWN for a recovery conflict, but that could be
>>> fixed by a trivial simplification of the code posted above, without
>>> introducing any new error code.
>>
>> I agree with ERRCODE_ADMIN_SHUTDOWN should not be used for a recovery
>> conflict. And if your proposal does not need to introduce new error
>> code, I also agree with not inventing new error code.
>
>>> I'd also be in favor of changing the one that uses
>>> ERRCODE_QUERY_CANCELLED to use ERRCODE_T_R_SERIALIZATION_FAILURE, as
>>> the former might be taken to imply active user intervention, and for
>>> consistency.
>>
>> +1.
>
> We already use ERRCODE_T_R_SERIALIZATION_FAILURE for retryable errors,
> which is almost every error. So no change required there.
>
> ERRCODE_ADMIN_SHUTDOWN is used only in situations where we cannot
> reconnect or retry because the database we said we wished to connect to
> no longer exists. That needs to be a different error code to a normal,
> retryable error, so that pgpool can tell the difference between things
> it can help with and things it cannot help with.
Yeah. Clients absolutely need to be able to distinguish transient and
permanent errors. Otherwise, how would a client know when to retry
a transaction (as he needs to in case of a serialization anomaly) and
when to report the error to the user?
ERRCODE_T_R_SERIALIZATION_FAILURE and ERRCODE_T_R_DEADLOCK_DETECTED
are probably both assumed to be transient failure by client aready. So
we should use those two for transient recovery conflicts (i.e. those
which go away if you retry) and something else for the others (like
database dropped)
This'd mean that the code is fine as it is, except that we should
raise ERRCODE_T_R_DEADLOCK_DETECTED instead of ERRCODE_QUERY_CANCELED
in CheckRecoveryConflictDeadlock(). I might be missing something though -
Simon, what were your reasons for using ERRCODE_QUERY_CANCELED there?
best regards,
Florian Pflug
