Re: Stopping logical replication protocol

> 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 <craig(at)2ndquadrant(dot)com>:

> On 3 Oct. 2016 10:15, "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com> wrote:
> >
> > On Tue, Sep 27, 2016 at 11:05 AM, Craig Ringer <craig(at)2ndquadrant(dot)com>
> wrote:
> > > On 26 September 2016 at 21:52, Vladimir Gordiychuk <folyga(at)gmail(dot)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?
>
>
>