Re: Timeline following for logical slots

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't specify one, and also
* as a safety measure to back off in case the client specifies a
* start_lsn that's further in 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