Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

From: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager
Date: 2017-12-01 15:14:54
Message-ID: CAD21AoAoViM8rbwWF8bPN-y1xAQBAWt1JTpmpi9_QuM0F_TUEw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, Dec 1, 2017 at 10:26 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Fri, Dec 1, 2017 at 3:04 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Nov 30, 2017 at 6:20 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>> This code ignores the existence of multiple databases; RELEXTLOCK
>>>> contains a relid, but no database OID. That's easy enough to fix, but
>>>> it actually causes no problem unless, by bad luck, you have two
>>>> relations with the same OID in different databases that are both being
>>>> rapidly extended at the same time -- and even then, it's only a
>>>> performance problem, not a correctness problem. In fact, I wonder if
>>>> we shouldn't go further: instead of creating these RELEXTLOCK
>>>> structures dynamically, let's just have a fixed number of them, say
>>>> 1024. When we get a request to take a lock, hash <dboid, reloid> and
>>>> take the result modulo 1024; lock the RELEXTLOCK at that offset in the
>>>> array.
>>>
>>> Attached the latest patch incorporated comments except for the fix of
>>> the memory size for relext lock.
>>
>> It doesn't do anything about the comment of mine quoted above.
>
> Sorry I'd missed the comment.
>
>> Since it's only possible to hold one relation extension lock at a time, we
>> don't really need the hash table here at all. We can just have an
>> array of 1024 or so locks and map every <db,relid> pair on to one of
>> them by hashing. The worst thing we'll get it some false contention,
>> but that doesn't seem awful, and it would permit considerable further
>> simplification of this code -- and maybe make it faster in the
>> process, because we'd no longer need the hash table, or the pin count,
>> or the extra LWLocks that protect the hash table. All we would have
>> is atomic operations manipulating the lock state, which seems like it
>> would be quite a lot faster and simpler.
>
> Agreed. With this change, we will have an array of the struct that has
> lock state and cv. The lock state has the wait count as well as the
> status of lock.
>
>> BTW, I think RelExtLockReleaseAll is broken because it shouldn't
>> HOLD_INTERRUPTS(); I also think it's kind of silly to loop here when
>> we know we can only hold one lock. Maybe RelExtLockRelease can take
>> bool force and do if (force) held_relextlock.nLocks = 0; else
>> held_relextlock.nLocks--. Or, better yet, have the caller adjust that
>> value and then only call RelExtLockRelease() if we needed to release
>> the lock in shared memory. That avoids needless branching.
>
> Agreed. I'd vote for the latter.
>
>> On a
>> related note, is there any point in having both held_relextlock.nLocks
>> and num_held_relextlocks?
>
> num_held_relextlocks is actually unnecessary, will be removed.
>
>> I think RelationExtensionLock should be a new type of IPC wait event,
>> rather than a whole new category.
>
> Hmm, I thought the wait event types of IPC seems related to events
> that communicates to other processes for the same purpose, for example
> parallel query, sync repli etc. On the other hand, the relation
> extension locks are one kind of the lock mechanism. That's way I added
> a new category. But maybe it can be fit to the IPC wait event.
>

Attached updated patch. I've done a performance measurement again on
the same configuration as before since the acquiring/releasing
procedures have been changed.

----- PATCHED -----
tps = 162.579320 (excluding connections establishing)
tps = 162.144352 (excluding connections establishing)
tps = 160.659403 (excluding connections establishing)
tps = 161.213995 (excluding connections establishing)
tps = 164.560460 (excluding connections establishing)
----- HEAD -----
tps = 157.738645 (excluding connections establishing)
tps = 146.178575 (excluding connections establishing)
tps = 143.788961 (excluding connections establishing)
tps = 144.886594 (excluding connections establishing)
tps = 145.496337 (excluding connections establishing)

* micro-benchmark
PATCHED = 1.61757e+07 (cycles/sec)
HEAD = 1.48685e+06 (cycles/sec)
The patched is 10 times faster than current HEAD.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Attachment Content-Type Size
Moving_extension_lock_out_of_heavyweight_lock_v9.patch text/x-patch 42.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2017-12-01 15:18:49 Re: [HACKERS] Custom compression methods
Previous Message Robert Haas 2017-12-01 15:14:43 Re: [HACKERS] SQL procedures