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

2016-05-06 Thread Kevin Grittner
On Fri, May 6, 2016 at 5:07 PM, Andres Freund 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

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

2016-05-06 Thread Andres Freund
Hi, 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 initia

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

2016-05-06 Thread Kevin Grittner
On Wed, Apr 20, 2016 at 8:08 PM, Ants Aasma wrote: > I had an idea I wanted to test out. The gist of it is to effectively > have the last slot of timestamp to xid map stored in the latest_xmin > field and only update the mapping when slot boundaries are crossed. > See attached WIP patch for detai

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

2016-05-03 Thread Andres Freund
On 2016-05-02 10:32:28 -0400, Robert Haas wrote: > You are certainly welcome to add a new open item to cover those > complaints. Done that. > But I do not want to blur together the discussion of > whether the feature is well-designed with the question of whether it > regresses performance when it

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

2016-05-02 Thread Bruce Momjian
On Mon, May 2, 2016 at 07:21:21AM -0700, Andres Freund wrote: > On 2016-05-02 09:03:19 -0400, Robert Haas wrote: > > On Fri, Apr 29, 2016 at 6:08 PM, Kevin Grittner wrote: > > > Now to continue with the performance benchmarks. I'm pretty sure > > > we've fixed the problems when the feature is di

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

2016-05-02 Thread Andres Freund
On 2016-05-02 18:15:40 +0300, Ants Aasma wrote: > On Mon, May 2, 2016 at 5:21 PM, Andres Freund wrote: > > On 2016-05-02 09:03:19 -0400, Robert Haas wrote: > >> On Fri, Apr 29, 2016 at 6:08 PM, Kevin Grittner wrote: > >> > Now to continue with the performance benchmarks. I'm pretty sure > >> > w

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

2016-05-02 Thread Ants Aasma
On Mon, May 2, 2016 at 5:21 PM, Andres Freund wrote: > On 2016-05-02 09:03:19 -0400, Robert Haas wrote: >> On Fri, Apr 29, 2016 at 6:08 PM, Kevin Grittner wrote: >> > Now to continue with the performance benchmarks. I'm pretty sure >> > we've fixed the problems when the feature is disabled >> >

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

2016-05-02 Thread Robert Haas
On Mon, May 2, 2016 at 10:21 AM, Andres Freund wrote: > On 2016-05-02 09:03:19 -0400, Robert Haas wrote: >> On Fri, Apr 29, 2016 at 6:08 PM, Kevin Grittner wrote: >> > Now to continue with the performance benchmarks. I'm pretty sure >> > we've fixed the problems when the feature is disabled >> >

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

2016-05-02 Thread Andres Freund
On 2016-05-02 09:03:19 -0400, Robert Haas wrote: > On Fri, Apr 29, 2016 at 6:08 PM, Kevin Grittner wrote: > > Now to continue with the performance benchmarks. I'm pretty sure > > we've fixed the problems when the feature is disabled > > (old_snapshot_threshold = -1), and there are several suggest

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

2016-05-02 Thread Robert Haas
On Fri, Apr 29, 2016 at 6:08 PM, Kevin Grittner wrote: > Now to continue with the performance benchmarks. I'm pretty sure > we've fixed the problems when the feature is disabled > (old_snapshot_threshold = -1), and there are several suggestions > for improving performance while it is on that need

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

2016-04-29 Thread Kevin Grittner
On Fri, Apr 22, 2016 at 8:06 AM, Kevin Grittner wrote: > On Thu, Apr 21, 2016 at 4:13 PM, Kevin Grittner wrote: > >> I have your test case running, and it is not immediately >> clear why old rows are not being vacuumed away. > > I have not found the reason that the vacuuming is not as aggressive

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

2016-04-23 Thread Amit Kapila
On Tue, Apr 19, 2016 at 8:41 PM, Kevin Grittner wrote: > > On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila wrote: > > On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund wrote: > >> > >> On 2016-04-16 16:44:52 -0400, Noah Misch wrote: > >> > That is more controversial than the potential ~2% regression f

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

2016-04-22 Thread Amit Kapila
On Thu, Apr 21, 2016 at 6:38 AM, Ants Aasma wrote: > > On Tue, Apr 19, 2016 at 6:11 PM, Kevin Grittner wrote: > > On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila wrote: > >> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund wrote: > >>> > >>> FWIW, I could be kinda convinced that it's temporarily ok,

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

