Re: walsender & parallelism

On 24/04/17 01:59, Andres Freund wrote:
> Hi,
>
> On 2017-04-22 17:53:19 +0200, Petr Jelinek wrote:
>> Here is patch. I changed both SIGINT and SIGUSR1 handlers, afaics it
>> does not break anything for existing walsender usage.
>
>> diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
>> index 761fbfa..e9dd886 100644
>> --- a/src/backend/replication/logical/launcher.c
>> +++ b/src/backend/replication/logical/launcher.c
>> @@ -254,7 +254,7 @@ logicalrep_worker_launch(Oid dbid, Oid subid, const char *subname, Oid userid,
>> BackgroundWorker bgw;
>> BackgroundWorkerHandle *bgw_handle;
>> int i;
>> - int slot;
>> + int slot = 0;
>> LogicalRepWorker *worker = NULL;
>> int nsyncworkers = 0;
>> TimestampTz now = GetCurrentTimestamp();
>
> That seems independent and unnecessary?
>

Yeah that leaked from different patch I was working on in parallel.

>
>> -/* SIGUSR1: set flag to send WAL records */
>> -static void
>> -WalSndXLogSendHandler(SIGNAL_ARGS)
>> -{
>> - int save_errno = errno;
>> -
>> - latch_sigusr1_handler();
>> -
>> - errno = save_errno;
>> -}
>
>> pqsignal(SIGPIPE, SIG_IGN);
>> - pqsignal(SIGUSR1, WalSndXLogSendHandler); /* request WAL sending */
>> + pqsignal(SIGUSR1, procsignal_sigusr1_handler);
>
> Those aren't actually entirely equivalent. I'm not sure it matters much,
> but WalSndXLogSendHandler didn't include a SetLatch(), but
> procsignal_sigusr1_handler() does. Normally redundant SetLatch() calls
> will just be elided, but what if walsender already woke up and did it's
> work?
>

They aren't exactly same no, but I don't see harm in what
procsignal_sigusr1_handler. I mean worst case scenario is latch is set
so walsender checks for work.

> I think it'd be better to have PostgresMain() set up signals by default,
> and then have WalSndSignals() overwrites the ones it needs separate.

Hmm that sounds like a good idea.

> WalSndLastCycleHandler is genuinely different. WalSndSigHupHandler
> currently sets a different variable from postgres.c, but that seems like
> a bad idea, because afaics we'll plainly ignore SIGHUPS unless in
> WalSndLoop, WalSndWriteData,WalSndWaitForWal. That actually seems like
> an active bug to me?
>

Hmm I don't think good solution is to use same variable though,
walsender needs to do slightly different thing on SIGHUP, plus the
got_SIGHUP is not the type of variable we want to export. I wonder if it
would be enough to just add check for got_SIGHUP somewhere at the
beginning of exec_replication_command().

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