Re: Get stuck when dropping a subscription during synchronizing table

On 2017-06-07 11:51:12 +0200, Petr Jelinek wrote:
> On 07/06/17 03:00, Andres Freund wrote:
> > On 2017-06-06 19:36:13 +0200, Petr Jelinek wrote:
> >
> >> As a side note, we are starting to have several IsSomeTypeOfProcess
> >> functions for these kind of things. I wonder if bgworker infrastructure
> >> should somehow provide this type of stuff (the proposed bgw_type might
> >> help there) as well as maybe being able to register interrupt handler
> >> for the worker (it's really hard to do it via custom SIGTERM handler as
> >> visible on worker, launcher and walsender issues we are fixing).
> >> Obviously this is PG11 related thought.
> >
> > Maybe it's also a sign that the bgworker infrastructure isn't really the
> > best thing to use for internal processes like parallel workers and lrep
> > processes - it's really built so core code *doesn't* know anything
> > specific about them, which isn't really what we want in some of these
> > cases. That's not to say that bgworkers and parallelism/lrep shouldn't
> > share infrastructure, don't get me wrong.
> >
>
> Well the nice thing about bgworkers is that it provides the basic
> infrastructure.

Right.

> Main reason lrep stuff is using it is that I didn't want to add
> another special backend kind to 20 different places, but turns out it
> still needs to be in some. So either we need to add more support for
> these kind of things to bgworkers or write something for internal
> workers that shares the infrastructure with bgworkers as you say.

Yea, I think we should radically unify a lot of the related
infrastructure between all processes. We've grown a lot of duplication.

> >> @@ -2848,6 +2849,14 @@ ProcessInterrupts(void)
> >> ereport(FATAL,
> >> (errcode(ERRCODE_ADMIN_SHUTDOWN),
> >> errmsg("terminating logical replication worker due to administrator command")));
> >> + else if (IsLogicalLauncher())
> >> + {
> >> + ereport(DEBUG1,
> >> + (errmsg("logical replication launcher shutting down")));
> >> +
> >> + /* The logical replication launcher can be stopped at any time. */
> >> + proc_exit(0);
> >> + }
> >
> > We could use PgBackendStatus->st_backendType for these, merging these
> > different paths.
> >
>
> Hmm, that's not that easy, st_backendType will be generic type for
> bgworker as the bgw_type patch didn't land yet (which is discussed in
> yet another thread [1]). It seems like an argument for going forward
> with it (the bgw_type patch) in PG10.

Yea. I left it as is for now.

> I don't mind, it has some overlap with your proposed fixes for latching
> so if you are willing go ahead.

Done.

Greetings,

Andres Freund