Re: Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <

On Fri, May 6, 2016 at 5:07 PM, Andres Freund <andres@anarazel.de> wrote:

> On 2016-05-06 14:18:22 -0500, Kevin Grittner wrote:
>> I rebased the patch Ants posted (attached), and am running
>> benchmarks on a cthulhu (a big NUMA machine with 8 memory nodes).
>> Normally I wouldn't post results without a lot more data points
>> with multiple samples at each, but the initial results have me
>> wondering whether people would like to see this pushed later today
>> so that it has some time in the buildfarm and then into beta1.
>
> I think that generally would make sense. We quite possibly need some
> further changes, but it seems more likely that we can find them if the
> patch runs close to the disabled performance.

ok

>> Running the r/w TPC-B (sort of) load with scale, jobs, and threads
>> at 1000, and the database configured as I would for a production
>> server of that size, preliminary TPS results are:
>>
>> master, -1:  8158
>> master, 10min: 2019
>> Ants' patch, 10min: 7804
>
> That's rather nice.  Did you test read-only as well?

Not yet.  I don't trust short runs, so I've been going with -T2400;
with setup times and so on, that limits me to one run per hour of
time I book the machine, and I'm competing with others for that.  I
do plan to run read-only, too.

From the 40 minute tests so far with Ants' patch (alternating settings):

old_snapshot_threshold = 10
7804
9524
9512

old_snapshot_threshold = -1
10421
8691
8977

It's disappointing that I am not getting more consistent numbers,
but NUMA can be hard to manage that way.

> If you'd feel more comfortable committing after I've run some
> performance tests, I could kick off some soon.

I think I should get it onto the buildfarm if we're going for
beta2, so there's time to recognize any problem (unlikely as that
*seems*) and back this out before beta if needed.  That said, all
additional data points welcome!

>> I can see arguments for tuning this far in time for the beta, as
>> well as the argument to wait until after the beta, so I'm just
>> throwing it out there to see what other people think.  I wouldn't
>> do it unless I have three runs at -1 and 10min with the patch, all
>> showing similar numbers.  If the BF chokes on it I would revert
>> this optimization attempt.
>
> +1 for going forward.  I'm still doubtful that it's a good idea to the
> map maintenance from GetSnapshotData(), but the issue becomes much less
> severe when addressed like this.
>
> The primary reasons why I'd like to move it is because of the
> significant amount of added gettimeofday() calls which are a real hog in
> some virtualized environments, and because I'm doubtful of tying the
> current time to the xmin horizon.

When I initially presented the proof of concept patch during the
9.5 development cycle it was based on transaction counts, and that
was the biggest criticism, and it came from many quarters.  Using
time was the big demand from just about everyone, and I'm not sure
how you do that without a mapping of time to xmin horizon.  If you
have some other idea, I'm all ears.

> Looks roughly sensible.

Will push shortly with the nit-pick fixes you requested.

Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company