> On Fri, Jan 14, 2011 at 1:51 PM, Robert Haas <robertmh...@gmail.com> wrote:
>> On Fri, Jan 14, 2011 at 12:29 PM, Simon Riggs <si...@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 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.
> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
