From: | Tatsuo Ishii <ishii(at)postgresql(dot)org> |
---|---|
To: | robertmhaas(at)gmail(dot)com |
Cc: | simon(at)2ndquadrant(dot)com, tgl(at)sss(dot)pgh(dot)pa(dot)us, ishii(at)postgresql(dot)org, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Error code for "terminating connection due to conflict with recovery" |
Date: | 2011-01-21 04:49:04 |
Message-ID: | 20110121.134904.645538350615349245.t-ishii@sraoss.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> On Fri, Jan 14, 2011 at 1:51 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Jan 14, 2011 at 12:29 PM, Simon Riggs <simon(at)2ndquadrant(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Tatsuo Ishii | 2011-01-21 04:56:45 | Re: How to know killed by pg_terminate_backend |
Previous Message | Fujii Masao | 2011-01-21 04:43:29 | Re: bug in SignalSomeChildren |