Re: [HACKERS] 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