On Wed, Oct 2, 2013 at 10:37 PM, Merlin Moncure <mmonc...@gmail.com> wrote: > On Wed, Oct 2, 2013 at 9:45 AM, Ants Aasma <a...@cybertec.at> wrote: >> I haven't reviewed the code in as much detail to say if there is an >> actual race here, I tend to think there's probably not, but the >> specific pattern that I had in mind is that with the following actual >> code: > > hm. I think there *is* a race. 2+ threads could race to the line: > > LocalRecoveryInProgress = xlogctl->SharedRecoveryInProgress; > > and simultaneously set the value of LocalRecoveryInProgress to false, > and both engage InitXLOGAccess, which is destructive. The move > operation is atomic, but I don't think there's any guarantee the reads > to xlogctl->SharedRecoveryInProgress are ordered between threads > without a lock. > > I don't think the memory barrier will fix this. Do you agree? If so, > my earlier patch (recovery4) is another take on this problem, and > arguable safer; the unlocked read is in a separate path that does not > engage InitXLOGAccess()
InitXLOGAccess only writes backend local variables, so it can't be destructive. In fact, it calls GetRedoRecPtr(), which does a spinlock acquisition cycle, which should be a full memory barrier. A read barrier after accessing SharedRecoveryInProgress is good enough, and it seems to be necessary to avoid a race condition for XLogCtl->ThisTimeLineID. Admittedly the race is so unprobable that in practice it probably doesn't matter. I just wanted to be spell out the correct way to do unlocked accesses as it can get quite confusing. I found Herb Sutter's "atomic<> weapons" talk very useful, thinking about the problem in terms of acquire and release makes it much clearer to me. Regards, Ants Aasma -- Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt Web: http://www.postgresql-support.de -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
