Re: walsender & parallelism

Hi,

On 2017-04-23 16:59:41 -0700, 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?
>
>
> > -/* 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?
>
> I think it'd be better to have PostgresMain() set up signals by default,
> and then have WalSndSignals() overwrites the ones it needs separate.
> 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?

Oh, and one more point: There desperately need to be tests exercising
"SQL via walsender". Including the issue of parallelism here, the missed
cancel handler, timeouts, and such. One way to do so is to use
pg_regress with an adjusted connection string (specifying
replication=database), doing the same for isolationtester, or using
dblink.

- Andres