On Wed, Oct 10, 2018 at 11:22:45AM +0900, Michael Paquier wrote:
> I am not sure if the performance argument is actually this much
> sensible, it could be as you say, but another thing that we could argue
> about is that the presence of duplicate entries in
> GetRunningTransactionData() can be used as a point to understand that
> 2PC transactions are running, and that among the two, one of them is a
> dummy entry while the other is pending for being cleared.
Actually there would be a performance impact for any deployments if we
were to do so, as ProcArrayLock is hold and we would need to scan 4
times procArray instead of twice. Many people already complain (justly)
that ProcArray is a performance bottleneck, it would be a bad idea to
make things worse...
> 1) Document that GetRunningTransactionData() fetches information also
> about 2PC entries, like that:
> * GetRunningTransactionData -- returns information about running transactions.
> *
> * Similar to GetSnapshotData but returns more information. We include
> - * all PGXACTs with an assigned TransactionId, even VACUUM processes.
> + * all PGXACTs with an assigned TransactionId, even VACUUM processes and
> + * dummy two-phase related entries created when preparing the transaction.
>
> 2) Update LogStandbySnapshot so as the list of XIDs fetched is sorted
> and ordered. This can be used as a sanity check for recovery via
> ProcArrayApplyRecoveryInfo().
This also would increase the pressure on ProcArrayLock with wal_level =
logical as the WAL record needs to be included while holding the lock.
So let's do as Konstantin is suggesting by skipping duplicated XIDs
after applying the qsort(). The current code is also doing a bad
documentation job, so we should mentioned that GetRunningTransactionData
may return duplicated XIDs because of the dummy 2PC entries which
overlap with the active ones, and also add a proper comment in
ProcArrayApplyRecoveryInfo(). Konstantin, do you want to give it a try
with a patch? Or should I?
--
Michael