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>
 <87o8tf6w59.fsf@ars-thinkpad>
 <CAA4eK1JjGtRhgP6ShDrxQ+yU=nEQ5U+OG3Y4ZYME117qgK23AQ@mail.gmail.com>
 <87mu8y7u9r.fsf@ars-thinkpad>
 <CAA4eK1KijcZSP0MMkNd8y2ycyZtxgFNt69YhC4UQj6+cB8EHcA@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: 
 <CAA4eK1KijcZSP0MMkNd8y2ycyZtxgFNt69YhC4UQj6+cB8EHcA@mail.gmail.com>
Date: Wed, 04 Mar 2020 16:29:44 +0300
Message-ID: <87lfog6yev.fsf@ars-thinkpad>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


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

>> 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.

Right.

> 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.

As I say at [1] logical slot copying facility is currently anyway broken
in this regard: restart_lsn is copied, but confirmed_flush isn't, and
the right fix, in my view, is to avoid DecodingContextFindStartpoint
there altogether (by checking donor's confirmed_flush is valid and
copying it) which would render this irrelevant. To speculate, even if
wanted to go through DecodingContextFindStartpoint for slot copying and
establish confirmed_flush on our own, surely we'd need to handle
serialized snapshots exactly as new slot creation does because dangers
of getting SNAPBUILD_CONSISTENT too early are the same in both cases.

> 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.

Well, these two fields have absolutely different values. BTW I find the
naming here somewhat unfortunate, and this phrase suggests that it
indeed leads to confusion.

Slot's restart_lsn is the LSN since which we start reading WAL and by
setting data.restart_lsn we prevent WAL we need from
recycling. start_decoding_at is the LSN since which we start
*streaming*, i.e. actually replaying COMMITs. So setting the first one
(as we must hold WAL) and not the second one (as we don't know the
streaming point yet when we start slot creation) is just fine.


[1] https://www.postgresql.org/message-id/flat/CA%2Bfd4k70BXLTm-N6q18LrL%3DGbKtwY3-2%2B%2BUVFw05SvFTkZgTyQ%40mail.gmail.com


-- cheers, arseny