On Mon, Jun 5, 2017 at 10:29 AM, Andres Freund <and...@anarazel.de> wrote:
> On 2017-06-02 17:20:23 -0700, Andres Freund wrote:
>> Attached is a *preliminary* patch series implementing this.  I've first
>> reverted the previous patch, as otherwise backpatchable versions of the
>> necessary patches would get too complicated, due to the signals used and
>> such.

That makes sense.

> I went again through this, and the only real thing I found that there
> was a leftover prototype in walsender.h.  I've in interim worked on
> backpatch versions of that series, annoying conflicts, but nothing
> really problematic.  The only real difference is adding SetLatch() calls
> to HandleWalSndInitStopping() < 9.6, and guarding SetLatch with an if <
> 9.5.
> As an additional patch (based on one by Petr), even though it more
> belongs to
> http://archives.postgresql.org/message-id/20170421014030.fdzvvvbrz4nckrow%40alap3.anarazel.de
> attached is a patch unifying SIGHUP between normal and walsender
> backends.  This needs to be backpatched all the way.  I've also attached
> a second patch, again based on Petr's, that unifies SIGHUP handling
> across all the remaining backends, but that's something that probably
> more appropriate for v11, although I'm still tempted to commit it
> earlier.

I have looked at all those patches. The set looks solid to me.

0001 and 0002 are straight-forward things. It makes sense to unify the
SIGUSR1 handling.

Here are some comments about 0003.

+ * This will trigger walsenders to send the remaining WAL, prevent them from
+ * accepting further commands. After that they'll wait till the last WAL is
+ * written.
I would rephrase the last sentence a bit:
"After that each WAL sender will wait until the end-of-checkpoint
record has been flushed on the receiver side."

+           /*
+            * 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.

About 0004... This definitely meritates a backpatch, PostgresMain() is
taken by WAL senders as well when executing queries.

-       if (got_SIGHUP)
+       if (ConfigRereadPending)
-           got_SIGHUP = false;
+           ConfigRereadPending = false;
A more appropriate name would be ConfigReloadPending perhaps?

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to