Re: Stopping logical replication protocol

<p dir="ltr"><p dir="ltr">On 26 Sep. 2016 02:16, "Vladimir Gordiychuk" <<a
href="mailto:folyga@gmail.com">folyga@gmail.com</a>>wrote:<br /> ><br /> > >It looks like you didn't import
myupdated patches, so I've rebased your new patches on top of them.<br /> > Yes, i forgot it, sorry. Thanks for you
fixes. <br/> ><br /> > >I did't see you explain why this was removed:<br /> ><br /> > -    /* fast path
*/<br/> > -    /* Try to flush pending output to the client */<br /> > -    if (pq_flush_if_writable() != 0)<br
/>> -        WalSndShutdown();<br /> > -<br /> > -    if (!pq_is_send_pending())<br /> > -       
return;<br/> ><br /> > I remove it, because during decode long transaction code, we always exist from function by
fastpath and not receive messages from client. Now we always include in 'for' loop and executes<br /> > /* Check for
inputfrom the client */ ProcessRepliesIfAny();<p dir="ltr">Makes sense.<br /> I<br /> > >Some of the changes to
pg_recvlogicallook to be copied from<br /> > >receivelog.c, most notably the functions ProcessKeepalive and<br />
>>ProcessXLogData .<br /> ><br /> > During refactoring huge function I also notice It, but decide not
includeadditional changes in already huge patch. I only split method that do everything to few small functions. <p
dir="ltr">Gooddecision. This improves things already and makes future refactoring easier.<p dir="ltr">> >I was
evaluatingthe tests and I don't think they actually demonstrate<br /> > >that the patch works. There's nothing
doneto check that<br /> > >pg_recvlogical exited because of client-initiated CopyDone. While<br /> >
>lookinginto that I found that it also never actually hits<br /> > >ProcessCopyDone(...) because libpq handles
theCopyDone reply from the<br /> > >server its self and treats it as end-of-stream. So the loop in<br /> >
>StreamLogicalLogwill just end and ProcessCopyDone() is dead code.<br /> ><br /> > The main idea was about
faststop logical replication as it available, because if stop replication gets more them 1 seconds it takes more
pain.<pdir="ltr">You should rely on time I tests as little as possible. Some of the test hosts are VERY slow. If
possiblesomething deterministic should be used.<p dir="ltr">> Increase this timeout to 3 or 5 second hide problems
thatthis PR try fix, that's why I think this lines should be restore to state from previous patch.<p dir="ltr">Per
aboveI don't agree.<p dir="ltr">><br /> > ```<br /> >     ok($spendTime <= 5, # allow extra time for slow
machines<br/> >         "pg_recvlogical exited promptly on signal when decoding");<br /> ><br /> >    
ok((time()- $cancelTime) <= 3, # allow extra time for slow machines<br /> >         "pg_recvlogical exited
promptlyon sigint when idle"<br /> >     );<br /> ><br /> > ```<br /> ><br /> > Also I do not understand
whywe do <br /> ><br /> > $proc->pump while $proc->pumpable;<br /> ><br /> > after send signal to
process, whywe can not just wait stop replication slot? <p dir="ltr">Because verbose output can produce a lot of
writes.If we don't pump the buffer pg_recvlogical could block writing on its socket.   Also it lets us capture stderr
whichwe need to observe that pg_recvlogical actually ended copy on user command rather than just receiving all the
inputavailable.