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.
------
Regards,
Alexander Korotkov
