Re: Reduce ProcArrayLock contention

On Wed, Jul 29, 2015 at 11:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> On 2015-07-29 12:54:59 -0400, Robert Haas wrote:
> > I would try to avoid changing lwlock.c. It's pretty easy when so
> > doing to create mechanisms that work now but make further upgrades to
> > the general lwlock mechanism difficult. I'd like to avoid that.
>
> I'm massively doubtful that re-implementing parts of lwlock.c is the
> better outcome. Then you have two different infrastructures you need to
> improve over time.

I agree and modified the patch to use 32-bit atomics based on idea
suggested by Robert and didn't modify lwlock.c.

> I understand. IMHO it will be a good idea though to ensure that the patch
does not cause regression for other setups such as a less powerful
> machine or while running with lower number of clients.
>

Okay, I have tried it on CentOS VM, but the data is totally screwed (at
same client count across 2 runs the variation is quite high ranging from 300
to 3000 tps) to make any meaning out of it. I think if you want to collect
data
on less powerful m/c, then also atleast we should ensure that it is taken in
a way that we are sure that it is not due to noise and there is actual
regression,
then only we can decide whether we should investigate that or not.
Can you please try taking data with attached script
(perf_pgbench_tpcb_write.sh), few things you need to change in script
based on your setup (environment variables defined in beginning of script
like PGDATA), other thing is that you need to ensure that name of binaries
for HEAD and Patch should be changed in script if you are naming them
differently. It will generate the performance data in test*.txt files.
Also try
to ensure that checkpoint should be configured such that it should not
occur in-between tests, else it will be difficult to conclude the results.
Some parameters you might want to consider for the same are
checkpoint_timeout, checkpoint_completion_target, min_wal_size,
max_wal_size.

>>
>>
>> I was telling that fact even without my patch. Basically I have
>> tried by commenting ProcArrayLock in ProcArrayEndTransaction.
>>

> I did not get that. You mean the TPS is same if you run with commenting
out ProcArrayLock in ProcArrayEndTransaction?

Yes, TPS is almost same as with Patch.

> Is that safe to do?

No, that is not safe. I have done that just to see what is the maximum
gain we can get by reducing the contention around ProcArrayLock.

>>
>>
>>
>> No, autovacuum generates I/O due to which sometimes there
>> is more variation in Write tests.

> Sure, but on an average does it still show similar improvements?
>

Yes, number with and without autovacuum show similar trend, it's just
that you can see somewhat better results (more performance improvement)
with autovacuum=off and do manual vacuum at end of each test.

> Or does the test becomes IO bound and hence the bottleneck shifts
somewhere
> else? Can you please post those numbers as well when you get chance?

The numbers posted in initial mail or in this mail are with autovacuum =on.

>>
>>
>>
>> So among these only step 2 can be common among different
>> algorithms, other's need some work specific to each optimization.
>>

> Right, but if we could encapsulate that work in a function that just
needs to work on some shared memory, I think we can build such
infrastructure.

For now, I have encapsulated the code into 2 separate functions, rather
than extending LWLock.c as that could easily lead to code which might
not be used in future even though currently it sounds like that and
in-future
if we need to use same technique elsewhere then we can look into it.

>
>
>> >
>> > Regarding the patch, the compare-and-exchange function calls that
you've used would work only for 64-bit machines, right? You would need to
use equivalent 32-bit calls on a 32-bit machine.
> >
>>
>> I thought that internal API will automatically take care of it,
>> example for msvc it uses _InterlockedCompareExchange64
>> which if doesn't work on 32-bit systems or is not defined, then
>> we have to use 32-bit version, but I am not certain about
>> that fact.
>>

> Hmm. The pointer will be a 32-bit field on a 32-bit machine. I don't know
how exchanging that with 64-bit integer be safe.
>

True, but that has to be in-general taken care by 64bit atomic API's,
like in this case it should fallback to either 32-bit version of API or
something that can work on 32-bit m/c. I think fallback support
is missing as of now in 64-bit API's which we should have if we want
to use those API's, but anyway for now I have modified the patch to
use 32-bit atomics.

Performance Data with modified patch.

pgbench setup
------------------------
scale factor - 300
Data is on magnetic disk and WAL on ssd.
pgbench -M prepared tpc-b

Data is median of 3 - 30 min runs, the detailed data for
all the 3 runs is in attached document
(perf_write_procarraylock_data.ods)

Head : commit c53f7387
Patch : group_xid_clearing_at_trans_end_rel_v2

Client_Count/Patch_ver (TPS) 1 8 16 32 64 128 HEAD 972 6004 11060 20074
23839 17798 Patch 1005 6260 11368 20318 30775 30215

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com