Re: out-of-order XID insertion in KnownAssignedXids

On 11.10.2018 12:06, Michael Paquier wrote:
> 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

Proposed patch is attached.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company