|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|
|Views:||Raw Message | Whole Thread | Download mbox|
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
>>> 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)
PATCHED = 1.61757e+07 (cycles/sec)
HEAD = 1.48685e+06 (cycles/sec)
The patched is 10 times faster than current HEAD.
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
|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|