Re: Timeline following for logical slots - Mailing list pgsql-hackers

Craig Ringer wrote:

> -    /* oldest LSN that the client has acked receipt for */
> +    /*
> +     * oldest LSN that the client has acked receipt for
> +     *
> +     * Decoding will start calling output plugin callbacks for commits
> +     * after this LSN unless a different start point is specified by
> +     * the client.
> +     */
>      XLogRecPtr    confirmed_flush;

How about this wording?
/* * Oldest LSN that the client has acked receipt for.  This is used as * the start_lsn point in case the client
doesn'tspecify one, and also * as a safety measure to back off in case the client specifies a * start_lsn that's
furtherin the future than this value. */XLogRecPtr    confirmed_flush;
 

> -    /* 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?  I think the
comment should be worded in a way that's not specific to logical
decoding; or, if the restart_lsn field *is* specific to logical
decoding, then the comment should state as much.


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

> @@ -262,11 +264,9 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
>          ctx->output_writer_private = p;
>  
>          /*
> -         * We start reading xlog from the restart lsn, even though we won't
> -         * start returning decoded data to the user until the point set up in
> -         * CreateDecodingContext. The restart_lsn is far enough back that we'll
> -         * see the beginning of any xact we're expected to return to the client
> -         * so we can assemble a complete reorder buffer for it.
> +         * Reading and decoding of WAL must start at restart_lsn so that the
> +         * entirety of each of the xacts that commit after confimed_lsn can be
> +         * accumulated into reorder buffers.
>           */
>          startptr = MyReplicationSlot->data.restart_lsn;

This one looks fine to me, except typo s/confimed/confirmed/

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