Re: Stopping logical replication protocol

On 9 September 2016 at 10:37, Craig Ringer <craig@2ndquadrant.com> wrote:

> I'm looking at the revised patch now.

Considerably improved.


I fixed a typo from decondig to decoding that's throughout all the
callback names.


This test is wrong:

+        while ((change = ReorderBufferIterTXNNext(rb, iterstate)) !=
NULL && rb->continue_decoding_cb != NULL &&
rb->continue_decoding_cb())

If continue_decoding_cb is non-null, it'll never be called since the
and will short-circuit. If it's null the and will be false so we'll
call rb->continue_decoding_cb().

It also violates PostgreSQL source formatting coding conventions; it
should be wrapped to 80 lines. (Yes, that's archaic, but it's kind of
useful when you've got multiple files open side-by-side, and at least
it's consistent across the codebase).

so it should be:

        while ((change = ReorderBufferIterTXNNext(rb, iterstate)) != NULL &&
               (rb->continue_decoding_cb == NULL ||
                rb->continue_decoding_cb()))

I'd prefer the continue_decoding_cb tests to be on the same line, but
it's slightly too wide. Eh, whatever.


Same logic issue later;

-         if(rb->continue_decoding_cb != NULL && rb->continue_decoding_cb())
+        if (rb->continue_decoding_cb == NULL || rb->continue_decoding_cb())


Commit message is slightly inaccurate. The walsender DID look for
client messages during ReorderBufferCommit decoding, but it never
reacted to CopyDone sent by a client when it saw it.


Rewrote comment on IsStreamingActive() per my original review advice.



I'm not sure why exactly you removed

-    /* fast path */
-    /* Try to flush pending output to the client */
-    if (pq_flush_if_writable() != 0)
-        WalSndShutdown();
-
-    if (!pq_is_send_pending())
-        return;
-

It should be OK if we flush pending output even after we receive
CopyDone. In fact, we have to, since we can't unqueue it and have to
send it before we can send our own CopyDone reply.
pq_flush_if_writable() should only return EOF if the socket is closed,
in which case fast bailout is the right thing to do.

Can you explain your thinking here and what the intended outcome is?



I've attached updated patches with a number of typo fixes,
copy-editing changes, a fix to the test logic, etc as noted above.

Setting "waiting on author" in CF per discussion of the need for tests.


--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services