From 2d2e40e3befd8b9e0d2757554537345b15fa6ea2 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 24 May 2016 15:47:51 -0400 Subject: [PATCH] Fetch XIDs atomically during vac_truncate_clog(). Because vac_update_datfrozenxid() updates datfrozenxid and datminmxid in-place, it's unsafe to assume that successive reads of those values will give consistent results. Fetch each one just once to ensure sane behavior in the minimum calculation. Noted while reviewing Alexander Korotkov's patch in the same area. Discussion: <8564.1464116473@sss.pgh.pa.us> --- src/backend/commands/vacuum.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 9cda925032..25a55ab0a5 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -1067,6 +1067,12 @@ vac_truncate_clog(TransactionId frozenXID, /* * Scan pg_database to compute the minimum datfrozenxid/datminmxid * + * Since vac_update_datfrozenxid updates datfrozenxid/datminmxid in-place, + * the values could change while we look at them. Fetch each one just + * once to ensure sane behavior of the comparison logic. (Here, as in + * many other places, we assume that fetching or updating an XID in shared + * storage is atomic.) + * * Note: we need not worry about a race condition with new entries being * inserted by CREATE DATABASE. Any such entry will have a copy of some * existing DB's datfrozenxid, and that source DB cannot be ours because @@ -1082,10 +1088,12 @@ vac_truncate_clog(TransactionId frozenXID, while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) { - Form_pg_database dbform = (Form_pg_database) GETSTRUCT(tuple); + volatile FormData_pg_database *dbform = (Form_pg_database) GETSTRUCT(tuple); + TransactionId datfrozenxid = dbform->datfrozenxid; + TransactionId datminmxid = dbform->datminmxid; - Assert(TransactionIdIsNormal(dbform->datfrozenxid)); - Assert(MultiXactIdIsValid(dbform->datminmxid)); + Assert(TransactionIdIsNormal(datfrozenxid)); + Assert(MultiXactIdIsValid(datminmxid)); /* * If things are working properly, no database should have a @@ -1096,21 +1104,21 @@ vac_truncate_clog(TransactionId frozenXID, * databases have been scanned and cleaned up. (We will issue the * "already wrapped" warning if appropriate, though.) */ - if (TransactionIdPrecedes(lastSaneFrozenXid, dbform->datfrozenxid) || - MultiXactIdPrecedes(lastSaneMinMulti, dbform->datminmxid)) + if (TransactionIdPrecedes(lastSaneFrozenXid, datfrozenxid) || + MultiXactIdPrecedes(lastSaneMinMulti, datminmxid)) bogus = true; - if (TransactionIdPrecedes(nextXID, dbform->datfrozenxid)) + if (TransactionIdPrecedes(nextXID, datfrozenxid)) frozenAlreadyWrapped = true; - else if (TransactionIdPrecedes(dbform->datfrozenxid, frozenXID)) + else if (TransactionIdPrecedes(datfrozenxid, frozenXID)) { - frozenXID = dbform->datfrozenxid; + frozenXID = datfrozenxid; oldestxid_datoid = HeapTupleGetOid(tuple); } - if (MultiXactIdPrecedes(dbform->datminmxid, minMulti)) + if (MultiXactIdPrecedes(datminmxid, minMulti)) { - minMulti = dbform->datminmxid; + minMulti = datminmxid; minmulti_datoid = HeapTupleGetOid(tuple); } } -- 2.40.0