References: <AB5978B2-1772-4FEE-A245-74C91704ECB0@amazon.com>
 <87ftjifoql.fsf@ars-thinkpad>
 <20191024213157.7pm6niybfxgpvmgg@alap3.anarazel.de>
 <87eez1fh48.fsf@ars-thinkpad>
 <CAA4eK1L=MDbmGu5-+BmY7Svc07jr+ZabiH8C_qo3RSc8pgUpDQ@mail.gmail.com>
 <8736bs81sx.fsf@ars-thinkpad>
 <CAA4eK1LdNmrib1jub8b=KvYUrzXW0VT4P3MVPMyiMfMY3K64dA@mail.gmail.com>
 <871rrb942q.fsf@ars-thinkpad>
 <CAA4eK1LYzrZ_+8VhD_N_dsQwjxA9t+AyGKT-Wjnc8S7jCwAcBw@mail.gmail.com>
 <87zhdx76d5.fsf@ars-thinkpad>
 <CAA4eK1Jdh0zab=+D91MkFPbevzyjFCZsMwRsQxJh4F9+m_vCRA@mail.gmail.com>
 <CAA4eK1JnSKkNdgLAHWY+YCE_3Li454So_thyTyNJa_G3hSVscA@mail.gmail.com>
 <8736bjoiax.fsf@ars-thinkpad>
 <CAA4eK1LuOGujY2v0u5bAp1VEKfWebHXBSEP0L-DO88p0MXFioQ@mail.gmail.com>
User-agent: mu4e 1.2.0; emacs 26.0.50
From: Arseny Sher <a.sher@postgrespro.ru>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, "Hsu\, John" <hsuchen@amazon.com>,
 "pgsql-bugs\@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>
Subject: Re: ERROR: subtransaction logged without previous top-level txn
 record
In-reply-to: 
 <CAA4eK1LuOGujY2v0u5bAp1VEKfWebHXBSEP0L-DO88p0MXFioQ@mail.gmail.com>
Date: Mon, 02 Mar 2020 10:41:54 +0300
Message-ID: <87o8tf6w59.fsf@ars-thinkpad>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


Amit Kapila <amit.kapila16@gmail.com> writes:

> On Sun, Feb 9, 2020 at 9:37 PM Arseny Sher <a.sher@postgrespro.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. 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. So
can_use_serialized_snap would be even somewhat confusing.


-- cheers, arseny