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

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.

> 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.

> 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.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services