2016-04-22 Thread Amit Kapila
On Wed, Apr 20, 2016 at 7:39 PM, Andres Freund wrote: > > On 2016-04-19 20:27:31 +0530, Amit Kapila wrote: > > On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund wrote: > > > > > > On 2016-04-16 16:44:52 -0400, Noah Misch wrote: > > > > That is more controversial than the potential ~2% regression for

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

2016-04-22 Thread Kevin Grittner
On Thu, Apr 21, 2016 at 4:13 PM, Kevin Grittner wrote: > I have your test case running, and it is not immediately > clear why old rows are not being vacuumed away. I have not found the reason that the vacuuming is not as aggressive as it should be with this old_snapshot_threshold, but I left you

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

2016-04-21 Thread Kevin Grittner
On Thu, Apr 21, 2016 at 2:10 PM, Ants Aasma wrote: > On Thu, Apr 21, 2016 at 5:16 PM, Kevin Grittner wrote: >> Could you provide enough to make that a self-contained >> reproducible test case [?] > [provided] Thanks! I have your test case running, and it is not immediately clear why old rows

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

2016-04-21 Thread Ants Aasma
On Thu, Apr 21, 2016 at 5:16 PM, Kevin Grittner wrote: > On Wed, Apr 20, 2016 at 8:08 PM, Ants Aasma wrote: > >> However, while checking out if my proof of concept patch actually >> works I hit another issue. I couldn't get my test for the feature to >> actually work. The test script I used is at

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

2016-04-21 Thread Kevin Grittner
On Wed, Apr 20, 2016 at 8:08 PM, Ants Aasma wrote: > However, while checking out if my proof of concept patch actually > works I hit another issue. I couldn't get my test for the feature to > actually work. The test script I used is attached. Could you provide enough to make that a self-containe

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

2016-04-20 Thread Ants Aasma
On Tue, Apr 19, 2016 at 6:11 PM, Kevin Grittner wrote: > On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila wrote: >> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund wrote: >>> >>> On 2016-04-16 16:44:52 -0400, Noah Misch wrote: >>> > That is more controversial than the potential ~2% regression for >>>

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

2016-04-20 Thread Andres Freund
On 2016-04-19 20:27:31 +0530, Amit Kapila wrote: > On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund wrote: > > > > On 2016-04-16 16:44:52 -0400, Noah Misch wrote: > > > That is more controversial than the potential ~2% regression for > > > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay

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

2016-04-19 Thread Amit Kapila
On Tue, Apr 19, 2016 at 8:44 PM, Robert Haas wrote: > > On Tue, Apr 19, 2016 at 11:11 AM, Kevin Grittner wrote: > > On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila wrote: > > >> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping() > >> takes EXCLUSIVE LWLock which seems to be a p

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

2016-04-19 Thread Kevin Grittner
On Tue, Apr 19, 2016 at 10:14 AM, Robert Haas wrote: > On Tue, Apr 19, 2016 at 11:11 AM, Kevin Grittner wrote: >> On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila wrote: >>> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund wrote: On 2016-04-16 16:44:52 -0400, Noah Misch wrote: > That i

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

2016-04-19 Thread Robert Haas
On Tue, Apr 19, 2016 at 11:11 AM, Kevin Grittner wrote: > On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila wrote: >> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund wrote: >>> >>> On 2016-04-16 16:44:52 -0400, Noah Misch wrote: >>> > That is more controversial than the potential ~2% regression for >>

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

2016-04-19 Thread Kevin Grittner
On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila wrote: > On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund wrote: >> >> On 2016-04-16 16:44:52 -0400, Noah Misch wrote: >> > That is more controversial than the potential ~2% regression for >> > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay

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

2016-04-19 Thread Amit Kapila
On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund wrote: > > On 2016-04-16 16:44:52 -0400, Noah Misch wrote: > > That is more controversial than the potential ~2% regression for > > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing > > that way, and Andres[4] is not. > > FWIW, I

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

2016-04-17 Thread Andres Freund
> Second, I don't think it will improve and become performant without > exposure to a wider audience. Huh? The issue is a relatively simple to spot architectural issue (taking a single exclusive lock during snapshot acquiration which only needs shared locks otherwise) - I don't see how any input i

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

