From: Aleksander Alekseev <a.alekseev@postgrespro.ru>
To: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Patch: fix lock contention for HASHHDR.mutex
Date: Thu, 17 Dec 2015 19:03:42 +0300
Message-ID: <20151217190342.07e4533a@fujitsu> (raw)
In-Reply-To: <20151217151418.GB2224@awork2.anarazel.de>
References: <20151211170001.78ded9d7@fujitsu>
<20151215122513.GC17724@alap3.anarazel.de>
<CA+TgmoavsNuBNQf6ROn020F6g9gXJjtMrSkSFZ=RL1t2eJHQ5Q@mail.gmail.com>
<20151217151418.GB2224@awork2.anarazel.de>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
> It'd really like to see it being replaced by a queuing lock
> (i.e. lwlock) before we go there. And then maybe partition the
> freelist, and make nentries an atomic.
I believe I just implemented something like this (see attachment). The
idea is to partition PROCLOCK hash table manually into NUM_LOCK_
PARTITIONS smaller and non-partitioned hash tables. Since these tables
are non-partitioned spinlock is not used and there is no lock
contention.
On 60-core server we gain 3.5-4 more TPS according to benchmark
described above. As I understand there is no performance degradation in
other cases (different CPU, traditional pgbench, etc).
If this patch seems to be OK I believe we could consider applying the
same change not only to PROCLOCK hash table.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:shard-proclock-hash-table.patch (text/x-patch, 13.5K) downloaddiff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.cindex 76fc615..1a86f86 100644--- a/src/backend/storage/lmgr/lock.c+++ b/src/backend/storage/lmgr/lock.c@@ -249,15 +249,56 @@ static volatile FastPathStrongRelationLockData *FastPathStrongRelationLocks;
* shared memory; LockMethodLocalHash is local to each backend.
*/
static HTAB *LockMethodLockHash;
-static HTAB *LockMethodProcLockHash;+static HTAB *LockMethodProcLockHashPartitions[NUM_LOCK_PARTITIONS];
static HTAB *LockMethodLocalHash;
+/*+ * LockMethodProcLockHash hash table is partitioned into NUM_LOCK_PARTITIONS+ * usual (non-partitioned, i.e. without HASH_PARTITION flag) hash tables. The+ * reason for partitioning LockMethodProcLockHash manually instead of just+ * using single hash table with HASH_PARTITION flag is following. If hash+ * table has HASH_PARTITION flag it uses a single spinlock to safely+ * access some of its fields. Turned out in case of this particular hash+ * table it causes a considerable performance degradation becouse of lock+ * contention on servers with tens of cores.+ */+#define LockMethodProcLockHash(hashcode) \+ (LockMethodProcLockHashPartitions[LockHashPartition(hashcode)])++/*+ * Each partition of LockMethodProcLockHash hash table has an unique name+ * generated from this template using snprintf.+ */+static const char ProcLockHashNameTemplate[] = "PROCLOCK hash, partition %d";++/*+ * Size of buffer required to store LockMethodProcLockHash partition name.+ *+ * 10 is number of digits in 2^32. 2 is length of "%d" string.+ */+#define PROC_LOCK_HASH_NAME_BUFF_SIZE (sizeof(ProcLockHashNameTemplate)+10-2)
/* private state for error cleanup */
static LOCALLOCK *StrongLockInProgress;
static LOCALLOCK *awaitedLock;
static ResourceOwner awaitedOwner;
+/*+ * Calculate total number of entries in LockMethodProcLockHash hash table.+ *+ * Caller is responsible for having acquired appropriate LWLocks.+ */+static long+GetProcLockHashNumEntries()+{+ int i;+ long sum = 0;++ for (i = 0; i < NUM_LOCK_PARTITIONS; i++)+ sum += hash_get_num_entries(LockMethodProcLockHashPartitions[i]);++ return sum;+}
#ifdef LOCK_DEBUG
@@ -373,16 +414,16 @@ void
InitLocks(void)
{
HASHCTL info;
- long init_table_size,- max_table_size;+ long shmem_table_size;
bool found;
+ int i;+ char proclock_hash_name[PROC_LOCK_HASH_NAME_BUFF_SIZE];
/*
* Compute init/max size to request for lock hashtables. Note these
* calculations must agree with LockShmemSize!
*/
- max_table_size = NLOCKENTS();- init_table_size = max_table_size / 2;+ shmem_table_size = NLOCKENTS();
/*
* Allocate hash table for LOCK structs. This stores per-locked-object
@@ -394,14 +435,13 @@ InitLocks(void)
info.num_partitions = NUM_LOCK_PARTITIONS;
LockMethodLockHash = ShmemInitHash("LOCK hash",
- init_table_size,- max_table_size,+ shmem_table_size,+ shmem_table_size,
&info,
HASH_ELEM | HASH_BLOBS | HASH_PARTITION);
/* Assume an average of 2 holders per lock */
- max_table_size *= 2;- init_table_size *= 2;+ shmem_table_size = (shmem_table_size * 2) / NUM_LOCK_PARTITIONS;
/*
* Allocate hash table for PROCLOCK structs. This stores
@@ -412,11 +452,17 @@ InitLocks(void)
info.hash = proclock_hash;
info.num_partitions = NUM_LOCK_PARTITIONS;
- LockMethodProcLockHash = ShmemInitHash("PROCLOCK hash",- init_table_size,- max_table_size,- &info,- HASH_ELEM | HASH_FUNCTION | HASH_PARTITION);+ for (i = 0; i < NUM_LOCK_PARTITIONS; i++)+ {+ snprintf(proclock_hash_name, sizeof(proclock_hash_name),+ ProcLockHashNameTemplate, i + 1);++ LockMethodProcLockHashPartitions[i] = ShmemInitHash(proclock_hash_name,+ shmem_table_size,+ shmem_table_size,+ &info,+ HASH_ELEM | HASH_FUNCTION);+ }
/*
* Allocate fast-path structures.
@@ -943,7 +989,7 @@ LockAcquireExtended(const LOCKTAG *locktag,
proclock_hashcode = ProcLockHashCode(&proclock->tag, hashcode);
SHMQueueDelete(&proclock->lockLink);
SHMQueueDelete(&proclock->procLink);
- if (!hash_search_with_hash_value(LockMethodProcLockHash,+ if (!hash_search_with_hash_value(LockMethodProcLockHash(hashcode),
(void *) &(proclock->tag),
proclock_hashcode,
HASH_REMOVE,
@@ -1102,7 +1148,7 @@ SetupLockInTable(LockMethod lockMethodTable, PGPROC *proc,
/*
* Find or create a proclock entry with this tag
*/
- proclock = (PROCLOCK *) hash_search_with_hash_value(LockMethodProcLockHash,+ proclock = (PROCLOCK *) hash_search_with_hash_value(LockMethodProcLockHash(hashcode),
(void *) &proclocktag,
proclock_hashcode,
HASH_ENTER_NULL,
@@ -1424,7 +1470,7 @@ CleanUpLock(LOCK *lock, PROCLOCK *proclock,
SHMQueueDelete(&proclock->lockLink);
SHMQueueDelete(&proclock->procLink);
proclock_hashcode = ProcLockHashCode(&proclock->tag, hashcode);
- if (!hash_search_with_hash_value(LockMethodProcLockHash,+ if (!hash_search_with_hash_value(LockMethodProcLockHash(hashcode),
(void *) &(proclock->tag),
proclock_hashcode,
HASH_REMOVE,
@@ -1881,7 +1927,7 @@ LockRelease(const LOCKTAG *locktag, LOCKMODE lockmode, bool sessionLock)
proclocktag.myLock = lock;
proclocktag.myProc = MyProc;
- locallock->proclock = (PROCLOCK *) hash_search(LockMethodProcLockHash,+ locallock->proclock = (PROCLOCK *) hash_search(LockMethodProcLockHash(locallock->hashcode),
(void *) &proclocktag,
HASH_FIND,
NULL);
@@ -2631,7 +2677,7 @@ FastPathGetRelationLockEntry(LOCALLOCK *locallock)
proclock_hashcode = ProcLockHashCode(&proclocktag, locallock->hashcode);
proclock = (PROCLOCK *)
- hash_search_with_hash_value(LockMethodProcLockHash,+ hash_search_with_hash_value(LockMethodProcLockHash(locallock->hashcode),
(void *) &proclocktag,
proclock_hashcode,
HASH_FIND,
@@ -2914,7 +2960,8 @@ LockRefindAndRelease(LockMethod lockMethodTable, PGPROC *proc,
proclock_hashcode = ProcLockHashCode(&proclocktag, hashcode);
- proclock = (PROCLOCK *) hash_search_with_hash_value(LockMethodProcLockHash,+ proclock = (PROCLOCK *) hash_search_with_hash_value(+ LockMethodProcLockHash(hashcode),
(void *) &proclocktag,
proclock_hashcode,
HASH_FIND,
@@ -3243,7 +3290,7 @@ PostPrepare_Locks(TransactionId xid)
* the same hash key, since there can be only one entry for any
* given lock with my own proc.
*/
- if (!hash_update_hash_key(LockMethodProcLockHash,+ if (!hash_update_hash_key(LockMethodProcLockHashPartitions[partition],
(void *) proclock,
(void *) &proclocktag))
elog(PANIC, "duplicate entry found while reassigning a prepared transaction's locks");
@@ -3410,7 +3457,7 @@ GetLockStatusData(void)
LWLockAcquire(LockHashPartitionLockByIndex(i), LW_SHARED);
/* Now we can safely count the number of proclocks */
- data->nelements = el + hash_get_num_entries(LockMethodProcLockHash);+ data->nelements = el + GetProcLockHashNumEntries();
if (data->nelements > els)
{
els = data->nelements;
@@ -3418,27 +3465,30 @@ GetLockStatusData(void)
repalloc(data->locks, sizeof(LockInstanceData) * els);
}
- /* Now scan the tables to copy the data */- hash_seq_init(&seqstat, LockMethodProcLockHash);-- while ((proclock = (PROCLOCK *) hash_seq_search(&seqstat)))+ for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
{
- PGPROC *proc = proclock->tag.myProc;- LOCK *lock = proclock->tag.myLock;- LockInstanceData *instance = &data->locks[el];+ /* Now scan the tables to copy the data */+ hash_seq_init(&seqstat, LockMethodProcLockHashPartitions[i]);- memcpy(&instance->locktag, &lock->tag, sizeof(LOCKTAG));- instance->holdMask = proclock->holdMask;- if (proc->waitLock == proclock->tag.myLock)- instance->waitLockMode = proc->waitLockMode;- else- instance->waitLockMode = NoLock;- instance->backend = proc->backendId;- instance->lxid = proc->lxid;- instance->pid = proc->pid;- instance->fastpath = false;+ while ((proclock = (PROCLOCK *) hash_seq_search(&seqstat)))+ {+ PGPROC *proc = proclock->tag.myProc;+ LOCK *lock = proclock->tag.myLock;+ LockInstanceData *instance = &data->locks[el];- el++;+ memcpy(&instance->locktag, &lock->tag, sizeof(LOCKTAG));+ instance->holdMask = proclock->holdMask;+ if (proc->waitLock == proclock->tag.myLock)+ instance->waitLockMode = proc->waitLockMode;+ else+ instance->waitLockMode = NoLock;+ instance->backend = proc->backendId;+ instance->lxid = proc->lxid;+ instance->pid = proc->pid;+ instance->fastpath = false;++ el++;+ }
}
/*
@@ -3487,7 +3537,7 @@ GetRunningTransactionLocks(int *nlocks)
LWLockAcquire(LockHashPartitionLockByIndex(i), LW_SHARED);
/* Now we can safely count the number of proclocks */
- els = hash_get_num_entries(LockMethodProcLockHash);+ els = GetProcLockHashNumEntries();
/*
* Allocating enough space for all locks in the lock table is overkill,
@@ -3495,42 +3545,46 @@ GetRunningTransactionLocks(int *nlocks)
*/
accessExclusiveLocks = palloc(els * sizeof(xl_standby_lock));
- /* Now scan the tables to copy the data */- hash_seq_init(&seqstat, LockMethodProcLockHash);-- /*- * If lock is a currently granted AccessExclusiveLock then it will have- * just one proclock holder, so locks are never accessed twice in this- * particular case. Don't copy this code for use elsewhere because in the- * general case this will give you duplicate locks when looking at- * non-exclusive lock types.- */- index = 0;- while ((proclock = (PROCLOCK *) hash_seq_search(&seqstat)))+ for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
{
- /* make sure this definition matches the one used in LockAcquire */- if ((proclock->holdMask & LOCKBIT_ON(AccessExclusiveLock)) &&- proclock->tag.myLock->tag.locktag_type == LOCKTAG_RELATION)+ /* Now scan the tables to copy the data */+ hash_seq_init(&seqstat, LockMethodProcLockHashPartitions[i]);++ /*+ * If lock is a currently granted AccessExclusiveLock then it will+ * have just one proclock holder, so locks are never accessed twice in+ * this particular case. Don't copy this code for use elsewhere+ * because in the general case this will give you duplicate locks when+ * looking at non-exclusive lock types.+ */+ index = 0;+ while ((proclock = (PROCLOCK *) hash_seq_search(&seqstat)))
{
- PGPROC *proc = proclock->tag.myProc;- PGXACT *pgxact = &ProcGlobal->allPgXact[proc->pgprocno];- LOCK *lock = proclock->tag.myLock;- TransactionId xid = pgxact->xid;+ /* make sure this definition matches the one used in LockAcquire */+ if ((proclock->holdMask & LOCKBIT_ON(AccessExclusiveLock)) &&+ proclock->tag.myLock->tag.locktag_type == LOCKTAG_RELATION)+ {+ PGPROC *proc = proclock->tag.myProc;+ PGXACT *pgxact = &ProcGlobal->allPgXact[proc->pgprocno];+ LOCK *lock = proclock->tag.myLock;+ TransactionId xid = pgxact->xid;- /*- * Don't record locks for transactions if we know they have- * already issued their WAL record for commit but not yet released- * lock. It is still possible that we see locks held by already- * complete transactions, if they haven't yet zeroed their xids.- */- if (!TransactionIdIsValid(xid))- continue;+ /*+ * Don't record locks for transactions if we know they have+ * already issued their WAL record for commit but not yet+ * released lock. It is still possible that we see locks held+ * by already complete transactions, if they haven't yet+ * zeroed their xids.+ */+ if (!TransactionIdIsValid(xid))+ continue;- accessExclusiveLocks[index].xid = xid;- accessExclusiveLocks[index].dbOid = lock->tag.locktag_field1;- accessExclusiveLocks[index].relOid = lock->tag.locktag_field2;+ accessExclusiveLocks[index].xid = xid;+ accessExclusiveLocks[index].dbOid = lock->tag.locktag_field1;+ accessExclusiveLocks[index].relOid = lock->tag.locktag_field2;- index++;+ index++;+ }
}
}
@@ -3614,23 +3668,27 @@ DumpAllLocks(void)
PROCLOCK *proclock;
LOCK *lock;
HASH_SEQ_STATUS status;
+ int i;
proc = MyProc;
if (proc && proc->waitLock)
LOCK_PRINT("DumpAllLocks: waiting on", proc->waitLock, 0);
- hash_seq_init(&status, LockMethodProcLockHash);-- while ((proclock = (PROCLOCK *) hash_seq_search(&status)) != NULL)+ for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
{
- PROCLOCK_PRINT("DumpAllLocks", proclock);+ hash_seq_init(&status, LockMethodProcLockHashPartitions[i]);- lock = proclock->tag.myLock;- if (lock)- LOCK_PRINT("DumpAllLocks", lock, 0);- else- elog(LOG, "DumpAllLocks: proclock->tag.myLock = NULL");+ while ((proclock = (PROCLOCK *) hash_seq_search(&status)) != NULL)+ {+ PROCLOCK_PRINT("DumpAllLocks", proclock);++ lock = proclock->tag.myLock;+ if (lock)+ LOCK_PRINT("DumpAllLocks", lock, 0);+ else+ elog(LOG, "DumpAllLocks: proclock->tag.myLock = NULL");+ }
}
}
#endif /* LOCK_DEBUG */
@@ -3749,7 +3807,7 @@ lock_twophase_recover(TransactionId xid, uint16 info,
/*
* Find or create a proclock entry with this tag
*/
- proclock = (PROCLOCK *) hash_search_with_hash_value(LockMethodProcLockHash,+ proclock = (PROCLOCK *) hash_search_with_hash_value(LockMethodProcLockHash(hashcode),
(void *) &proclocktag,
proclock_hashcode,
HASH_ENTER_NULL,
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.alekseev@postgrespro.ru, andres@anarazel.de, robertmhaas@gmail.com
Subject: Re: Patch: fix lock contention for HASHHDR.mutex
In-Reply-To: <20151217190342.07e4533a@fujitsu>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox