Hello, I noticed that commit 06c418e163e9 uses waitLSN->mutex (a spinlock) to protect the contents of waitLSN -- and it's used to walk an arbitrary long list of processes waiting ... and also, an arbitrary number of processes could be calling this code. I think using a spinlock for this is unwise, as it'll cause busy-waiting whenever there's contention. Wouldn't it be better to use an LWLock for this? Then the processes would sleep until the lock is freed.
While nosing about the code, other things struck me: I think there should be more comments about WaitLSNProcInfo and WaitLSNState in waitlsn.h. In addLSNWaiter it'd be better to assign 'cur' before acquiring the lock. Is a plan array really the most efficient data structure for this, considering that you have to reorder each time you add an element? Maybe it is, but wouldn't it make sense to use memmove() when adding one element rather iterating all the remaining elements to the end of the queue? I think the include list in waitlsn.c could be tightened a bit: @@ -18,28 +18,18 @@ #include <math.h> #include "pgstat.h" -#include "fmgr.h" -#include "access/transam.h" -#include "access/xact.h" #include "access/xlog.h" -#include "access/xlogdefs.h" #include "access/xlogrecovery.h" -#include "catalog/pg_type.h" #include "commands/waitlsn.h" -#include "executor/spi.h" #include "funcapi.h" #include "miscadmin.h" -#include "storage/ipc.h" #include "storage/latch.h" -#include "storage/pmsignal.h" #include "storage/proc.h" #include "storage/shmem.h" -#include "storage/sinvaladt.h" -#include "utils/builtins.h" #include "utils/pg_lsn.h" #include "utils/snapmgr.h" -#include "utils/timestamp.h" #include "utils/fmgrprotos.h" +#include "utils/wait_event_types.h" -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Las cosas son buenas o malas segun las hace nuestra opinión" (Lisias)
