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

No that's not right.

At Tue, 22 Nov 2016 17:45:34 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20161122(dot)174534(dot)266086549(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> 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.)
...
> > > 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.

If no behavior is shared among normal backend and walsender, it
would be a good reason not to share the handler function. What
you are willing to do seems so.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center