Re: Timeline following for logical slots

On 2016-04-26 17:22:49 -0300, Alvaro Herrera wrote:
> > - /* oldest LSN that might be required by this replication slot */
> > + /*
> > + * oldest LSN that might be required by this replication slot.
> > + *
> > + * Points to the LSN of the most recent xl_running_xacts record where
> > + * no transaction that commits after confirmed_flush is already in
> > + * progress. At this point all xacts committing after confirmed_flush
> > + * can be read entirely into reorder buffers and all visibility
> > + * information can be reconstructed.
> > + */
> > XLogRecPtr restart_lsn;
>
> I'm unsure about this one. Why are we speaking of reorder buffers here,
> if this is generically about replication slots? Is it that slots used
> by physical replication do not *need* a restart_lsn?

It's used in physical slots as well, so I agree. Also I think the
xl_running_xacts bit is going into way to much detail; it could very
well just be shutdown checkpoint or other similar locations.

> > diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
> > index 5af6751..5f74941 100644
> > --- a/src/backend/replication/logical/logicalfuncs.c
> > +++ b/src/backend/replication/logical/logicalfuncs.c
> > @@ -236,8 +236,10 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
> > PG_TRY();
> > {
> > /*
> > - * Passing InvalidXLogRecPtr here causes decoding to start returning
> > - * commited xacts to the client at the slot's confirmed_flush.
> > + * Passing InvalidXLogRecPtr here causes logical decoding to
> > + * start calling the output plugin for transactions that commit
> > + * at or after the slot's confirmed_flush. This filters commits
> > + * out from the client but they're still decoded.
> > */
> > ctx = CreateDecodingContext(InvalidXLogRecPtr,
> > options,
>
> I this it's weird to have the parameter explained in the callsite rather
> than in the comment about CreateDecodingContext. I think this patch
> needs to apply to logical.c, not logicalfuncs.

I still think the entire comment ought to be removed.

- Andres