On Sun, Jan 7, 2018 at 11:26 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote:
> On Fri, Jan 5, 2018 at 1:39 AM, Robert Haas <robertmh...@gmail.com> wrote:
>> On Tue, Jan 2, 2018 at 1:09 AM, Mithun Cy <mithun...@enterprisedb.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.
>
On second thought, perhaps we should also do performance measurement
with the patch that uses HTAB instead a fixed array. Probably the
performance with that patch will be equal to or slightly greater than
current HEAD, hopefully not be worse. In addition to that, if the
performance degradation by false collision doesn't happen or we can
avoid it by increasing GUC parameter, I think it's better than current
fixed array approach. Thoughts?
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
