Hi, Tomas!
Thank you for your review.
On Wed, Oct 28, 2020 at 4:36 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> I did a quick review on this patch series. A couple comments:
>
>
> 0001
> ----
>
> This looks quite suspicious to me - SimpleLruReadPage_ReadOnly is
> changed to return information about what lock was used, merely to allow
> the callers to do an Assert() that the value is not LW_NONE.
Yes, but this is not merely to allow callers to do an Assert().
Sometimes in multixacts it could save us some relocks. So, we can
skip relocking lock to exclusive mode if it's in exclusive already.
Adding Assert() to every caller is probably overkill.
> IMO we could achieve exactly the same thing by passing a simple flag
> that would say 'make sure we got a lock' or something like that. In
> fact, aren't all callers doing the assert? That'd mean we can just do
> the check always, without the flag. (I see GetMultiXactIdMembers does
> two calls and only checks the second result, but I wonder if that's
> intended or omission.)
Having just the flag is exactly what the original version by Andrey
did. But if we have to read two multixact offsets pages or multiple
members page in one GetMultiXactIdMembers()), then it does relocks
from exclusive mode to exclusive mode. I decide that once we decide
to optimize this locks, this situation is nice to evade.
> In any case, it'd make the lwlock.c changes unnecessary, I think.
I agree that it would be better to not touch lwlock.c. But I didn't
find a way to evade relocking exclusive mode to exclusive mode without
touching lwlock.c or making code cumbersome in other places.
> 0002
> ----
>
> Specifies the number cached MultiXact by backend. Any SLRU lookup ...
>
> should be 'number of cached ...'
Sounds reasonable.
> 0003
> ----
>
> * Conditional variable for waiting till the filling of the next multixact
> * will be finished. See GetMultiXactIdMembers() and RecordNewMultiXact()
> * for details.
>
> Perhaps 'till the next multixact is filled' or 'gets full' would be
> better. Not sure.
Sounds reasonable as well.
------
Regards,
Alexander Korotkov