Re: Possible bug in logical replication.

On Wed, Jul 18, 2018 at 02:30:53PM -0400, Alvaro Herrera wrote:
> In the immortal words of Julian Bream: "yeah, I didn't like any of
> that".

One wikipedia lookup later, I still don't know where this quote comes
from, but at least I understand who the man is.

I may be missing something, but I cannot apply your patch on HEAD so I
have not tested it.  Anyway, I read through it and the thing does not
look logically wrong.

> 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.

To be pedantic here, you could move the declarations of startlsn,
old_resowner and ctx directly inside the PG_TRY block.

> 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.

Matter of taste perhaps, I was fine with just manipulating the record
pointer.

> 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.

This is referring to the system catalog field in pg_replication_slots.

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

Nice.

>  	PG_TRY();
>  	{
> -		/* restart at slot's confirmed_flush */
> +		/*
> +		 * Create our decoding context in fast_forward mode, passing start_lsn
> +		 * as Invalid, so that we start processing from confirmed_flush.
> +		 */

I'd rather mention InvalidXLogRecPtr directly here.  Invalid alone makes
no real sense.

> +      gotrecord = XLogReadRecord(ctx->reader, startlsn, &errm) != NULL;

I would put parenthesis for clarity.
--
Michael