Re: [HACKERS] Race conditions with WAL sender PID lookups

Hi,

At Thu, 8 Jun 2017 13:15:02 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqTfu2P1QCZB22LJG+P4ET-pS0yW+qSTWO9JcVcCtaPZAA@mail.gmail.com>
> On Thu, Jun 8, 2017 at 3:31 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > I think if you're going to fix it so that we take spinlocks on
> > MyWalSnd in a bunch of places that we didn't take them before, it
> > would make sense to fix all the places where we're accessing those
> > fields without a spinlock instead of only some of them, unless there
> > are good reasons why we only need it in some cases and not others, in
> > which case I think the patch should add comments to each place where
> > the lock was not taken explaining why it's OK.  It didn't take me and
> > my copy of vi very long to find instances that you didn't change.
> 
> I take that you are referring to the two lookups in
> WalSndWaitForWal(), one in exec_replication_command(), one in
> WalSndLoop() and one in WalSndDone(). First let's remember that all
> the fields protected by the spinlock are only updated in a WAL sender
> process, so reading them directly is safe in this context: we know
> that no other processes will write them. And all those functions are
> called only in the context of a WAL sender process. So the copies of
> MyWalSnd that you are referring to here don't need a spinlock. Is
> there something I am missing?
> 
> Actually, perhaps it would make sense to add some Assert(am_walsender)
> in this code?

It is not obvious that reading MyWalSnd without spinlock is safe
without knowing that it is modified only by itself. Based on that
I see two ways to go.

1. Put spinlock there (WalSndWaitForWal and..) even though it is  not required. It ensures safety even if it is
modifiedby  other processes for possible additional conflict with other  readers. (Unless someone careless forgets lock
fornew code).
 

2. Put a comment there instead (maybe assertion is not verbose  enough), containing something like the mention above.


By the way, this this discussion make me think of
SyncRepInitConfig. Taking SyncRepLock seems unnecessary there to
me. 

> > I also think that should provide some analysis regarding the question
> > Thomas asked - are these actually bugs, or are they OK for some
> > reason?  We shouldn't just plaster the code with spinlock acquisitions
> > unless it's really necessary.  It seems at least possible that these
> > changes could cause performance regressions, and it doesn't look like
> > you've done any testing or provided any analysis indicating whether
> > that's likely to happen or not.
> 
> Now taking a step back on the patch, v3 that I sent 3 weeks ago.
> Additional spinlocks are taken in:
> 1) SyncRepReleaseWaiters(), which is called in a WAL sender. So indeed
> no spinlocks are needed here. The last patch is incorrect here.

Agreed that it is not necessary, but we might should put one
depending on the conclusion of the discussion.

> 2) SyncRepGetSyncStandbysPriority() and SyncRepGetSyncStandbysQuorum()
> as of HEAD via SyncRepGetSyncStandbys(), something that's not good if
> done lock-less as this gets used for pg_stat_replication for
> pg_stat_get_wal_senders(). This can return a garbage list of sync
> standbys to the client as WAL senders update their flush, write and
> apply positions in parallel to that, and don't care about SyncRepLock
> as this gets calculated with the standby feedback messages. The
> problem here is the gap between walsnd->sync_standby_priority and
> walsnd->flush:
> -- With a primary, and more or more sync standbys, take a checkpoint
> in SyncRepGetSyncStandbysPriority() after the first check is passed.
> -- Shutdown the standby and restart it.
> -- The standby reconnects, initializes the WAL sender slot with
> InitWalSenderSlot().
> -- Let the process of SyncRepGetSyncStandbysPriority() continue, the
> sync standby does not show up.
> That would be really unlikely to happen, but the code is written in
> such a way that it could happen. One could also say that this gives a
> closer idea that the standby disconnected but it looks wrong to me to
> do this value lookup without a required lock.

It doesn't seem to be a problem for
pg_stat_get_wal_senders(). For SyncRepReleaseWaiters, also it
doesn't seem to be a problem because next reply message runs the
process again.

> 3) In walreceiver.c's pg_stat_get_wal_receiver's:
> - Launch pg_stat_get_wal_receiver and take a checkpoint on it.
> - Pass the lookups of pid and ready_to_display.
> - Make the WAL receiver stop.
> - The view reports inconsistent data. If the WAL receiver data was
> just initialized, the caller would get back PID values or similar 0.
> Particularly a WAL receiver marked with !ready_to_display could show
> data to the caller. That's not cool.

Totally agreed.

> 4) KeepFileRestoredFromArchive() needs a lock, as this is called from
> the startup process. That's harmless, the worst that can happen is
> that needreload is switched to true for a process that has been marked
> with a PID of 0 because of a WAL sender restart (slot taken again and
> initialized). But that will just process an extra reload in a worst
> case.

Agreed to the disucussion but it doesn't seem to me to be
necessary, however, seems better to have.

By the way InitWalSenderSlot seems fogetting to initialize
needreload and sync_standby_priority. The initialization code
would be better to be arranged in the same order with its
defintion. Initializing needreload is quite natural from its
definition. (Still it is possible that an extra reload happens if
a reload is requested just after the initialization...)

> > Basically, I don't think this patch is ready to commit.  We have
> > neither evidence that it is necessary nor evidence that it is
> > complete.  I don't want to be unfair, here, but it seems to me you
> > ought to do a little more legwork before throwing this over the wall
> > to the pool of committers.
> 
> Fair enough. Is the analysis written above more convincing?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center