From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Patch: fix lock contention for HASHHDR.mutex
Date: Thu, 21 Jan 2016 17:03:34 +0300
Message-ID: <56A0E536.8020504@postgrespro.ru> (raw)
In-Reply-To: <20151230160137.034371e0@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>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
30.12.2015 16:01, Aleksander Alekseev:
> Here is a clean version of the patch. Step 1 is an optimization. Step 2
> refactors this:
>
> HTAB *
> ShmemInitHash(const char *name, /* table string name for shmem index */
> - long init_size, /* initial table size */
> + long init_size, /* initial table size XXX ignored,
> refactor! */
Hi, I found that the patch is still not reviewed, so I've looked it over.
I haven't dived into this subject before, so my comments will be more
general than relating to your investigation.
Sorry if some things seem like nitpicks, I just suppose that clear patch
would be more convenient for reviewers.
First of all, why not merge both patches into one? They aren't too big
anyway.
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.
+ /* number of entries in hash table */
+ long nentries[NUM_LOCK_PARTITIONS];
+ /* linked list of free elements */
+ HASHELEMENT *freeList[NUM_LOCK_PARTITIONS];
I think comments should be changed, to be more informative here.
+ if (IS_PARTITIONED(hashp->hctl))
+ partitions_number = NUM_LOCK_PARTITIONS;
+ else
+ partitions_number = 1;
+
+ nelem_alloc = ((int) nelem) / partitions_number;
+ if (nelem_alloc == 0)
+ nelem_alloc = 1;
Add a comment here too.
-/* Number of partitions of the shared buffer mapping hashtable */
-#define NUM_BUFFER_PARTITIONS 128
-
/* Number of partitions the shared lock tables are divided into */
-#define LOG2_NUM_LOCK_PARTITIONS 4
+#define LOG2_NUM_LOCK_PARTITIONS 7
#define NUM_LOCK_PARTITIONS (1 << LOG2_NUM_LOCK_PARTITIONS)
+/* Number of partitions of the shared buffer mapping hashtable */
+#define NUM_BUFFER_PARTITIONS NUM_LOCK_PARTITIONS
+
I'm sure, it was discussed above. but these definitions do not look
obvious at all.
Maybe you should explain this magic number 7 in the comment above?
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Subject: Re: Patch: fix lock contention for HASHHDR.mutex
In-Reply-To: <56A0E536.8020504@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