2016-04-17 Thread David Steele
On 4/16/16 4:44 PM, Noah Misch wrote: > The key judgment to finalize here is whether it's okay to release this feature > given its current effect[1], when enabled, on performance. That is more > controversial than the potential ~2% regression for old_snapshot_threshold=-1. > Alvaro[2] and Robert[

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

2016-04-16 Thread Andres Freund
Hi, On 2016-04-16 18:27:06 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2016-04-16 17:52:44 -0400, Tom Lane wrote: > >> That's more than a 5X penalty, which seems like it would make the > >> feature unusable; unless there is an argument that that's an extreme > >> case that wouldn't be r

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

2016-04-16 Thread Tom Lane
Andres Freund writes: > On 2016-04-16 17:52:44 -0400, Tom Lane wrote: >> That's more than a 5X penalty, which seems like it would make the >> feature unusable; unless there is an argument that that's an extreme >> case that wouldn't be representative of most real-world usage. >> Which there may we

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

2016-04-16 Thread Andres Freund
On 2016-04-16 17:52:44 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2016-04-16 16:44:52 -0400, Noah Misch wrote: > >> That is more controversial than the potential ~2% regression for > >> old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing > >> that way, and Andres[4] i

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

2016-04-16 Thread Tom Lane
Andres Freund writes: > On 2016-04-16 16:44:52 -0400, Noah Misch wrote: >> That is more controversial than the potential ~2% regression for >> old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing >> that way, and Andres[4] is not. > FWIW, I could be kinda convinced that it's tem

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

2016-04-16 Thread Andres Freund
On 2016-04-16 16:44:52 -0400, Noah Misch wrote: > That is more controversial than the potential ~2% regression for > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing > that way, and Andres[4] is not. FWIW, I could be kinda convinced that it's temporarily ok, if there'd be a c

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

2016-04-14 Thread Alexander Korotkov
On Thu, Apr 14, 2016 at 12:23 AM, Andres Freund wrote: > On 2016-04-13 16:05:25 -0500, Kevin Grittner wrote: > > OK, thanks. I can't think of anything else to ask for at this > > point. If you feel that you have enough to press for some > > particular course of action, go for it. > > I think we

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

2016-04-13 Thread Andres Freund
On 2016-04-13 16:05:25 -0500, Kevin Grittner wrote: > OK, thanks. I can't think of anything else to ask for at this > point. If you feel that you have enough to press for some > particular course of action, go for it. I think we, at the very least, need a clear proposal how to resolve the scalab

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

2016-04-13 Thread Kevin Grittner
On Wed, Apr 13, 2016 at 3:47 PM, Andres Freund wrote: > On 2016-04-13 15:21:31 -0500, Kevin Grittner wrote: >> What is the kernel on which these tests were run? > > 3.16. I can upgrade to 4.4 if necessary. No, I'm not aware of any problems from 3.8 on. > But I still believe very strongly that t

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

2016-04-13 Thread Andres Freund
On 2016-04-13 15:21:31 -0500, Kevin Grittner wrote: > On Wed, Apr 13, 2016 at 3:01 PM, Andres Freund wrote: > > > If you want me to rn some other tests I can, but ISTM we have the > > data we need? > > Thanks for the additional detail on how this was run. I think I > still need a little more co

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

2016-04-13 Thread Kevin Grittner
On Wed, Apr 13, 2016 at 3:01 PM, Andres Freund wrote: > If you want me to rn some other tests I can, but ISTM we have the > data we need? Thanks for the additional detail on how this was run. I think I still need a little more context, though: What is the kernel on which these tests were run?

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

2016-04-13 Thread Andres Freund
Hi Kevin, On 2016-04-13 12:21:10 -0700, Andres Freund wrote: > 0: > progress: 100.0 s, 593351.0 tps, lat 0.215 ms stddev 0.118 > progress: 200.0 s, 594035.9 tps, lat 0.215 ms stddev 0.118 > progress: 300.0 s, 594013.3 tps, lat 0.215 ms stddev 0.117 > > -1: > progress: 100.0 s, 600835.3 tps, lat 0

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

