Re: Race conditions with WAL sender PID lookups

At Fri, 12 May 2017 11:44:19 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoA15xsu1gbOH=L1XU7g7zuKk1UACtOz+-mqOwP1_xBC_g(at)mail(dot)gmail(dot)com>
> On Thu, May 11, 2017 at 10:48 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > Hi all,
> >
> > I had my eyes on the WAL sender code this morning, and I have noticed
> > that walsender.c is not completely consistent with the PID lookups it
> > does in walsender.c. In two code paths, the PID value is checked
> > without holding the WAL sender spin lock (WalSndRqstFileReload and
> > pg_stat_get_wal_senders), which looks like a very bad idea contrary to
> > what the new WalSndWaitStopping() does and what InitWalSenderSlot() is
> > doing for ages.
> >
> > Any thoughts about the patch attached to make things more consistent?
> > It seems to me that having some safeguards would be nice for
> > robustness.
>
> +1. I think this is a sensible change.

It intends to avoid exccesive locking during looking up stats
values. But we don't have so much vacant WanSnd slots in a
reasonable setup. Thus it seems reasonable to read the pid value
within the lock section since it adds practically no additional
cost. pg_stat_get_wal_receiver seems to need the same amendment
since the code is a parallel to that of wal receiver.

Or, if we were too sensitive to such locks for nothing, we could
use double-checked locking but I don't think we are so here.

In short, +1 too and walreceiver needs the same amendment.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center