public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tomas Vondra <[email protected]>
To: Alexander Korotkov <[email protected]>
Cc: Andrey Borodin <[email protected]>
Cc: Anastasia Lubennikova <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: MultiXact\SLRU buffers configuration
Date: Wed, 28 Oct 2020 02:36:51 +0100
Message-ID: <20201028013651.de5cj2xadgmba5nf@development> (raw)
In-Reply-To: <CAPpHfdsUtSGxRrKn65F_wrE66sNztNY4TpDGDHh2kvAZZDOMqA@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAPpHfduekT=BrM3rLqvo4ajzKYmDk+6aCoYJrNHhK0Je+v2z=Q@mail.gmail.com>
<[email protected]>
<CAPpHfduT0Kb+8fMt4t0EF5G_3hig+wYgeRadQnNq20F4VxP7LA@mail.gmail.com>
<CAPpHfdsUtSGxRrKn65F_wrE66sNztNY4TpDGDHh2kvAZZDOMqA@mail.gmail.com>
On Tue, Oct 27, 2020 at 08:23:26PM +0300, Alexander Korotkov wrote:
>On Tue, Oct 27, 2020 at 8:02 PM Alexander Korotkov <[email protected]> wrote:
>> On Mon, Oct 26, 2020 at 6:45 PM Andrey Borodin <[email protected]> wrote:
>> > 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 sizes of offsets\members buffers. I concur that patch 2 of the patchset does not seem documented enough.
>>
>> Thank you. I've made a few more minor adjustments to the patchset.
>> I'm going to push 0001 and 0003 if there are no objections.
>
>I get that patchset v5 doesn't pass the tests due to typo in assert.
>The fixes version is attached.
>
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.
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.)
In any case, it'd make the lwlock.c changes unnecessary, I think.
0002
----
Specifies the number cached MultiXact by backend. Any SLRU lookup ...
should be 'number of cached ...'
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.
This thread started with a discussion about making the SLRU sizes
configurable, but this patch version only adds a local cache. Does this
achieve the same goal, or would we still gain something by having GUCs
for the SLRUs?
If we're claiming this improves performance, it'd be good to have some
workload demonstrating that and measurements. I don't see anything like
that in this thread, so it's a bit hand-wavy. Can someone share details
of such workload (even synthetic one) and some basic measurements?
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
view thread (81+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: MultiXact\SLRU buffers configuration
In-Reply-To: <20201028013651.de5cj2xadgmba5nf@development>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox