Re: Some problems of recovery conflict wait events

On Tue, 10 Mar 2020 at 00:57, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/03/09 14:10, Masahiko Sawada wrote:
> > On Mon, 9 Mar 2020 at 13:24, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>
> >>
> >>
> >> On 2020/03/08 13:52, Masahiko Sawada wrote:
> >>> On Thu, 5 Mar 2020 at 20:16, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2020/03/05 16:58, Masahiko Sawada wrote:
> >>>>> On Wed, 4 Mar 2020 at 15:21, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2020/03/04 14:31, Masahiko Sawada wrote:
> >>>>>>> On Wed, 4 Mar 2020 at 13:48, Fujii Masao <masao.fujii@oss.nttdata.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.
> >>>>>
> >>>>> Thank you for reviewing this 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?
> >>>>>
> >>>>> You're right. Will fix it.
> >>>>>
> >>>>>>
> >>>>>>      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.
> >>>>>
> >>>>> Isn't the startup process waiting for other backend to terminate?
> >>>>
> >>>> Yeah, you're right. I agree that "waiting" should be reported in this case.
> >>>>
> >>>> Currently ResolveRecoveryConflictWithLock() and
> >>>> ResolveRecoveryConflictWithBufferPin() don't call
> >>>> ResolveRecoveryConflictWithVirtualXIDs and don't report "waiting"
> >>>> in PS display. You changed them so that they report "waiting". I agree
> >>>> to have this change. But this change is an improvement rather than
> >>>> a bug fix, i.e., we should apply this change only in v13?
> >>>>
> >>>
> >>> Did you mean ResolveRecoveryConflictWithDatabase and
> >>> ResolveRecoveryConflictWithBufferPin?
> >>
> >> Yes! Sorry for my typo.
> >>
> >>> In the current code as far as I
> >>> researched there are two cases where we don't add "waiting" and one
> >>> case where we doubly add "waiting".
> >>>
> >>> ResolveRecoveryConflictWithDatabase and
> >>> ResolveRecoveryConflictWithBufferPin don't update the ps title.
> >>> Although the path where GetCurrentTimestamp() >= ltime is false in
> >>> ResolveRecoveryConflictWithLock also doesn't update the ps title, it's
> >>> already updated in WaitOnLock. On the other hand, the path where
> >>> GetCurrentTimestamp() >= ltime is true in
> >>> ResolveRecoveryConflictWithLock updates the ps title but it's wrong as
> >>> I reported.
> >>>
> >>> I've split the patch into two patches: 0001 patch fixes the issue
> >>> about doubly updating ps title, 0002 patch makes the recovery conflict
> >>> resolution on database and buffer pin update the ps title.
> >>
> >> Thanks for splitting the patches. I think that 0001 patch can be back-patched.
> >>
> >> -                       /*
> >> -                        * 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))
> >>
> >> Originally, "waiting" is reported in PS if we've been waiting for more than
> >> 500 msec, as the above does. But you got rid of those codes in the patch.
> >> Did you confirm that it's safe to do that? If not, isn't it better to apply
> >> the attached patch?
> >
> > In WaitOnLock() we update the ps title regardless of waiting time. So
> > I thought we can change it to make these behavior consistent. But
> > considering back-patch, your patch looks better than mine.
>
> Yeah, so I pushed the 0001 patch at first!
> I will review the other patches later.

Thank you!

For 0002 patch which makes ResolveRecoveryConflictWithDatabase and
ResolveRecoveryConflictWithBufferPin update the ps title, I think
these are better to wait for 5ms before updating the ps title like
ResolveRecoveryConflictWithVirtualXIDs, for consistency among recovery
conflict resolution functions, but what do you think?

Regards,


--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services