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. 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.
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. On a
related note, is there any point in having both held_relextlock.nLocks
and num_held_relextlocks?
I think RelationExtensionLock should be a new type of IPC wait event,
rather than a whole new category.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company