On Wed, Oct 2, 2013 at 10:37 PM, Merlin Moncure <mmoncure@gmail.com> wrote:
> On Wed, Oct 2, 2013 at 9:45 AM, Ants Aasma <ants@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
