At Tue, 22 Nov 2016 12:35:56 +0800, Craig Ringer <cr...@2ndquadrant.com> wrote 
in <CAMsr+YF9-pojdPukTO7XGi+G1w=aRkv=tv7xbg0opqdd+xx...@mail.gmail.com>
> On 22 November 2016 at 11:35, Kyotaro HORIGUCHI
> <horiguchi.kyot...@lab.ntt.co.jp> wrote:
> > Hello,
> >
> > At Mon, 21 Nov 2016 21:39:07 -0500, Robert Haas <robertmh...@gmail.com> 
> > wrote in 
> > <ca+tgmozh6m5btmtzzm1vbpfghmengese4urj3-owknu56sk...@mail.gmail.com>
> >> On Mon, Nov 21, 2016 at 3:21 AM, Craig Ringer <cr...@2ndquadrant.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.


Kyotaro Horiguchi
NTT Open Source Software Center

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to