Re: Possible bug in logical replication.

On 2018-Jul-12, Michael Paquier wrote:

> On Wed, Jul 04, 2018 at 10:50:28AM +0900, Michael Paquier wrote:
> > On Tue, Jul 03, 2018 at 01:17:48AM -0400, Alvaro Herrera wrote:
> > > Let me review tomorrow.
> >
> > Of course, please feel free.
>
> Alvaro, are you planning to look at that to close the loop? The latest
> version is here:
> https://postgr.es/m/20180709070200.GC30202@paquier.xyz

In the immortal words of Julian Bream: "yeah, I didn't like any of that".

I started thinking that the "while we could do X, we better not because
Y" new wording in the comment was misleading -- the comment is precisely
to convey that we must NOT do X, so why say "we could"? I reworded that
comment a few times until it made sense. Then I noticed the other
comments were either misplaced or slightly misleading, so I moved them
to their proper places, then reworded them thoroughly.

I also moved some assignments from the declaration section to the code
section, so that I could attach proper comments to each, to improve
clarity of *why* we do those things.

I then noticed that we get a XLogRecord from XLogReadRecord, but then
fail to do anything with it, so I changed the code to use a bool
instead, which I think is clearer.

I think the proposed comment before the LogicalDecodingProcessRecord
call failed to convey the important ideas, so I rewrote that one also.

There is no struct member called confirmed_flush_lsn anywhere.

The tense of some words in CreateDecodingContext was wrong.

I also back-patched two minor changes from Tom's 3cb646264e8c.

BTW I think I'm starting to have a vague idea of logical decoding now.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services