Subject: Re: out-of-order XID insertion in KnownAssignedXids
To: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: <0c96b653-4696-d4b4-6b5d-78143175d113@postgrespro.ru>
 <20181005080422.GA7863@paquier.xyz>
 <581a80f7-3992-f319-302d-7c9a384d64f7@postgrespro.ru>
 <2A819787-9E67-41EF-B89A-A906996924E5@anarazel.de>
 <fc51532c-dbf5-dce0-b31c-82c7a0b837ed@postgrespro.ru>
 <20181008163049.nvaui5kjrsav2ojn@alap3.anarazel.de>
 <20181009055900.GH2137@paquier.xyz> <20181009075258.GI2137@paquier.xyz>
 <1e1d56e2-b056-5fa1-bc89-a6bb3836a1c8@postgrespro.ru>
 <20181010022245.GB2204@paquier.xyz> <20181011090611.GC23570@paquier.xyz>
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Message-ID: <54e55450-0c87-9dd4-c773-8d2f2fb6054c@postgrespro.ru>
Date: Thu, 11 Oct 2018 20:04:11 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20181011090611.GC23570@paquier.xyz>
Content-Type: multipart/mixed;
 boundary="------------1B1F07603BC073C1A89F91D4"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------1B1F07603BC073C1A89F91D4
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit


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


--------------1B1F07603BC073C1A89F91D4
Content-Type: text/x-patch;
 name="runnings_xids-2.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="runnings_xids-2.patch"

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)

--------------1B1F07603BC073C1A89F91D4--