On Mon, 9 Mar 2020 at 21:46, Arseny Sher <a.s...@postgrespro.ru> wrote: > > > Masahiko Sawada <masahiko.saw...@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
