Date: Thu, 29 Oct 2020 00:21:01 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Andrey Borodin <x4mmm@yandex-team.ru>,
	Anastasia Lubennikova <a.lubennikova@postgrespro.ru>,
	Daniel Gustafsson <daniel@yesql.se>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: MultiXact\SLRU buffers configuration
Message-ID: <20201028232101.4awgmc2cu2cemu7p@development>
References: <BA079D14-8D11-4A73-B6EC-FE393C712439@yesql.se>
 <2ECE132B-C042-4489-930E-DBC5D0DAB84A@yandex-team.ru>
 <a7f1c4e1-1015-92a4-2bd4-6736bd13d03e@postgrespro.ru>
 <2087E87D-44CA-4443-8E6A-5087F07443F4@yandex-team.ru>
 <CAPpHfduekT=BrM3rLqvo4ajzKYmDk+6aCoYJrNHhK0Je+v2z=Q@mail.gmail.com>
 <8A94938B-054C-4439-9866-2C220B4D0DD7@yandex-team.ru>
 <CAPpHfduT0Kb+8fMt4t0EF5G_3hig+wYgeRadQnNq20F4VxP7LA@mail.gmail.com>
 <CAPpHfdsUtSGxRrKn65F_wrE66sNztNY4TpDGDHh2kvAZZDOMqA@mail.gmail.com>
 <20201028013651.de5cj2xadgmba5nf@development>
 <CAPpHfdsC7-ZkaKD9xYoVejdD5OeGc3jr7du3-84nkHuNGuKnKQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAPpHfdsC7-ZkaKD9xYoVejdD5OeGc3jr7du3-84nkHuNGuKnKQ@mail.gmail.com>
Precedence: bulk

On Wed, Oct 28, 2020 at 10:36:39PM +0300, Alexander Korotkov wrote:
>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.
>

Hmm, OK. That can only happen in GetMultiXactIdMembers, which is the
only place where we do retry, right? Do we actually know this makes any
measurable difference? It seems we're mostly imagining that it might
help, but we don't have any actual proof of that (e.g. a workload which
we might benchmark). Or am I missing something?

For me, the extra conditions make it way harder to reason about the
behavior of the code, and I can't convince myself it's worth it.


>> 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.
>

Hmm. OK.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services