On 2017-06-01 17:29:12 -0700, Andres Freund wrote:
> On 2017-06-02 08:38:51 +0900, Michael Paquier wrote:
> > On Fri, Jun 2, 2017 at 7:05 AM, Andres Freund <andres@anarazel.de> wrote:
> > > I'm a unhappy how this is reusing SIGINT for WalSndLastCycleHandler.
> > > Normally INT is used cancel interrupts, and since walsender is now also
> > > working as a normal backend, this overlap is bad. Even for plain
> > > walsender backend this seems bad, because now non-superusers replication
> > > users will terminate replication connections when they do
> > > pg_cancel_backend(). For replication=dbname users it's especially bad
> > > because there can be completely legitimate uses of pg_cancel_backend().
> >
> > Signals for WAL senders are set in WalSndSignals() which uses SIG_IGN
> > for SIGINT now in ~9.6, and StatementCancelHandler does not get set up
> > for a non-am_walsender backend. Am I missing something?
>
> Yes, but nothing in those observeration actually addresses my point?
>
> Some points:
>
> 1) 086221cf6b1727c2baed4703c582f657b7c5350e changes things so walsender
> backends use SIGINT for WalSndLastCycleHandler(), which is now
> triggerable by pg_cancel_backend(). Especially for logical rep
> walsenders it's not absurd sending that.
> 2) Walsenders now can run normal queries.
> 3) Afaict 086221cf6b1727c2baed4703c582f657b7c5350e doesn't really
> address the PANIC problem for database connected walsenders at all,
> because it doesn't even cancel non-replication commands. I.e. an
> already running query can just continue to run. Which afaict just
> entirely breaks shutdown. If the connection is idle, or running a
> query, we'll just wait forever.
> 4) the whole logic introduced in the above commit doesn't actually
> appear to handle logical decoding senders properly - wasn't the whole
> issue at hand that those can write WAL in some case? But
> nevertheless WalSndWaitForWal() does a
> WalSndSetState(WALSNDSTATE_STOPPING); *and then continues decoding
> and waiting* - which seems to obviate the entire point of that commit.
>
> I'm working on a patch rejiggering things so:
>
> a) upon shutdown checkpointer (so we can use procsignal), not
> postmaster, sends PROCSIG_WALSND_INIT_STOPPING to all walsenders (so
> we don't have to use up a normal signal handler)
> b) Upon reception walsenders immediately exit if !replication_active,
> otherwise sets got_STOPPING
> c) Logical decoding will finish if got_STOPPING is sent, *WITHOUT*, as
> currently done, moving to WALSNDSTATE_STOPPING. Not yet quite sure
> how to best handle the WALSNDSTATE_STOPPING transition in WalSndLoop().
> d) Once all remaining walsenders are in stopping state, postmaster sends
> SIGUSR2 to trigger shutdown (basically as before)
>
> Does that seem to make sense?
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.
This also fixes several of the issues from the somewhat related thread at
http://archives.postgresql.org/message-id/20170421014030.fdzvvvbrz4nckrow%40alap3.anarazel.de
I'm not perfectly happy with the use of XLogBackgroundFlush() but we
don't currently expose anything else to flush all pending WAL afaics -
it's not too bad either. Without that we can end up waiting forever
while if the last XLogInserts are done by an asynchronously committing
backend or the relevant backends exited before getting to flush out
their records, because walwriter has already been shut down at that
point.
Comments?
- Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers