Re: Re: Use procsignal_sigusr1_handler and RecoveryConflictInterrupt() from walsender?

Hello,

At Tue, 22 Nov 2016 12:35:56 +0800, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote in <CAMsr+YF9-pojdPukTO7XGi+G1w=aRkv=tV7xBG0OPQdD+xX+-Q(at)mail(dot)gmail(dot)com>
> On 22 November 2016 at 11:35, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > Hello,
> >
> > At Mon, 21 Nov 2016 21:39:07 -0500, Robert Haas <robertmhaas(at)gmail(dot)com> wrote in <CA+TgmoZh6M5bTmtZZm1vBpFGHmeNgESe4UrJ3-OwKnu56SKB7g(at)mail(dot)gmail(dot)com>
> >> On Mon, Nov 21, 2016 at 3:21 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> >> > I'm still interested in hearing comments from experienced folks about
> >> > whether it's sane to do this at all, rather than have similar
> >> > duplicate signal handling for the walsender.
> >>
> >> Well, I mean, if it's reasonable to share code in a given situation,
> >> that is generally better than NOT sharing code...
> >
> > Walsender handles SIGUSR1 completely different way from normal
> > backends. The procsignal_sigusr1_handler is designed to work as
> > the peer of SendProcSignal (not ProcSendSignal:). Walsender is
> > just using a latch to be woken up. It has nothing to do with
> > SendProcSignal.
>
> Indeed, at the moment it doesn't. I'm proposing to change that, since
> walsenders doing logical decoding on a standby need to be notified of
> conflicts with recovery, many of which are the same as for normal user
> backends and bgworkers.
>
> The main exception is snapshot conflicts, where logical decoding
> walsenders don't care about conflicts with specific tables, they want
> to be terminated if the effective catalog_xmin on the upstream
> increases past their needed catalog_xmin. They don't care about
> non-catalog (or non-heap-catalog) tables. So I expect we'd just want
> to ignore PROCSIG_RECOVERY_CONFLICT_SNAPSHOT when running a walsender
> and handle conflict with catalog_xmin increases separately.

Thank you for the explanation. It seems to be reasonable for
walsender to have the same mechanism with
procsignal_sigusr1_handler. Sharing a handler or having another
one is a matter of design. (But sharing it might not be better if
walsender handles only two reasons.)

> > IMHO, I don't think walsender is allowed to just share the
> > backends' handler for a practical reason that pss_signalFlags can
> > harm.
>
> I'm not sure I see the problem. The ProcSignalReason argument to
> RecoveryConflictInterrupt states that:
>
> * Also, because of race conditions, it's important that all the signals be
> * defined so that no harm is done if a process mistakenly receives one.

It won't cause actual problem, but it is not managed on the
*current* walsender. I meant that taking any action based on
unmanaged variable seems to be a flaw of design. But anyway no
problem if it is redesigned to be used on walsender.

> > If you need to expand the function of SIGUSR1 of walsender, more
> > thought would be needed.
>
> Yeah, I definitely don't think it's as simple as just using
> procsignal_sigusr1_handler as-is. I expect we'd likely create a new
> global IsWalSender and ignore some RecoveryConflictInterrupt cases
> when in a walsender, at least PROCSIG_RECOVERY_CONFLICT_SNAPSHOT, and
> probably add a new case for catalog_xmin conflicts that's only acted
> on when IsWalSender.

The global is unncecessary if walsender have a different handler
from normal backends. If there's at least one or few additional
reasons for signal, sharing SendProcSignal and having dedicate
handler might be better.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center