Hi! On Mon, Sep 28, 2020 at 5:41 PM Andrey M. Borodin <x4...@yandex-team.ru> wrote: > > 28 авг. 2020 г., в 23:08, Anastasia Lubennikova > > <a.lubennik...@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 reproduce problem from production... > > You are absolutely correct in point 2: I did only tests with sane values. And > observed extreme performance degradation with values ~ 64 megabytes. I do not > know which highest values should we pick? 1Gb? Or highest possible > functioning value? > > 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. ------ Regards, Alexander Korotkov
v4-0001-Use-shared-lock-in-GetMultiXactIdMembers-for-offsets.patch
Description: Binary data
v4-0002-Make-MultiXact-local-cache-size-configurable.patch
Description: Binary data
v4-0003-Add-conditional-variable-to-wait-for-next-MultXact-o.patch
Description: Binary data
