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

On Fri, Jan 5, 2018 at 1:39 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jan 2, 2018 at 1:09 AM, Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> wrote:
>> So in case of N_RELEXTLOCK_ENTS = 1 we can see regression as high 25%. ?

Thank you for the performance measurement!

> So now the question is: what do these results mean for this patch?
>
> I think that the chances of someone simultaneously bulk-loading 16 or
> more relations that all happen to hash to the same relation extension
> lock bucket is pretty darn small. Most people aren't going to be
> running 16 bulk loads at the same time in the first place, and if they
> are, then there's a good chance that at least some of those loads are
> either actually to the same relation, or that many or all of the loads
> are targeting the same filesystem and the bottleneck will occur at
> that level, or that the loads are to relations which hash to different
> buckets. Now, if we want to reduce the chances of hash collisions, we
> could boost the default value of N_RELEXTLOCK_ENTS to 2048 or 4096.
>
> However, if we take the position that no hash collision probability is
> low enough and that we must eliminate all chance of false collisions,
> except perhaps when the table is full, then we have to make this
> locking mechanism a whole lot more complicated. We can no longer
> compute the location of the lock we need without first taking some
> other kind of lock that protects the mapping from {db_oid, rel_oid} ->
> {memory address of the relevant lock}. We can no longer cache the
> location where we found the lock last time so that we can retake it.
> If we do that, we're adding extra cycles and extra atomics and extra
> code that can harbor bugs to every relation extension to guard against
> something which I'm not sure is really going to happen. Something
> that's 3-8% faster in a case that occurs all the time and as much as
> 25% slower in a case that virtually never arises seems like it might
> be a win overall.
>
> However, it's quite possible that I'm not seeing the whole picture
> here. Thoughts?
>

I agree that the chances of the case where through-put got worse is
pretty small and we can get performance improvement in common cases.
Also, we could mistakenly overestimate the number of blocks we need to
add by false collisions. Thereby the performance might got worse and
we extend a relation more than necessary but I think the chances are
small. Considering the further parallel operations (e.g. parallel
loading, parallel index creation etc) multiple processes will be
taking a relext lock of the same relation. Thinking of that, the
benefit of this patch that improves the speeds of acquiring/releasing
the lock would be effective.

In short I personally think the current patch is simple and the result
is not a bad. But If community cannot accept these degradations we
have to deal with the problem. For example, we could make the length
of relext lock array configurable by users. That way, users can reduce
the possibility of collisions. Or we could improve the relext lock
manager to eliminate false collision by changing it to a
open-addressing hash table. The code would get complex but false
collisions don't happen unless the array is not full.

Regards,

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