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

On Fri, Dec 1, 2017 at 3:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Nov 30, 2017 at 6:20 AM, Masahiko Sawada <sawada.mshk@gmail.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.

Regards,

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