From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
To: Aleksander Alekseev <a.alekseev@postgrespro.ru>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Patch: fix lock contention for HASHHDR.mutex
Date: Wed, 27 Jan 2016 16:19:43 +0300
Message-ID: <56A8C3EF.7030604@postgrespro.ru> (raw)
In-Reply-To: <20160122134837.788ecf83@fujitsu>
References: <20151211170001.78ded9d7@fujitsu>
<20151215122513.GC17724@alap3.anarazel.de>
<CA+TgmoavsNuBNQf6ROn020F6g9gXJjtMrSkSFZ=RL1t2eJHQ5Q@mail.gmail.com>
<20151217151418.GB2224@awork2.anarazel.de>
<20151217190342.07e4533a@fujitsu>
<CA+Tgmob1-7=Bh21LyLXk7Yst1nzcA1P=rv0UKq1zH+A9PVehrg@mail.gmail.com>
<56740E41.4010708@sigaev.ru>
<CA+Tgmobp1Dsk5M0UDjm91kCtt+LLJ+tTzs0nucw4Vw2ZfQx29w@mail.gmail.com>
<20151222183953.771cb58b@fujitsu>
<CA+TgmoZC6RsDt09LsYvSgmZFjy-Jwc6rD4GiySY3d4QTh8c3ow@mail.gmail.com>
<20151228120112.6a847dfd@fujitsu>
<20151229184851.1bb7d1bd@fujitsu>
<20151230160137.034371e0@fujitsu>
<56A0E536.8020504@postgrespro.ru>
<20160122134837.788ecf83@fujitsu>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
22.01.2016 13:48, Aleksander Alekseev:
>> Then, this thread became too tangled. I think it's worth to write a
>> new message with the patch, the test script, some results and brief
>> overview of how does it really works. It will make following review
>> much easier.
> Sure.
>
> HASHHDR represents a hash table. It could be usual or partitioned.
> Partitioned table is stored in a shared memory and accessed by multiple
> processes simultaneously. To prevent data corruption hash table is
> partitioned and each process has to acquire a lock for a corresponding
> partition before accessing data in it. Number of partition is determine
> by lower bits of key's hash value. Most tricky part is --- dynahash
> knows nothing about these locks, all locking is done on calling side.
>
> Since shared memory is pre-allocated and can't grow to allocate memory
> in a hash table we use freeList. Also we use nentries field to count
> current number of elements in a hash table. Since hash table is used by
> multiple processes these fields are protected by a spinlock. Which as
> it turned out could cause lock contention and create a bottleneck.
>
> After trying a few approaches I discovered that using one spinlock per
> partition works quite well. Here are last benchmark results:
>
> http://www.postgresql.org/message-id/20151229184851.1bb7d1bd@fujitsu
>
> Note that "no locks" solution cant be used because it doesn't guarantee
> that all available memory will be used in some corner cases.
>
> You can find a few more details and a test script in the first message
> of this thread. If you have any other questions regarding this patch
> please don't hesitate to ask.
InitLocks
/*
* Compute init/max size to request for lock hashtables. Note these
* calculations must agree with LockShmemSize!
*/
This comment certainly requires some changes.
BTW, could you explain why init_table_size was two times less than
max_table_size?
Although, I don't see any problems with your changes.
- hctl->nentries = 0;
- hctl->freeList = NULL;
Why did you delete these two lines? I wonder if you should rewrite them
instead?
+ * This particular number of partitions significantly reduces lock
contention
+ * in partitioned hash tables, almost if partitioned tables didn't use any
+ * locking at all
As far as I understood, this number was obtained experimentally? Maybe
you should note that in the comment.
And the last, but not least.
+ nelem_alloc = ((int) nelem) / partitions_number;
+ if (nelem_alloc == 0)
+ nelem_alloc = 1;
+
+ for (i = 0; i < partitions_number; i++)
+ if (!element_alloc(hashp, nelem_alloc, i))
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
It looks like you forgot to round the result of the division.
For example, if you have nelem=25 and partitions_number=6.
25 / 6 = 4. And then you allocate 24 nelems, while 1 is lost.
What about productivity, I did a test on my notebook. Patched version
shows small performance improvement.
pgbench -j 1 -c 1 -f pgbench.sql -T 300 postgres
base patched
127tps 145tps
pgbench -j 8 -c 8 -f pgbench.sql -T 300 postgres
base patched
247tps 270tps
But I haven't any big machine to perform tests, where the patch is
supposed to make significant changes.
So I would rely on your and the other reviewers results.
Except mentioned notes, I suppose the patch is good enough to pass it to
a reviewer.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
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.lubennikova@postgrespro.ru, a.alekseev@postgrespro.ru
Subject: Re: Patch: fix lock contention for HASHHDR.mutex
In-Reply-To: <56A8C3EF.7030604@postgrespro.ru>
* 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