From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: out-of-order XID insertion in KnownAssignedXids
Date: Thu, 11 Oct 2018 20:04:11 +0300
Message-ID: <54e55450-0c87-9dd4-c773-8d2f2fb6054c@postgrespro.ru> (raw)
In-Reply-To: <20181011090611.GC23570@paquier.xyz>
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>
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
Attachments:runnings_xids-2.patch (text/x-patch, 2.5K) downloaddiff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.cindex 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.cindex 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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: k.knizhnik@postgrespro.ru, michael@paquier.xyz, andres@anarazel.de
Subject: Re: out-of-order XID insertion in KnownAssignedXids
In-Reply-To: <54e55450-0c87-9dd4-c773-8d2f2fb6054c@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox