Re: Possible bug in logical replication.

On Fri, Jun 15, 2018 at 06:27:56PM +0300, Arseny Sher wrote:
> I confirm that starting reading WAL since restart_lsn as implemented in
> f731cfa fixes this issue, as well as the second issue tushar mentioned
> at [1].

Thanks!

+ /*
+ * Start reading WAL at restart_lsn, which certainly points to the valid
+ * record.
+ */
XLogRecPtr startlsn = MyReplicationSlot->data.restart_lsn;
XLogRecPtr retlsn = MyReplicationSlot->data.confirmed_flush;

What does this one actually bring?

PG_TRY();
{
- /* restart at slot's confirmed_flush */
+ /* start_lsn doesn't matter here, we don't replay xacts at all */
ctx = CreateDecodingContext(InvalidXLogRecPtr,
NIL,
true,

Okay for this one.

- /*
- * The {begin_txn,change,commit_txn}_wrapper callbacks above will
- * store the description into our tuplestore.
- */
+ /* Changes are not actually produced in fast_forward mode. */

This one is a good idea. Now CreateDecodingContext is missing the
description of what fast_forward actually does, aka no changes are
produced. Could you update your patch to reflect that? That would be
useful for future callers of CreateDecodingContext as well.

- /* Stop once the moving point wanted by caller has been reached */
- if (moveto <= ctx->reader->EndRecPtr)
- break;
-
CHECK_FOR_INTERRUPTS();

It seems to me that we still want to have the slot forwarding finish in
this case even if this is interrupted. Petr, isn't that the intention
here?
--
Michael