Re: logical copy_replication_slot issues

On Mon, 9 Mar 2020 at 21:46, Arseny Sher <a.sher@postgrespro.ru> wrote:
>
>
> Masahiko Sawada <masahiko.sawada@2ndquadrant.com> writes:
>
> >     /*
> > -    * Create logical decoding context, to build the initial snapshot.
> > +    * Create logical decoding context to find start point or, if we don't
> > +    * need it, to 1) bump slot's restart_lsn and xmin 2) check plugin sanity.
> >      */
> >
> > Do we need to numbering that despite not referring them?
>
> No, it just seemed clearer to me this way. I don't mind removing the
> numbers if you feel this is better.
>

Okay.

> >     ctx = CreateInitDecodingContext(plugin, NIL,
> > -                                   false,  /* do not build snapshot */
> > +                                   false,  /* do not build data snapshot */
> >                                     restart_lsn,
> >                                     logical_read_local_xlog_page, NULL, NULL,
> >                                     NULL);
> > I'm not sure this change makes the comment better. Could you elaborate
> > on the motivation of this change?
>
> Well, DecodingContextFindStartpoint always builds a snapshot allowing
> historical *catalog* lookups. This bool controls whether the snapshot
> should additionally be suitable for looking at the actual data, this is
> e.g. used by initial data sync in the native logical replication.

Okay.

Anyway, since the patch looks good to me I've marked this patch as
"Ready for Committer". I think we can defer these things to
committers.

Regards,

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