> Vladimir, can I have your opinion on the latest patch or if you want to make changes, an updated patch?
I am satisfied with all our changes and I thinks it enough to complete this PR. 2016-10-03 6:51 GMT+03:00 Craig Ringer <cr...@2ndquadrant.com>: > On 3 Oct. 2016 10:15, "Michael Paquier" <michael.paqu...@gmail.com> wrote: > > > > On Tue, Sep 27, 2016 at 11:05 AM, Craig Ringer <cr...@2ndquadrant.com> > wrote: > > > On 26 September 2016 at 21:52, Vladimir Gordiychuk <fol...@gmail.com> > wrote: > > >>>You should rely on time I tests as little as possible. Some of the > test > > >>> hosts are VERY slow. If possible something deterministic should be > used. > > >> > > >> That's why this changes difficult to verify. Maybe in that case we > should > > >> write benchmark but not integration test? > > >> In that case we can say, before this changes stopping logical > replication > > >> gets N ms but after apply changes it gets NN ms where NN ms less than > N ms. > > >> Is it available add this kind of benchmark to postgresql? I will be > grateful > > >> for links. > > > > > > It's for that reason that I added a message printed only in verbose > > > mode that pg_recvlogical emits when it's exiting after a > > > client-initiated copydone. > > > > > > You can use the TAP tests, print diag messages, etc. But we generally > > > want them to run fairly quickly, so big benchmark runs aren't > > > desirable. You'll notice that I left diag messages in to report the > > > timing for the results in your tests, I just changed the tests so they > > > didn't depend on very tight timing for success/failure anymore. > > > > > > We don't currently have any automated benchmarking infrastructure. > > > > Which seems like this patch is not complete yet. > > Personally I think it is. I'm just explaining why I adjusted Vladimir's > tests to be less timing sensitive and rely on a qualitative property > instead. > > That said, it's had recent change and it isn't a big intrusive change that > really need attention this cf. > > > I am marking it as > > returned with feedback, but it may be a better idea to move it to next > > CF once a new version with updated tests shows up. > > I'll move it now. I think the tests are fine, if Vladimir agrees, so IMO > it's ready to go. More eyes wouldn't hurt though. > > If Vladimir wants benchmarking based tests that's a whole separate project > IMO. Something that will work robustly on the weird slow machines we have > isn't simple. Probably a new buildfarm option etc since we won't want to > clutter the really slow old niche boxes with it. > > Vladimir, can I have your opinion on the latest patch or if you want to > make changes, an updated patch? > > >
