Re: logical replication and PANIC during shutdown checkpoint in publisher

On 2017-06-05 15:30:38 +0900, Michael Paquier wrote:
> I have looked at all those patches. The set looks solid to me.

Thanks!


> Here are some comments about 0003.
> +           /*
> +            * Have WalSndLoop() terminate the connection in an orderly
> +            * manner, after writing out all the pending data.
> +            */
> +           if (got_STOPPING)
> +               got_SIGUSR2 = true;
> I think that for correctness the state of the WAL sender should be
> switched to WALSNDSTATE_STOPPING in XLogSendLogical() as well.

No, that would be wrong.  If we switched here, checkpointer would finish
waiting, even though XLogSendLogical() might get called again.  That
e.g. could happen the TCP socket was full, and XLogSendLogical() gets
called again.


> A more appropriate name would be ConfigReloadPending perhaps?

Hm, ok.


> 0005 looks like a fine one-liner to me.
> 
> For 0006, you could include as well the removal of worker_spi_sighup()
> in the refactoring.

Ok.  I'll leave that patch for now, since I think it's probably better
to apply it only to master once v10 branched off.


> I think that it would be interesting to be able to
> trigger a feedback message using SIGHUP in WAL receivers, refactoring
> at the same time SIGHUP handling for WAL receivers. It is possible for
> example to abuse SIGHUP in autovacuum for cost parameters.

Could you clarify a bit here, I can't follow?  Do you think it's
actually a good idea to combine that with the largely mechanical patch?

- Andres