Re: Error code for "terminating connection due to conflict with recovery"

On Fri, Jan 14, 2011 at 1:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jan 14, 2011 at 12:29 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> This whole thing is confused. No change is appropriate here at all.
>>
>> We issue ERRCODE_T_R_SERIALIZATION_FAILURE almost all of the time for
>> recovery conflicts.
>>
>> We issue ERRCODE_ADMIN_SHUTDOWN only if the conflict is non-retryable,
>> which occurs if someone drops the database that the user was connected
>> to when they get kicked off. That code exists specifically to inform the
>> user that they *cannot* reconnect. So pgpool should not be trying to
>> trap that error and reconnect.
>
> CheckRecoveryConflictDeadlock() uses ERRCODE_QUERY_CANCELLED.
> ProcessInterrupts() sometimes uses ERRCODE_T_R_SERIALIZATION_FAILURE
> and sometimes uses ERRCODE_ADMIN_SHUTDOWN.  It seems to me that it
> wouldn't be a bad thing to be a bit more consistent, and perhaps to
> have dedicated error codes for recovery conflicts.  This bit strikes
> me as particularly strange:
>
>                else if (RecoveryConflictPending && RecoveryConflictRetryable)
>                {
>                        pgstat_report_recovery_conflict(RecoveryConflictReason);
>                        ereport(FATAL,
>
> (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
>                          errmsg("terminating connection due to
> conflict with recovery"),
>                                         errdetail_recovery_conflict()));
>                }
>                else if (RecoveryConflictPending)
>                {
>                        pgstat_report_recovery_conflict(RecoveryConflictReason);
>                        ereport(FATAL,
>                                        (errcode(ERRCODE_ADMIN_SHUTDOWN),
>                          errmsg("terminating connection due to
> conflict with recovery"),
>                                         errdetail_recovery_conflict()));
>                }
>
> That's the same error message at the same severity level with two
> different SQLSTATEs depending on RecoveryConflictRetryable.  Seems a
> bit cryptic.

So what we do want to do about this?

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'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.

It's no longer clear to me that we actually need a new error code for
this - using the same one everywhere seems like it might be
sufficient, unless someone wants to make an argument why it isn't.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company