2016-04-13 Thread Andres Freund
On 2016-04-13 14:08:49 -0500, Kevin Grittner wrote: > On Wed, Apr 13, 2016 at 1:56 PM, Andres Freund wrote: > > > I'll run with -1 once the current (longer) run has finished. > > Just for the record, were any of the other results purporting to be > with the feature "off" also actually running wi

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

2016-04-13 Thread Robert Haas
On Wed, Apr 13, 2016 at 3:08 PM, Kevin Grittner wrote: > On Wed, Apr 13, 2016 at 1:56 PM, Andres Freund wrote: > >> I'll run with -1 once the current (longer) run has finished. > > Just for the record, were any of the other results purporting to be > with the feature "off" also actually running w

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

2016-04-13 Thread Andres Freund
On 2016-04-13 13:52:15 -0500, Kevin Grittner wrote: > On Wed, Apr 13, 2016 at 12:25 PM, Robert Haas wrote: > > > [test results with old_snapshot_threshold = 0 and 10] > > From the docs: > > | A value of -1 disables this feature, and is the default. > > > Yuck. Aside from the fact that performance

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

2016-04-13 Thread Jim Nasby
On 4/12/16 12:30 PM, Tom Lane wrote: It'd be good if you document the problems you found somewhere, before you forget them, just in case somebody does want to try to lift the restriction. I agree that scattered code comments wouldn't be the way. Just a quick email to -hackers to get the info int

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

2016-04-13 Thread Kevin Grittner
On Wed, Apr 13, 2016 at 1:56 PM, Andres Freund wrote: > I'll run with -1 once the current (longer) run has finished. Just for the record, were any of the other results purporting to be with the feature "off" also actually running with the feature set for its fastest possible timeout? -- Kevin G

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

2016-04-13 Thread Andres Freund
On 2016-04-13 13:52:15 -0500, Kevin Grittner wrote: > On Wed, Apr 13, 2016 at 12:25 PM, Robert Haas wrote: > > > [test results with old_snapshot_threshold = 0 and 10] > > From the docs: > > | A value of -1 disables this feature, and is the default. Hm, ok, let me run that as well then. The rea

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

2016-04-13 Thread Kevin Grittner
On Wed, Apr 13, 2016 at 12:25 PM, Robert Haas wrote: > [test results with old_snapshot_threshold = 0 and 10] >From the docs: | A value of -1 disables this feature, and is the default. > Yuck. Aside from the fact that performance tanks when the feature is > turned on, it seems that there is a

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

2016-04-13 Thread Andres Freund
On 2016-04-13 13:25:14 -0400, Robert Haas wrote: > > With -c old_snapshot_threshold=0: > > > > latency average = 0.218 ms > > latency stddev = 0.154 ms > > tps = 584666.289753 (including connections establishing) > > tps = 584867.785569 (excluding connections establishing) > > > > > > With -c old_s

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

2016-04-13 Thread Robert Haas
On Wed, Apr 13, 2016 at 1:19 PM, Andres Freund wrote: > On an EC2 m4.10xlarge (dedicated, but still a VM) - sorry I don't have > anything better at hand right now, and it was already running. > > postgres config: > postgres -D /srv/data/dev/ > -c shared_buffers=64GB \ > -c max_wa

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

2016-04-13 Thread Andres Freund
On 2016-04-12 14:53:57 -0500, Kevin Grittner wrote: > On Tue, Apr 12, 2016 at 2:28 PM, Andres Freund wrote: > > On 2016-04-12 14:17:12 -0500, Kevin Grittner wrote: > >> Well, something is different between your environment and mine, > >> since I saw no difference at scale 100 and 2.2% at scale 200

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

2016-04-13 Thread Kevin Grittner
On Wed, Apr 13, 2016 at 10:59 AM, Andres Freund wrote: >> but as an example, if I only see such regression on a Linux kernel >> with version a version < 3.8 I am going to be less concerned about >> getting something into 9.6, since IMO it is completely irresponsible >> to run a NUMA machine with

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

2016-04-13 Thread Andres Freund
On 2016-04-13 10:31:19 -0500, Kevin Grittner wrote: > With a real-world application with realistic simulated user load > there was no such regression and a big gain in performance over > time, so we're talking about adjusting how broad a range of > workloads it benefits. I think it depends very he

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

