Re: Reviving the "Stopping logical replication protocol" patch from Vladimir Gordichuk

Dave Cramer

On Sun, 13 Jan 2019 at 23:19, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> On Mon, 3 Dec 2018 at 19:38, Dave Cramer <davecramer(at)gmail(dot)com> wrote:
>
>> Dmitry,
>>
>> Please see attached rebased patches
>>
>
> I'm fine with patch 0001, though I find this comment a bit hard to follow:
>
> + * The send_data callback must enqueue complete CopyData messages to libpq
> + * using pq_putmessage_noblock or similar, since the walsender loop may
> send
> + * CopyDone then exit and return to command mode in response to a client
> + * CopyDone between calls to send_data.
>
>
I think it needs splitting up into a couple of sentences.
>
> Fair point, remember it was originally written by a non-english speaker

>
> In patch 0002, stopping during a txn. It's important that once we skip any
> action, we continue skipping. In patch 0002 I'd like it to be clearer that
> we will *always* skip the rb->commit callback if rb->continue_decoding_cb()
> returned false and aborted the while loop. I suggest storing the result of
> (rb->continue_decoding_cb == NULL || rb->continue_decoding_cb()) in an
> assignment within the while loop, and testing that result later.
>
> e.g.
>
> (continue_decoding = (rb->continue_decoding_cb == NULL ||
> rb->continue_decoding_cb()))
>
> and later
>
> if (continue_decoding) {
> rb->commit(rb, txn, commit_lsn);
> }
>
> Will do

> I don't actually think it's necessary to re-test the continue_decoding
> callback and skip commit here. If we've sent all the of the txn
> except the commit, we might as well send the commit, it's tiny and might
> save some hassle later.
>
>
> I think a comment on the skipped commit would be good too:
>
> /*
> * If we skipped any changes due to a client CopyDone we must not send a
> commit
> * otherwise the client would incorrectly think it received a complete
> transaction.
> */
>
> I notice that the fast-path logic in WalSndWriteData is removed by this
> patch. It isn't moved; there's no comparison of last_reply_timestamp
> and wal_sender_timeout now. What's the rationale there? You want to ensure
> that we reach ProcessRepliesIfAny() ? Can we cheaply test for a readable
> client socket then still take the fast-path if it's not readable?
>

This may have been a mistake as I am taking this over from a very old patch
that I did not originally write. I'll have a look at this

I

>
> --
> Craig Ringer http://www.2ndQuadrant.com/
> 2ndQuadrant - PostgreSQL Solutions for the Enterprise
>