Date: Fri, 12 Oct 2018 11:49:01 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@lists.postgresql.org,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: out-of-order XID insertion in KnownAssignedXids
Message-ID: <20181012024901.GC30064@paquier.xyz>
References: <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>
 <54e55450-0c87-9dd4-c773-8d2f2fb6054c@postgrespro.ru>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="cPi+lWm09sJ+d57q"
Content-Disposition: inline
In-Reply-To: <54e55450-0c87-9dd4-c773-8d2f2fb6054c@postgrespro.ru>
Precedence: bulk


--cPi+lWm09sJ+d57q
Content-Type: multipart/mixed; boundary="NklN7DEeGtkPCoo3"
Content-Disposition: inline


--NklN7DEeGtkPCoo3
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Oct 11, 2018 at 08:04:11PM +0300, Konstantin Knizhnik wrote:
> Proposed patch is attached.

The problem I have with this patch doing the duplication removal and
qsort work in LogCurrentRunningXacts is that it would still lock
ProcArrayLock until the WAL record has been written with wal_level =
logical.  With many sessions and 2PC transactions running around, this
would be a performance impact for any deployments every time a
checkpoint happens or every time the bgwriter decide to log a standby
snapshot.  Hence I would go instead with the attached, which does the
legwork at recovery, which is a one-time code path as you mentioned.
Okay, this makes the recovery a bit longer but that's way better than
impacting all deployments of Postgres, even those not using 2PC when
normally running.  And as the sorting phase already happens we just need
to do something like the attached.

One thing that we could also do for HEAD is to add in
RunningTransactionsData if the transaction comes from a 2PC entry, and
allow recovery to do more sanity checks.  This would require a WAL
format change.  The proposed patch needs to be back-patched down to
9.3.
--
Michael

--NklN7DEeGtkPCoo3
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="duplicated-xids-recovery.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/=
procarray.c
index bf2f4dbed2..6317b856a1 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -799,10 +799,20 @@ ProcArrayApplyRecoveryInfo(RunningTransactions runnin=
g)
 		qsort(xids, nxids, sizeof(TransactionId), xidComparator);
=20
 		/*
-		 * Add the sorted snapshot into KnownAssignedXids
+		 * Add the sorted snapshot into KnownAssignedXids.  The running-xacts
+		 * snapshot may include duplicated xids because of prepared
+		 * transactions, so ignore them.
 		 */
 		for (i =3D 0; i < nxids; i++)
+		{
+			if (i > 0 && TransactionIdEquals(xids[i - 1], xids[i]))
+			{
+				elog(DEBUG1, "found duplicated transaction %u from prepared transactio=
n for KnownAssignedXids",
+					 xids[i]);
+				continue;
+			}
 			KnownAssignedXidsAdd(xids[i], xids[i], true);
+		}
=20
 		KnownAssignedXidsDisplay(trace_recovery(DEBUG3));
 	}
@@ -1898,7 +1908,8 @@ ProcArrayInstallRestoredXmin(TransactionId xmin, PGPR=
OC *proc)
  * GetRunningTransactionData -- returns information about running transact=
ions.
  *
  * 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
+ * prepared transactions.
  *
  * We acquire XidGenLock and ProcArrayLock, but the caller is responsible =
for
  * releasing them. Acquiring XidGenLock ensures that no new XIDs enter the=
 proc
@@ -1912,6 +1923,11 @@ ProcArrayInstallRestoredXmin(TransactionId xmin, PGP=
ROC *proc)
  * This is never executed during recovery so there is no need to look at
  * KnownAssignedXids.
  *
+ * Dummy PGXACTs from prepared transaction are included, meaning that this
+ * may return entries with duplicated TransactionId values coming from
+ * transaction finishing to prepare.  Nothing is done about duplicated
+ * entries here to not hold on ProcArrayLock more than necessary.
+ *
  * We don't worry about updating other counters, we want to keep this as
  * simple as possible and leave GetSnapshotData() as the primary code for
  * that bookkeeping.

--NklN7DEeGtkPCoo3--

--cPi+lWm09sJ+d57q
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlvAC50ACgkQnvQgOdby
QH3OHw//Z7eXuXg7RSIYrpa1jPN32ZVyFdnqLimpvuXUpZIH6N//+66oDicCJ2+k
t75Ly9NSwXACXLAbeOd05izjiDmS9zh+Qxeo2o89fpKpHOYLd13bOFwy1QAUUSnx
8g30lMtkD+C8dFBfvRz61/FfulGLTGGT2PRk8tXeR5ogjNzw4dod3YPKJeFPS+XS
9Ors96rAvSNNncrKz+iqipFMha1ye4hnragyOX9lO772Wz+fLSliLE1BP0fGe37+
7LP8TbUzhl4FWWMx3HwRC29NtpBuOxC/NUIn25Y05cseus+6Z8qILGCINROAjqB6
8b7I63bN1jqkdDVX5ahUnL3MpXGGp9Xm2L/HbyzzCXhuO417dNwgl66GYhes525j
wnMGYZdW3gz9X6MRAXRe3X02qwEXw/jD8RikxsSGGOV90JqIb2jCGRFxV+N26zW3
fl4EoH7Lj1bp/I2VLdTPHXHv9/efd0zQ/RO6kxrd+LhUN7gOE07BP0eAGvogKjHc
pbWtYLKRrI1IbcRMb4QSVadmD5xiGc89AHuW8f9nwmtE472Hjp3aR2vkwUO9BYdr
LuPFLDY7xDC9mloi5AaV0IywHGJeVcMRq1siWJvzCa7MH/Oeiw34m2YflEpyO8VZ
ZLOZjm+HawYlFg/GLJoWfWjzZJvY4Emb88HoaBS38H2JFw1MFWs=
=hQK+
-----END PGP SIGNATURE-----

--cPi+lWm09sJ+d57q--