Re: Stopping logical replication protocol

From: Craig Ringer <craig(at)2ndquadrant(dot)com>
To: Vladimir Gordiychuk <folyga(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Álvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Andres Freund <andres(at)anarazel(dot)de>
Subject: Re: Stopping logical replication protocol
Date: 2016-09-09 04:03:04
Message-ID: CAMsr+YGV-37wG-B8Rdv=Vg7icquObKGWud3LpGTrE60HiVmw3Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 9 September 2016 at 10:37, Craig Ringer <craig(at)2ndquadrant(dot)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

Attachment Content-Type Size
0001-Respect-client-initiated-CopyDone-in-walsender.patch text/x-patch 3.4 KB
0002-Respect-client-initiated-CopyDone-during-transaction.patch text/x-patch 11.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2016-09-09 04:25:02 Re: CVE-2016-1238 fix breaks (at least) pg_rewind tests
Previous Message Andrey Borodin 2016-09-09 03:50:53 Re: GiST penalty functions [PoC]