Re: MultiXact\SLRU buffers configuration

> 26 окт. 2020 г., в 06:05, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
>
> Hi!
>
> On Mon, Sep 28, 2020 at 5:41 PM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>>> 28 авг. 2020 г., в 23:08, Anastasia Lubennikova <a.lubennikova@postgrespro.ru> написал(а):
>>>
>>> 1) The first patch is sensible and harmless, so I think it is ready for committer. I haven't tested the performance
impact,though. 
>>>
>>> 2) I like the initial proposal to make various SLRU buffers configurable, however, I doubt if it really solves the
problem,or just moves it to another place? 
>>>
>>> The previous patch you sent was based on some version that contained changes for other slru buffers numbers:
'multixact_offsets_slru_buffers'and 'multixact_members_slru_buffers'. Have you just forgot to attach them? The patch
message"[PATCH v2 2/4]" hints that you had 4 patches) 
>>> Meanwhile, I attach the rebased patch to calm down the CFbot. The changes are trivial.
>>>
>>> 2.1) I think that both min and max values for this parameter are too extreme. Have you tested them?
>>>
>>> +               &multixact_local_cache_entries,
>>> +               256, 2, INT_MAX / 2,
>>>
>>> 2.2) MAX_CACHE_ENTRIES is not used anymore, so it can be deleted.
>>>
>>> 3) No changes for third patch. I just renamed it for consistency.
>>
>> Thank you for your review.
>>
>> Indeed, I had 4th patch with tests, but these tests didn't work well: I still did not manage to stress SLRUs to
reproduceproblem from production... 
>>
>> You are absolutely correct in point 2: I did only tests with sane values. And observed extreme performance
degradationwith values ~ 64 megabytes. I do not know which highest values should we pick? 1Gb? Or highest possible
functioningvalue? 
>>
>> I greatly appreciate your review, sorry for so long delay. Thanks!
>
> I took a look at this patchset.
>
> The 1st and 3rd patches look good to me.  I made just minor improvements.
> 1) There is still a case when SimpleLruReadPage_ReadOnly() relocks the
> SLRU lock, which is already taken in exclusive mode.  I've evaded this
> by passing the lock mode as a parameter to
> SimpleLruReadPage_ReadOnly().
> 3) CHECK_FOR_INTERRUPTS() is not needed anymore, because it's called
> inside ConditionVariableSleep() if needed.  Also, no current wait
> events use slashes, and I don't think we should introduce slashes
> here.  Even if we should, then we should also rename existing wait
> events to be consistent with a new one.  So, I've renamed the new wait
> event to remove the slash.
>
> Regarding the patch 2.  I see the current documentation in the patch
> doesn't explain to the user how to set the new parameter.  I think we
> should give users an idea what workloads need high values of
> multixact_local_cache_entries parameter and what doesn't.  Also, we
> should explain the negative aspects of high values
> multixact_local_cache_entries.  Ideally, we should get the advantage
> without overloading users with new nontrivial parameters, but I don't
> have a particular idea here.
>
> I'd like to propose committing 1 and 3, but leave 2 for further review.

Thanks for your review, Alexander!
+1 for avoiding double locking in SimpleLruReadPage_ReadOnly().
Other changes seem correct to me too.


I've tried to find optimal value for cache size and it seems to me that it affects multixact scalability much less than
sizesof offsets\members buffers. I concur that patch 2 of the patchset does not seem documented enough. 

Best regards, Andrey Borodin.