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
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index bf2f4db..bdf1f65 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -793,13 +793,8 @@ ProcArrayApplyRecoveryInfo(RunningTransactions running)
}
/*
- * Sort the array so that we can add them safely into
- * KnownAssignedXids.
- */
- qsort(xids, nxids, sizeof(TransactionId), xidComparator);
-
- /*
- * Add the sorted snapshot into KnownAssignedXids
+ * Add the sorted snapshot into KnownAssignedXids,
+ * xids are already sorted by LogCurrentRunningXacts
*/
for (i = 0; i < nxids; i++)
KnownAssignedXidsAdd(xids[i], xids[i], true);
@@ -1898,7 +1893,8 @@ ProcArrayInstallRestoredXmin(TransactionId xmin, PGPROC *proc)
* 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.
*
* We acquire XidGenLock and ProcArrayLock, but the caller is responsible for
* releasing them. Acquiring XidGenLock ensures that no new XIDs enter the proc
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index c9bb3e9..57b0e26 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -29,6 +29,7 @@
#include "storage/procarray.h"
#include "storage/sinvaladt.h"
#include "storage/standby.h"
+#include "utils/builtins.h"
#include "utils/hsearch.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
@@ -977,9 +978,20 @@ LogCurrentRunningXacts(RunningTransactions CurrRunningXacts)
/* array of TransactionIds */
if (xlrec.xcnt > 0)
- XLogRegisterData((char *) CurrRunningXacts->xids,
- (xlrec.xcnt + xlrec.subxcnt) * sizeof(TransactionId));
+ {
+ int i, j, nxids = xlrec.xcnt + xlrec.subxcnt;
+ TransactionId* xids = CurrRunningXacts->xids;
+
+ /* Sort XIDs array and exclude duplicated XIDs caused by two-phase related entries */
+ qsort(xids, nxids, sizeof(TransactionId), xidComparator);
+ for (i=0, j=1; j < nxids; j++)
+ if (xids[i] != xids[j])
+ xids[++i] = xids[j];
+
+ nxids = i + 1;
+ XLogRegisterData((char *) xids, nxids * sizeof(TransactionId));
+ }
recptr = XLogInsert(RM_STANDBY_ID, XLOG_RUNNING_XACTS);
if (CurrRunningXacts->subxid_overflow)