2016-04-13 Thread Andres Freund
On 2016-04-13 11:27:09 -0400, Robert Haas wrote: > That page is sort of confusing, because it says that platform has > those things but then says ***, which is footnoted to mean "linux > kernel emulation available", but it's not too clear whether that > applies to all atomics or just 8-byte atomics

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

2016-04-13 Thread Kevin Grittner
On Wed, Apr 13, 2016 at 10:01 AM, Andres Freund wrote: > My problem with that is that snapshot-too-old is essentially a > efficiency feature for busy and large databases. Regressing noticeably > when it's enabled in it's natural habitat seems sad. With a real-world application with realistic sim

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

2016-04-13 Thread Tom Lane
Robert Haas writes: > On Wed, Apr 13, 2016 at 10:42 AM, Tom Lane wrote: >> No, you're ignoring my point, which is what happens on single-CPU >> 32-bit machines, and whether we aren't going to destroy performance >> on low-end machines in pursuit of better performance on high-end. > One of us is

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

2016-04-13 Thread Robert Haas
On Wed, Apr 13, 2016 at 11:18 AM, Andres Freund wrote: > I think generally the only platform of concern wrt is arm (< armv8), > which doesn't have 64bit atomicity and doesn't have > single-copy-atomicity for 8 byte values either (C.f. > https://wiki.postgresql.org/wiki/Atomics). That page is sort

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

2016-04-13 Thread Andres Freund
On 2016-04-13 11:08:21 -0300, Alvaro Herrera wrote: > The patch being proposed for commit is fiddly architecture-specific > stuff which is likely to destabilize the tree for quite some time, and > cause lots of additional work to Andres and anyone else likely to work > on such low-level details, su

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

2016-04-13 Thread Andres Freund
On 2016-04-13 10:42:03 -0400, Tom Lane wrote: > Robert Haas writes: > > On Wed, Apr 13, 2016 at 10:20 AM, Tom Lane wrote: > >> That's what I thought you were going to say, and it means that any > >> "performance improvement" patch that relies on 64-bit atomics in hotspot > >> code paths is going

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

2016-04-13 Thread Robert Haas
On Wed, Apr 13, 2016 at 11:01 AM, Andres Freund wrote: > Well, I'm less likely to write a patch when there's no chance that it's > going to be applied. Which the rest of the thread sounds like... I hope somebody writes it at some point, because we surely want to fix this for 9.7. However, I agre

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

2016-04-13 Thread Andres Freund
On 2016-04-13 08:36:47 -0400, Robert Haas wrote: > I think that a significant performance regression which affects people > not using snapshot_too_old would be a stop-ship issue, but I disagree > that an issue which only affects people using the feature is a > must-fix. It may be desirable to fix

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

2016-04-13 Thread Robert Haas
On Wed, Apr 13, 2016 at 10:42 AM, Tom Lane wrote: > Robert Haas writes: >> On Wed, Apr 13, 2016 at 10:20 AM, Tom Lane wrote: >>> That's what I thought you were going to say, and it means that any >>> "performance improvement" patch that relies on 64-bit atomics in hotspot >>> code paths is going

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

2016-04-13 Thread Tom Lane
Robert Haas writes: > On Wed, Apr 13, 2016 at 10:20 AM, Tom Lane wrote: >> That's what I thought you were going to say, and it means that any >> "performance improvement" patch that relies on 64-bit atomics in hotspot >> code paths is going to be a complete disaster on anything but modern Intel >

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

2016-04-13 Thread Robert Haas
On Wed, Apr 13, 2016 at 10:20 AM, Tom Lane wrote: > Robert Haas writes: >> On Wed, Apr 13, 2016 at 9:52 AM, Tom Lane wrote: >>> Robert Haas writes: I have never understood why you didn't include 64-bit atomics in the original atomics implementation, and I really think we should have >

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

2016-04-13 Thread Tom Lane
Robert Haas writes: > On Wed, Apr 13, 2016 at 9:52 AM, Tom Lane wrote: >> Robert Haas writes: >>> I have never understood why you didn't include 64-bit atomics in the >>> original atomics implementation, and I really think we should have >>> committed a patch to add them long before now. >> Wha

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

2016-04-13 Thread Robert Haas
On Wed, Apr 13, 2016 at 9:52 AM, Tom Lane wrote: > Robert Haas writes: >> I have never understood why you didn't include 64-bit atomics in the >> original atomics implementation, and I really think we should have >> committed a patch to add them long before now. > > What will you do on 32-bit pla

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

2016-04-13 Thread Alvaro Herrera
Robert Haas wrote: > On Tue, Apr 12, 2016 at 11:05 PM, Andres Freund wrote: > > I didn't plan to do anything without a few +1's. I don't think we can > > release with the state of things as is though. I don't see a less > > intrusive way than to get rid of that spinlock on all platforms capable >

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

2016-04-13 Thread Tom Lane
Robert Haas writes: > I have never understood why you didn't include 64-bit atomics in the > original atomics implementation, and I really think we should have > committed a patch to add them long before now. What will you do on 32-bit platforms (or, more generally, anything lacking 64-bit-wide a

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

2016-04-13 Thread Robert Haas
On Tue, Apr 12, 2016 at 11:05 PM, Andres Freund wrote: > On 2016-04-12 23:52:14 -0300, Alvaro Herrera wrote: >> Andres Freund wrote: >> > I'm kinda inclined to apply that portion (or just the whole patch with >> > the spurious #ifdef 0 et al fixed) into 9.6; and add the necessary >> > checks in a

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

2016-04-12 Thread Andres Freund
On 2016-04-12 23:52:14 -0300, Alvaro Herrera wrote: > Andres Freund wrote: > > > I'm kinda inclined to apply that portion (or just the whole patch with > > the spurious #ifdef 0 et al fixed) into 9.6; and add the necessary > > checks in a few places. Because I really think this is likely to hit >

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

2016-04-12 Thread Alvaro Herrera
Andres Freund wrote: > I'm kinda inclined to apply that portion (or just the whole patch with > the spurious #ifdef 0 et al fixed) into 9.6; and add the necessary > checks in a few places. Because I really think this is likely to hit > unsuspecting users. !!! Be sure to consult with the RMT befo

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

2016-04-12 Thread Kevin Grittner
On Tue, Apr 12, 2016 at 2:53 PM, Kevin Grittner wrote: > Readonly with client and job counts matching scale. Single-socket i7, BTW. >> A lot of this will be different between >> single-socket and multi-socket servers; as soon as you have the latter >> the likelihood of contention being bad goes

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

2016-04-12 Thread Kevin Grittner
On Tue, Apr 12, 2016 at 2:28 PM, Andres Freund wrote: > On 2016-04-12 14:17:12 -0500, Kevin Grittner wrote: >> Well, something is different between your environment and mine, >> since I saw no difference at scale 100 and 2.2% at scale 200. > > In a readonly test or r/w? Readonly with client and j

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

2016-04-12 Thread Andres Freund
Hi, On 2016-04-12 14:17:12 -0500, Kevin Grittner wrote: > Well, something is different between your environment and mine, > since I saw no difference at scale 100 and 2.2% at scale 200. In a readonly test or r/w? A lot of this will be different between single-socket and multi-socket servers; as

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

2016-04-12 Thread Kevin Grittner
On Tue, Apr 12, 2016 at 1:56 PM, Andres Freund wrote: > On 2016-04-12 13:44:00 -0500, Kevin Grittner wrote: >> On Tue, Apr 12, 2016 at 12:38 PM, Andres Freund wrote: >>> On 2016-04-12 16:49:25 +, Kevin Grittner wrote: On a big NUMA machine with 1000 connections in saturation load th

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

2016-04-12 Thread Andres Freund
On 2016-04-12 13:44:00 -0500, Kevin Grittner wrote: > On Tue, Apr 12, 2016 at 12:38 PM, Andres Freund wrote: > > On 2016-04-12 16:49:25 +, Kevin Grittner wrote: > >> On a big NUMA machine with 1000 connections in saturation load > >> there was a performance regression due to spinlock contentio

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

2016-04-12 Thread Tom Lane
Kevin Grittner writes: > On Tue, Apr 12, 2016 at 12:08 PM, Alvaro Herrera > wrote: >> old_snapshot_threshold is PGC_POSTMASTER, so this is okay AFAICS, but >> perhaps it'd be a good idea to add a oneline comment to guc.c indicating >> to verify this code if there's an intention to lift that limit

<    1   2