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