Hello, 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. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
