On Mon, Mar 2, 2020 at 7:07 PM Arseny Sher <a.sher@postgrespro.ru> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
>
> > I think here you are trying to deduce the meaning. I don't see that
> > it can clearly define that don't use serialized snapshots. It is not
> > clear to me why have you changed the below code, basically why it is
> > okay to pass InvalidXLogRecPtr instead of restart_lsn?
> >
> > @@ -327,7 +327,7 @@ CreateInitDecodingContext(char *plugin,
> > ReplicationSlotMarkDirty();
> > ReplicationSlotSave();
> >
> > - ctx = StartupDecodingContext(NIL, restart_lsn, xmin_horizon,
> > + ctx = StartupDecodingContext(NIL, InvalidXLogRecPtr, xmin_horizon,
> > need_full_snapshot, false,
> > read_page, prepare_write, do_write,
> > update_progress);
>
> Because when we create the slot we don't demand to stream from some
> specific point. In fact we just can't, because we don't know since which
> LSN it is actually possible to stream, i.e. when we'd have good snapshot
> and no old (which we haven't seen in full) xacts running. It is up to
> snapbuild.c to define this point. The previous coding was meaningless:
> we asked for some random restart_lsn and snapbuild.c would silently
> advance it to earliest suitable LSN.
>
Hmm, if this is the case then it should be true even without solving
this particular problem and we should be able to make this change.
Leaving that aside, I think this change can make copy replication slot
functionality to also skip using serialized snapshots with this patch
which is not our intention. Also, it doesn't seem like a good idea to
ignore setting start_decoding_at when we already set
slot->data.restart_lsn with this value.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com