Re: Get stuck when dropping a subscription during synchronizing table

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

>
>>
>> - LogicalRepCtx->launcher_pid = 0;
>> -
>> - /* ... and if it returns, we're done */
>> - ereport(DEBUG1,
>> - (errmsg("logical replication launcher shutting down")));
>> + /* Not reachable */
>> +}
>
> Maybe put a pg_unreachable() there?
>

Ah didn't know it existed.

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

> I can take care of this one, if you/Peter want.
>

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

[1]
https://www.postgresql.org/message-id/flat/0d795703-a885-2193-2331-f00d7a3a4e42(at)2ndquadrant(dot)com#0d795703-a885-2193-2331-f00d7a3a4e42@2ndquadrant.com

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