Re: Logical decoding fast-forward and slot advance

On 08/01/18 08:02, Simon Riggs wrote:
> On 31 December 2017 at 10:44, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:
> 
>> Attached is patch which adds ability to do fast-forwarding while
>> decoding. That means wal is consumed as fast as possible and changes are
>> not given to output plugin for sending. The implementation is less
>> invasive than I originally though it would be. Most of it is just
>> additional filter condition in places where we would normally filter out
>> changes because we don't yet have full snapshot.
> 
> Looks good.
>

Thanks.

> The precise definition of "slot advance" or "fast forward" isn't
> documented in the patch. If we advance past everything, why is there
> not just one test in LogicalDecodingProcessRecord() to say if
> (ctx->fast_forward)? Why put it in multiple decoding subroutines?
> 

Because we still need to track transactions (otherwise slot's restart
position will not move forward) and mark transactions which did DDL
changes so that historical snapshots are made. Otherwise if we moved
slot forward and then started real decoding from that position we'd have
wrong view of catalogs.

We'd have to write different version of LogicalDecodingProcessRecord()
and duplicate some of the code in the Decode* functions there which
seems like it would be harder to maintain. I might be inclined to do it
with this approach if the current approach would mean adding new branch
into every Decode* function, but since there is already branch for
filtering actions during initial snapshot build, I think it's better to
just extend that.

> If ctx->fast_forward is set it might throw off other opps, so it would
> be useful to see some Asserts elsewhere to make sure we understand and
> avoid breakage
Hmm, I think the really only places where this can be issue and also can
be checked using Assert are the cb wrappers in logical.c which call the
output plugin (output plugin is not supposed to be called when
fast-forwarding) so I Added assert to each of them.

> In pg_replication_slot_advance() the moveto variable is set to
> PG_GETARG_LSN(1) and then unconditionally overwritten before it is
> used for anything. Why?
> 

Eh, there is missing Min, it should be used for clamping, not done
unconditionally. Fixed and added regression test for this.

Updated version attached.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services