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

Hi,

On 2018-10-22 12:03:26 +0900, Michael Paquier wrote:
> (moving to -hackers)
>
> On Sun, Oct 14, 2018 at 10:42:40AM -0700, Andres Freund wrote:
> > I'm unhappy this approach was taken over objections. Without a real
> > warning.
>
> Oops, that was not clear to me. Sorry about that! I did not see you
> objecting again after the last arguments I raised. The end of PREPARE
> TRANSACTION is designed like that since it has been introduced, so
> changing the way the dummy GXACT is inserted before the main one is
> cleared from its XID does not sound wise to me. The current design is
> also present for a couple of reasons, please see this thread:
> https://www.postgresql.org/message-id/13905.1119109281@sss.pgh.pa.us
> This has resulted in e26b0abd.

None of them explains why having "corrupt" WAL that's later fixed up is
a good idea.

> Among the things I thought are:
> - Clearing the XID at the same time the dummy entry is added, which
> actually means to hold on ProcArrayLock longer while doing more at the
> end of prepare. I actually don't think you can do that cleanly without
> endangering the transaction visibility for other backends, and syncrep
> may cause the window to get wider.

> - Changing GetRunningTransactionData so as duplicates are removed at
> this stage. However this also requires to hold ProcArrayLock for
> longer.

That's *MUCH* better than what we have right
now. GetRunningTransactionData() isn't called all that often, for once,
and for another the WAL then is correct.

> > Even leaving the crummyness aside, did you check other users of
> > XLOG_RUNNING_XACTS, e.g. logical decoding?
>
> I actually spent some time checking that, so it is not innocent.

"innocent"?

> SnapBuildWaitSnapshot() waits for transactions to commit or abort based
> on the XIDs in the record. And that's the only place where those XIDs
> are used so it won't matter to wait twice for the same transaction to
> finish. The error callback would be used only once.

Right. We used to use it more (and it'd probably caused problems), but
since 955a684e0401954a58e956535107bc4b7136d952 it should be ok...

Greetings,

Andres Freund