Re: pgsql: Avoid duplicate XIDs at recovery when building initial snapshot

On 2018-Oct-22, Andres Freund wrote:

> Hi,
>
> On 2018-10-22 12:36:25 -0300, Alvaro Herrera wrote:
> > On 2018-Oct-14, Andres Freund wrote:
> >
> > > On 2018-10-14 13:26:24 +0000, Michael Paquier wrote:
> > > > Avoid duplicate XIDs at recovery when building initial snapshot
> >
> > > I'm unhappy this approach was taken over objections. Without a real
> > > warning. Even leaving the crummyness aside, did you check other users
> > > of XLOG_RUNNING_XACTS, e.g. logical decoding?
> >
> > Mumble. Is there a real problem here -- I mean, did this break logical
> > decoding? Maybe we need some more tests to ensure this stuff works
> > sanely for logical decoding.
>
> Hm? My point is that this fix just puts a band-aid onto *one* of the
> places that read a XLOG_RUNNING_XACTS. Which still leaves the contents
> of WAL record corrupted. There's not even a note at the WAL-record's
> definition or its logging denoting that the contents are not what you'd
> expect. I don't mean that the fix would break logical decoding, but
> that it's possible that an equivalent of the problem affecting hot
> standby also affects logical decoding. And even leaving those two users
> aside, it's possible that there will be further vulernable internal
> users or extensions parsing the WAL.

Ah! I misinterpreted what you were saying. I agree we shouldn't let
the WAL message have wrong data. Of course we shouldn't just add a
code comment stating that the data is wrong :-)

> > FWIW and not directly related, I recently became aware (because of a
> > customer question) that txid_current_snapshot() is oblivious of
> > XLOG_RUNNING_XACTS in a standby. AFAICS it's not a really serious
> > concern, but it was surprising.
>
> That's more fundamental than just XLOG_RUNNING_XACTS though, no? The
> whole way running transactions (i.e. including those that are just
> detected by looking at their xid) are handled in the known xids struct
> and in snapshots seems incompatible with that, no?

hmm ... as I recall, txid_current_snapshot also only considers xacts by
xid, so read-only xacts are not considered -- seems to me that if you
think of snapshots in a general way, you're right, but for whatever you
want txid_current_snapshot for (and I don't quite remember what that is)
then it's not really important.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services