Re: ERROR: subtransaction logged without previous top-level txn record

On Mon, Mar 2, 2020 at 1:11 PM Arseny Sher <a(dot)sher(at)postgrespro(dot)ru> wrote:
>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>
> > On Sun, Feb 9, 2020 at 9:37 PM Arseny Sher <a(dot)sher(at)postgrespro(dot)ru> wrote:
> > + /*
> > + * Don't use serialized snapshot if we are not sure where all
> > + * currently running xacts will finish (new slot creation).
> > + * (Actually, if we came here through xl_running_xacts, we could perform
> > + * SNAPBUILD_FULL_SNAPSHOT -> SNAPBUILD_CONSISTENT transition properly,
> > + * but added lines of code would hardly worth the benefit.)
> > + */
> > + if (builder->start_decoding_at == InvalidXLogRecPtr)
> > + return false;
> >
> > Instead of using start_decoding_at to decide whether to restore
> > snapshot or not, won't it be better to have new variable in SnapBuild
> > (say can_use_serialized_snap or something like that) and for this
> > purpose?
>
> start_decoding_at who is initialized externally at
> AllocateSnapshotBuilder is what actually defines how to handle
> serialized snapshots: if it is valid LSN, snapbuild must trust the
> caller that WAL reading starts early enough to stream since this LSN, so
> we deserialize the snap and jump into CONSISTENT. If it is invalid, we
> don't know the safe streaming point yet, and it remains invalid until we
> learn full snapshot and then wait for all xacts finishing.
>

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);

> So such bool
> would be a pointless synonym.
>
> Moreover, as cited comment mentions:
>
> > + * (Actually, if we came here through xl_running_xacts, we could perform
> > + * SNAPBUILD_FULL_SNAPSHOT -> SNAPBUILD_CONSISTENT transition properly,
> > + * but added lines of code would hardly worth the benefit.)
>
> there is nothing wrong in using the serialized snapshot per se. It's
> just that we must wait for all xacts finishing after getting the
> snapshot and this is impossible if we don't know who is running.
>

I am not denying any such possibility and even if we do something like
that it will be for the master branch.

> So
> can_use_serialized_snap would be even somewhat confusing.
>

It is possible that your idea of trying to deduce from
start_decoding_at is better, but I am not sure about it if anyone else
also thinks that is a good way to do, then fine.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com