diff --git a/src/backend/port/unix_latch.c b/src/backend/port/unix_latch.c index 92ae780158..147e22cee4 100644 --- a/src/backend/port/unix_latch.c +++ b/src/backend/port/unix_latch.c @@ -51,6 +51,7 @@ #include "miscadmin.h" #include "portability/instr_time.h" #include "postmaster/postmaster.h" +#include "storage/barrier.h" #include "storage/latch.h" #include "storage/pmsignal.h" #include "storage/shmem.h" @@ -515,12 +516,11 @@ SetLatch(volatile Latch *latch) pid_t owner_pid; /* - * XXX there really ought to be a memory barrier operation right here, to - * ensure that any flag variables we might have changed get flushed to - * main memory before we check/set is_set. Without that, we have to - * require that callers provide their own synchronization for machines - * with weak memory ordering (see latch.h). + * The memory barrier has be to be placed here to ensure that any flag + * variables possibly changed by this process have been flushed to main + * memory, before we check/set is_set. */ + pg_memory_barrier(); /* Quick exit if already set */ if (latch->is_set) @@ -574,14 +574,12 @@ ResetLatch(volatile Latch *latch) latch->is_set = false; /* - * XXX there really ought to be a memory barrier operation right here, to - * ensure that the write to is_set gets flushed to main memory before we + * Ensure that the write to is_set gets flushed to main memory before we * examine any flag variables. Otherwise a concurrent SetLatch might * falsely conclude that it needn't signal us, even though we have missed * seeing some flag updates that SetLatch was supposed to inform us of. - * For the moment, callers must supply their own synchronization of flag - * variables (see latch.h). */ + pg_memory_barrier(); } /* diff --git a/src/backend/port/win32_latch.c b/src/backend/port/win32_latch.c index da0657c915..c7d4bdddc2 100644 --- a/src/backend/port/win32_latch.c +++ b/src/backend/port/win32_latch.c @@ -27,6 +27,7 @@ #include "miscadmin.h" #include "portability/instr_time.h" #include "postmaster/postmaster.h" +#include "storage/barrier.h" #include "storage/latch.h" #include "storage/pmsignal.h" #include "storage/shmem.h" @@ -293,6 +294,13 @@ SetLatch(volatile Latch *latch) { HANDLE handle; + /* + * The memory barrier has be to be placed here to ensure that any flag + * variables possibly changed by this process have been flushed to main + * memory, before we check/set is_set. + */ + pg_memory_barrier(); + /* Quick exit if already set */ if (latch->is_set) return; @@ -325,4 +333,12 @@ ResetLatch(volatile Latch *latch) Assert(latch->owner_pid == MyProcPid); latch->is_set = false; + + /* + * Ensure that the write to is_set gets flushed to main memory before we + * examine any flag variables. Otherwise a concurrent SetLatch might + * falsely conclude that it needn't signal us, even though we have missed + * seeing some flag updates that SetLatch was supposed to inform us of. + */ + pg_memory_barrier(); } diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c index c99c270a7c..b2b3a81f54 100644 --- a/src/backend/replication/syncrep.c +++ b/src/backend/replication/syncrep.c @@ -172,20 +172,10 @@ SyncRepWaitForLSN(XLogRecPtr XactCommitLSN) * walsender changes the state to SYNC_REP_WAIT_COMPLETE, it will * never update it again, so we can't be seeing a stale value in that * case. - * - * Note: on machines with weak memory ordering, the acquisition of the - * lock is essential to avoid race conditions: we cannot be sure the - * sender's state update has reached main memory until we acquire the - * lock. We could get rid of this dance if SetLatch/ResetLatch - * contained memory barriers. */ syncRepState = MyProc->syncRepState; if (syncRepState == SYNC_REP_WAITING) - { - LWLockAcquire(SyncRepLock, LW_SHARED); syncRepState = MyProc->syncRepState; - LWLockRelease(SyncRepLock); - } if (syncRepState == SYNC_REP_WAIT_COMPLETE) break; diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index ac647d6545..3add619b5d 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -214,7 +214,6 @@ StrategyGetBuffer(BufferAccessStrategy strategy) { /* reset bgwprocno first, before setting the latch */ StrategyControl->bgwprocno = -1; - pg_write_barrier(); /* * Not acquiring ProcArrayLock here which is slightly icky. It's diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h index 138a748f62..f1577b0095 100644 --- a/src/include/storage/latch.h +++ b/src/include/storage/latch.h @@ -57,13 +57,6 @@ * SetLatch *after* that. SetLatch is designed to return quickly if the * latch is already set. * - * Presently, when using a shared latch for interprocess signalling, the - * flag variable(s) set by senders and inspected by the wait loop must - * be protected by spinlocks or LWLocks, else it is possible to miss events - * on machines with weak memory ordering (such as PPC). This restriction - * will be lifted in future by inserting suitable memory barriers into - * SetLatch and ResetLatch. - * * On some platforms, signals will not interrupt the latch wait primitive * by themselves. Therefore, it is critical that any signal handler that * is meant to terminate a WaitLatch wait calls SetLatch.