Re: [HACKERS] Re: Use procsignal_sigusr1_handler and RecoveryConflictInterrupt() from walsender?
On 22 November 2016 at 17:49, Kyotaro HORIGUCHI wrote: >> > 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. I've explored this some more, and it looks like using procsignal_sigusr1_handler for handling recovery conflicts in the walsender during logical decoding actually makes a lot of sense. Almost all behaviour is shared, and so far I haven't needed any special cases at all. I needed to add a new recovery signal for conflict with catalog_xmin advance on upstream, but that was it. Many of the cases make no sense for physical walsenders, so it probably makes sense to bail out early if it's a physical walsender, but for a walsender doing logical replication the only one that I don't think makes sense is conflict with snapshot, which won't get sent and is harmless if received. (The comment on it is slightly wrong anyway; it claims it's only used by normal user backends in transactions, but database conflicts are fired even when not in an xact.) -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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 wrote in <20161122.174534.266086549.horiguchi.kyot...@lab.ntt.co.jp> > Hello, > > At Tue, 22 Nov 2016 12:35:56 +0800, Craig Ringer > wrote in > > On 22 November 2016 at 11:35, Kyotaro HORIGUCHI > > wrote: > > > Hello, > > > > > > At Mon, 21 Nov 2016 21:39:07 -0500, Robert Haas > > > wrote in > > > > > >> On Mon, Nov 21, 2016 at 3:21 AM, Craig Ringer > > >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Use procsignal_sigusr1_handler and RecoveryConflictInterrupt() from walsender?
Hello, At Tue, 22 Nov 2016 12:35:56 +0800, Craig Ringer wrote in > On 22 November 2016 at 11:35, Kyotaro HORIGUCHI > wrote: > > Hello, > > > > At Mon, 21 Nov 2016 21:39:07 -0500, Robert Haas > > wrote in > > > >> On Mon, Nov 21, 2016 at 3:21 AM, Craig Ringer > >> 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
Re: [HACKERS] Re: Use procsignal_sigusr1_handler and RecoveryConflictInterrupt() from walsender?
On 22 November 2016 at 11:35, Kyotaro HORIGUCHI wrote: > Hello, > > At Mon, 21 Nov 2016 21:39:07 -0500, Robert Haas wrote > in >> On Mon, Nov 21, 2016 at 3:21 AM, Craig Ringer 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Use procsignal_sigusr1_handler and RecoveryConflictInterrupt() from walsender?
Hello, At Mon, 21 Nov 2016 21:39:07 -0500, Robert Haas wrote in > On Mon, Nov 21, 2016 at 3:21 AM, Craig Ringer 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. IMHO, I don't think walsender is allowed to just share the backends' handler for a practical reason that pss_signalFlags can harm. If you need to expand the function of SIGUSR1 of walsender, more thought would be needed. 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
Re: [HACKERS] Re: Use procsignal_sigusr1_handler and RecoveryConflictInterrupt() from walsender?
On Mon, Nov 21, 2016 at 3:21 AM, Craig Ringer 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... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: Use procsignal_sigusr1_handler and RecoveryConflictInterrupt() from walsender?
On 18 November 2016 at 10:57, Craig Ringer wrote: > Hi all > > While adding support for logical decoding on standby I noticed that > the walsender doesn't respect > SIGUSR1 with PROCSIG_RECOVERY_CONFLICT_DATABASE set. I have an update on this after further study. Surprisingly I haven't found a way to crash the walsender with it, but as expected it is also not really correct as-is. It's fine for terminating walsender sessions on database drop, or for holding a lock on an object too long. Termination due to conflict with vacuum is more problematic because: * it'll try to terminate a logical decoding walsender even if there's no real conflict, since only normal relation blocks were affected, not user catalogs or system catalogs; and * most of the time its attempts to terminate won't do anything because there's no xact running on the walsender at the time it checks for termination. So that's definitely going to need more work. 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. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
