Thread: [HACKERS] Use sync commit for logical replication apply in TAP tests
Hi, The commit 887227a1c changed the defaults for subscriptions to do async commit. But since the tests often wait for disk flush and there is no concurrent activity this has increased the amount of time needed for each test. So the attached patch changes the subscriptions create in tab tests to use sync commit which improves performance there (because we also replicate only very few transactions in the tests). -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 4/18/17 22:25, Petr Jelinek wrote: > The commit 887227a1c changed the defaults for subscriptions to do async > commit. But since the tests often wait for disk flush and there is no > concurrent activity this has increased the amount of time needed for > each test. So the attached patch changes the subscriptions create in tab > tests to use sync commit which improves performance there (because we > also replicate only very few transactions in the tests). I see only a 1.5% (1s/70s) improvement in the run time of the whole test suite from this. Is that what you were expecting? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 20/04/17 14:57, Peter Eisentraut wrote: > On 4/18/17 22:25, Petr Jelinek wrote: >> The commit 887227a1c changed the defaults for subscriptions to do async >> commit. But since the tests often wait for disk flush and there is no >> concurrent activity this has increased the amount of time needed for >> each test. So the attached patch changes the subscriptions create in tab >> tests to use sync commit which improves performance there (because we >> also replicate only very few transactions in the tests). > > I see only a 1.5% (1s/70s) improvement in the run time of the whole test > suite from this. Is that what you were expecting? > Hmm, on my machine the difference is around 50% (~1m vs ~2m). -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 4/20/17 09:46, Petr Jelinek wrote: > On 20/04/17 14:57, Peter Eisentraut wrote: >> On 4/18/17 22:25, Petr Jelinek wrote: >>> The commit 887227a1c changed the defaults for subscriptions to do async >>> commit. But since the tests often wait for disk flush and there is no >>> concurrent activity this has increased the amount of time needed for >>> each test. So the attached patch changes the subscriptions create in tab >>> tests to use sync commit which improves performance there (because we >>> also replicate only very few transactions in the tests). >> >> I see only a 1.5% (1s/70s) improvement in the run time of the whole test >> suite from this. Is that what you were expecting? > > Hmm, on my machine the difference is around 50% (~1m vs ~2m). Double hmm, I ran a few more tests and different machines and get consistent but underwhelming improvements. I don't mind applying the patch nonetheless, but maybe we can get a few more test results from others. (Instructions: apply the patch and time make -C src/test/subscription check) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: > Double hmm, I ran a few more tests and different machines and get > consistent but underwhelming improvements. > I don't mind applying the patch nonetheless, but maybe we can get a few > more test results from others. > (Instructions: apply the patch and time make -C src/test/subscription check) OK, here's some results. All are typical debug builds (--enable-cassert in particular), and each number cited is best result of three runs: UNPATCHED PATCHED sss1 1m13.828s 1m12.763s (H/W RAID controller + spinning rust) laptop 1m9.236s 1m7.969s (Macbook Pro, SSD) gaur 11m25.58s 11m14.04s (1990s-era spinning rust) prairiedog 8m37.848s 9m26.256s (ATA/66, 5400RPM spinning rust) (For context, about 2m10s of gaur's cycle time is the temp install preparation, and 32s of prairiedog's; it's just a few seconds on the newer machines.) I have little faith in the numbers from gaur because I saw run-to-run variations of a couple of minutes; I suspect that the bgworker launching bug I described yesterday is making itself felt in this test too. prairiedog also had rather more variance than one would expect, making me wonder if something similar is happening there. But based on these results, I would say that as a test-speed enhancement, this patch is a failure. I'd also question the wisdom of testing only a non-default code path. There could be an argument for running some of the subscription tests with async commit and some without, just to improve code coverage. regards, tom lane