Re: Some problems of recovery conflict wait events

On 2020/03/04 14:31, Masahiko Sawada wrote:
> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>
>>
>>
>> On 2020/03/04 13:27, Michael Paquier wrote:
>>> On Wed, Mar 04, 2020 at 01:13:19PM +0900, Masahiko Sawada wrote:
>>>> Yeah, so 0001 patch sets existing wait events to recovery conflict
>>>> resolution. For instance, it sets (PG_WAIT_LOCK | LOCKTAG_TRANSACTION)
>>>> to the recovery conflict on a snapshot. 0003 patch improves these wait
>>>> events by adding the new type of wait event such as
>>>> WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT. Therefore 0001 (and 0002) patch
>>>> is the fix for existing versions and 0003 patch is an improvement for
>>>> only PG13. Did you mean even 0001 patch doesn't fit for back-patching?
>>
>> Yes, it looks like a improvement rather than bug fix.
>>
>
> Okay, understand.
>
>>> I got my eyes on this patch set. The full patch set is in my opinion
>>> just a set of improvements, and not bug fixes, so I would refrain from
>>> back-backpatching.
>>
>> I think that the issue (i.e., "waiting" is reported twice needlessly
>> in PS display) that 0002 patch tries to fix is a bug. So it should be
>> fixed even in the back branches.
>
> So we need only two patches: one fixes process title issue and another
> improve wait event. I've attached updated patches.

Thanks for updating the patches! I started reading 0001 patch.

- /*
- * Report via ps if we have been waiting for more than 500 msec
- * (should that be configurable?)
- */
- if (update_process_title && new_status == NULL &&
- TimestampDifferenceExceeds(waitStart, GetCurrentTimestamp(),
- 500))

The patch changes ResolveRecoveryConflictWithSnapshot() and
ResolveRecoveryConflictWithTablespace() so that they always add
"waiting" into the PS display, whether wait is really necessary or not.
But isn't it better to display "waiting" in PS basically when wait is
necessary, like originally ResolveRecoveryConflictWithVirtualXIDs()
does as the above?

ResolveRecoveryConflictWithDatabase(Oid dbid)
{
+ char *new_status = NULL;
+
+ /* Report via ps we are waiting */
+ new_status = set_process_title_waiting();

In ResolveRecoveryConflictWithDatabase(), there seems no need to
display "waiting" in PS because no wait occurs when recovery conflict
with database happens.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters