Prevent concurrent SimpleLruTruncate() for any given SLRU.

The SimpleLruTruncate() header comment states the new coding rule.  To
achieve this, add locktype "frozenid" and two LWLocks.  This closes a
rare opportunity for data loss, which manifested as "apparent
wraparound" or "could not access status of transaction" errors.  Data
loss is more likely in pg_multixact, due to released branches' thin
margin between multiStopLimit and multiWrapLimit.  If a user's physical
replication primary logged ":  apparent wraparound" messages, the user
should rebuild standbys of that primary regardless of symptoms.  At less
risk is a cluster having emitted "not accepting commands" errors or
"must be vacuumed" warnings at some point.  One can test a cluster for
this data loss by running VACUUM FREEZE in every database.  Back-patch
to 9.5 (all supported versions).

Discussion: https://postgr.es/m/20190218073103.GA1434723@rfd.leadboat.com

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 26fda20d1939480f1b9a124215fb95acc81a6aa2..fc329c5cff968487281949155de6e05df733da15 100644 (file)
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10226,7 +10226,8 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
    and general database objects (identified by class OID and object OID,
    in the same way as in <structname>pg_description</structname> or
    <structname>pg_depend</structname>).  Also, the right to extend a
-   relation is represented as a separate lockable object.
+   relation is represented as a separate lockable object, as is the right to
+   update <structname>pg_database</structname>.<structfield>datfrozenxid</structfield>.
    Also, <quote>advisory</quote> locks can be taken on numbers that have
    user-defined meanings.
   </para>
@@ -10254,6 +10255,7 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</replaceable>:<replaceable>&l
        Type of the lockable object:
        <literal>relation</literal>,
        <literal>extend</literal>,
+       <literal>frozenid</literal>,
        <literal>page</literal>,
        <literal>tuple</literal>,
        <literal>transactionid</literal>,

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 7dcddf478a1123c4d483621769ff04e07a1514b8..304c49f07b76befa97b838e4853cd41aea78c4b9 100644 (file)
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1742,6 +1742,12 @@ postgres   27093  0.0  0.0  30096  2752 ?        Ss   11:34   0:00 postgres: ser
       <entry><literal>extend</literal></entry>
       <entry>Waiting to extend a relation.</entry>
      </row>
+     <row>
+      <entry><literal>frozenid</literal></entry>
+      <entry>Waiting to
+       update <structname>pg_database</structname>.<structfield>datfrozenxid</structfield>
+       and <structname>pg_database</structname>.<structfield>datminmxid</structfield>.</entry>
+     </row>
      <row>
       <entry><literal>object</literal></entry>
       <entry>Waiting to acquire a lock on a non-relation database object.</entry>
@@ -1910,6 +1916,11 @@ postgres   27093  0.0  0.0  30096  2752 ?        Ss   11:34   0:00 postgres: ser
       <entry><literal>NotifyQueue</literal></entry>
       <entry>Waiting to read or update <command>NOTIFY</command> messages.</entry>
      </row>
+     <row>
+      <entry><literal>NotifyQueueTail</literal></entry>
+      <entry>Waiting to update limit on <command>NOTIFY</command> message
+       storage.</entry>
+     </row>
      <row>
       <entry><literal>NotifySLRU</literal></entry>
       <entry>Waiting to access the <command>NOTIFY</command> message SLRU
@@ -2086,6 +2097,11 @@ postgres   27093  0.0  0.0  30096  2752 ?        Ss   11:34   0:00 postgres: ser
       <entry><literal>WALWrite</literal></entry>
       <entry>Waiting for WAL buffers to be written to disk.</entry>
      </row>
+     <row>
+      <entry><literal>WrapLimitsVacuum</literal></entry>
+      <entry>Waiting to update limits on transaction id and multixact
+       consumption.</entry>
+     </row>
      <row>
       <entry><literal>XactBuffer</literal></entry>
       <entry>Waiting for I/O on a transaction status SLRU buffer.</entry>

diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index d1dbb43e096c1e07f5488e1675525be87f0c14a0..7640f153c227bcfebe06c21599469e8dc9fd39a6 100644 (file)
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -1191,6 +1191,14 @@ SimpleLruFlush(SlruCtl ctl, bool allow_redirtied)
 
 /*
  * Remove all segments before the one holding the passed page number
+ *
+ * All SLRUs prevent concurrent calls to this function, either with an LWLock
+ * or by calling it only as part of a checkpoint.  Mutual exclusion must begin
+ * before computing cutoffPage.  Mutual exclusion must end after any limit
+ * update that would permit other backends to write fresh data into the
+ * segment immediately preceding the one containing cutoffPage.  Otherwise,
+ * when the SLRU is quite full, SimpleLruTruncate() might delete that segment
+ * after it has accrued freshly-written data.
  */
 void
 SimpleLruTruncate(SlruCtl ctl, int cutoffPage)

diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index a087a5554210c30fc46d1e9d6560dba83ca15c23..a50f60b99af280e17813501142d4321656593b59 100644 (file)
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -349,8 +349,8 @@ ExtendSUBTRANS(TransactionId newestXact)
 /*
  * Remove all SUBTRANS segments before the one holding the passed transaction ID
  *
- * This is normally called during checkpoint, with oldestXact being the
- * oldest TransactionXmin of any running transaction.
+ * oldestXact is the oldest TransactionXmin of any running transaction.  This
+ * is called only during checkpoint.
  */
 void
 TruncateSUBTRANS(TransactionId oldestXact)

diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 71b7577afc067ff7814bd199f023f231e299b9a6..4c1286eb988eea89378dc53f37882e4b213edfd5 100644 (file)
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -244,19 +244,22 @@ typedef struct QueueBackendStatus
 /*
  * Shared memory state for LISTEN/NOTIFY (excluding its SLRU stuff)
  *
- * The AsyncQueueControl structure is protected by the NotifyQueueLock.
+ * The AsyncQueueControl structure is protected by the NotifyQueueLock and
+ * NotifyQueueTailLock.
  *
- * When holding the lock in SHARED mode, backends may only inspect their own
- * entries as well as the head and tail pointers. Consequently we can allow a
- * backend to update its own record while holding only SHARED lock (since no
- * other backend will inspect it).
+ * When holding NotifyQueueLock in SHARED mode, backends may only inspect
+ * their own entries as well as the head and tail pointers. Consequently we
+ * can allow a backend to update its own record while holding only SHARED lock
+ * (since no other backend will inspect it).
  *
- * When holding the lock in EXCLUSIVE mode, backends can inspect the entries
- * of other backends and also change the head and tail pointers.
+ * When holding NotifyQueueLock in EXCLUSIVE mode, backends can inspect the
+ * entries of other backends and also change the head pointer. When holding
+ * both NotifyQueueLock and NotifyQueueTailLock in EXCLUSIVE mode, backends
+ * can change the tail pointer.
  *
  * NotifySLRULock is used as the control lock for the pg_notify SLRU buffers.
- * In order to avoid deadlocks, whenever we need both locks, we always first
- * get NotifyQueueLock and then NotifySLRULock.
+ * In order to avoid deadlocks, whenever we need multiple locks, we first get
+ * NotifyQueueTailLock, then NotifyQueueLock, and lastly NotifySLRULock.
  *
  * Each backend uses the backend[] array entry with index equal to its
  * BackendId (which can range from 1 to MaxBackends).  We rely on this to make
@@ -2177,6 +2180,10 @@ asyncQueueAdvanceTail(void)
    int         newtailpage;
    int         boundary;
 
+   /* Restrict task to one backend per cluster; see SimpleLruTruncate(). */
+   LWLockAcquire(NotifyQueueTailLock, LW_EXCLUSIVE);
+
+   /* Compute the new tail. */
    LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE);
    min = QUEUE_HEAD;
    for (BackendId i = QUEUE_FIRST_LISTENER; i > 0; i = QUEUE_NEXT_LISTENER(i))
@@ -2185,7 +2192,6 @@ asyncQueueAdvanceTail(void)
        min = QUEUE_POS_MIN(min, QUEUE_BACKEND_POS(i));
    }
    oldtailpage = QUEUE_POS_PAGE(QUEUE_TAIL);
-   QUEUE_TAIL = min;
    LWLockRelease(NotifyQueueLock);
 
    /*
@@ -2205,6 +2211,17 @@ asyncQueueAdvanceTail(void)
         */
        SimpleLruTruncate(NotifyCtl, newtailpage);
    }
+
+   /*
+    * Advertise the new tail.  This changes asyncQueueIsFull()'s verdict for
+    * the segment immediately prior to the new tail, allowing fresh data into
+    * that segment.
+    */
+   LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE);
+   QUEUE_TAIL = min;
+   LWLockRelease(NotifyQueueLock);
+
+   LWLockRelease(NotifyQueueTailLock);
 }
 
 /*

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index aba13c31d1bc2e957cb7b61bd6983b4f91389740..5189a5ad5e376c7fc1071b1187a3647e8a1d20e7 100644 (file)
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1361,6 +1361,14 @@ vac_update_datfrozenxid(void)
    bool        bogus = false;
    bool        dirty = false;
 
+   /*
+    * Restrict this task to one backend per database.  This avoids race
+    * conditions that would move datfrozenxid or datminmxid backward.  It
+    * avoids calling vac_truncate_clog() with a datfrozenxid preceding a
+    * datfrozenxid passed to an earlier vac_truncate_clog() call.
+    */
+   LockDatabaseFrozenIds(ExclusiveLock);
+
    /*
     * Initialize the "min" calculation with
     * GetOldestNonRemovableTransactionId(), which is a reasonable
@@ -1551,6 +1559,9 @@ vac_truncate_clog(TransactionId frozenXID,
    bool        bogus = false;
    bool        frozenAlreadyWrapped = false;
 
+   /* Restrict task to one backend per cluster; see SimpleLruTruncate(). */
+   LWLockAcquire(WrapLimitsVacuumLock, LW_EXCLUSIVE);
+
    /* init oldest datoids to sync with my frozenXID/minMulti values */
    oldestxid_datoid = MyDatabaseId;
    minmulti_datoid = MyDatabaseId;
@@ -1660,6 +1671,8 @@ vac_truncate_clog(TransactionId frozenXID,
     */
    SetTransactionIdLimit(frozenXID, oldestxid_datoid);
    SetMultiXactIdLimit(minMulti, minmulti_datoid, false);
+
+   LWLockRelease(WrapLimitsVacuumLock);
 }
 
 

diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c
index 20103200952e70d4dc35c3a177bd73df6f41d4da..7409de94059255892cb00110ce0386b24c32c84f 100644 (file)
--- a/src/backend/storage/lmgr/lmgr.c
+++ b/src/backend/storage/lmgr/lmgr.c
@@ -460,6 +460,21 @@ UnlockRelationForExtension(Relation relation, LOCKMODE lockmode)
    LockRelease(&tag, lockmode, false);
 }
 
+/*
+ *     LockDatabaseFrozenIds
+ *
+ * This allows one backend per database to execute vac_update_datfrozenxid().
+ */
+void
+LockDatabaseFrozenIds(LOCKMODE lockmode)
+{
+   LOCKTAG     tag;
+
+   SET_LOCKTAG_DATABASE_FROZEN_IDS(tag, MyDatabaseId);
+
+   (void) LockAcquire(&tag, lockmode, false, false);
+}
+
 /*
  *     LockPage
  *
@@ -1098,6 +1113,11 @@ DescribeLockTag(StringInfo buf, const LOCKTAG *tag)
                             tag->locktag_field2,
                             tag->locktag_field1);
            break;
+       case LOCKTAG_DATABASE_FROZEN_IDS:
+           appendStringInfo(buf,
+                            _("pg_database.datfrozenxid of database %u"),
+                            tag->locktag_field1);
+           break;
        case LOCKTAG_PAGE:
            appendStringInfo(buf,
                             _("page %u of relation %u of database %u"),

diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index e6985e8eedfb1dee824b24ad83fd26b401970640..774292fd942774b87f11dbeebb76f6c5e64f02f2 100644 (file)
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -50,3 +50,6 @@ MultiXactTruncationLock               41
 OldSnapshotTimeMapLock             42
 LogicalRepWorkerLock               43
 XactTruncationLock                 44
+# 45 was XactTruncationLock until removal of BackendRandomLock
+WrapLimitsVacuumLock               46
+NotifyQueueTailLock                    47

diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
index e992d1bbfcedfc16337d86818da3f08ff98b0693..f592292d067b8d5071d1c54c03487688aeacc22a 100644 (file)
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -29,6 +29,7 @@
 const char *const LockTagTypeNames[] = {
    "relation",
    "extend",
+   "frozenid",
    "page",
    "tuple",
    "transactionid",
@@ -254,6 +255,17 @@ pg_lock_status(PG_FUNCTION_ARGS)
                nulls[8] = true;
                nulls[9] = true;
                break;
+           case LOCKTAG_DATABASE_FROZEN_IDS:
+               values[1] = ObjectIdGetDatum(instance->locktag.locktag_field1);
+               nulls[2] = true;
+               nulls[3] = true;
+               nulls[4] = true;
+               nulls[5] = true;
+               nulls[6] = true;
+               nulls[7] = true;
+               nulls[8] = true;
+               nulls[9] = true;
+               break;
            case LOCKTAG_PAGE:
                values[1] = ObjectIdGetDatum(instance->locktag.locktag_field1);
                values[2] = ObjectIdGetDatum(instance->locktag.locktag_field2);

diff --git a/src/include/storage/lmgr.h b/src/include/storage/lmgr.h
index 3acc11aa5a3b1d357f2d2a80ea1198cda6d82e07..f7cabcbbf550e2fe484c5842100a9ac62da0a988 100644 (file)
--- a/src/include/storage/lmgr.h
+++ b/src/include/storage/lmgr.h
@@ -59,6 +59,9 @@ extern bool ConditionalLockRelationForExtension(Relation relation,
                                                LOCKMODE lockmode);
 extern int RelationExtensionLockWaiterCount(Relation relation);
 
+/* Lock to recompute pg_database.datfrozenxid in the current database */
+extern void LockDatabaseFrozenIds(LOCKMODE lockmode);
+
 /* Lock a page (currently only used within indexes) */
 extern void LockPage(Relation relation, BlockNumber blkno, LOCKMODE lockmode);
 extern bool ConditionalLockPage(Relation relation, BlockNumber blkno, LOCKMODE lockmode);

diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index fdabf427210ac98e5babebd6c49dab12fa04d348..1c3e9c1999f5627de2ac0d784974d5acfd7433ce 100644 (file)
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -138,6 +138,7 @@ typedef enum LockTagType
 {
    LOCKTAG_RELATION,           /* whole relation */
    LOCKTAG_RELATION_EXTEND,    /* the right to extend a relation */
+   LOCKTAG_DATABASE_FROZEN_IDS,    /* pg_database.datfrozenxid */
    LOCKTAG_PAGE,               /* one page of a relation */
    LOCKTAG_TUPLE,              /* one physical tuple */
    LOCKTAG_TRANSACTION,        /* transaction (for waiting for xact done) */
@@ -194,6 +195,15 @@ typedef struct LOCKTAG
     (locktag).locktag_type = LOCKTAG_RELATION_EXTEND, \
     (locktag).locktag_lockmethodid = DEFAULT_LOCKMETHOD)
 
+/* ID info for frozen IDs is DB OID */
+#define SET_LOCKTAG_DATABASE_FROZEN_IDS(locktag,dboid) \
+   ((locktag).locktag_field1 = (dboid), \
+    (locktag).locktag_field2 = 0, \
+    (locktag).locktag_field3 = 0, \
+    (locktag).locktag_field4 = 0, \
+    (locktag).locktag_type = LOCKTAG_DATABASE_FROZEN_IDS, \
+    (locktag).locktag_lockmethodid = DEFAULT_LOCKMETHOD)
+
 /* ID info for a page is RELATION info + BlockNumber */
 #define SET_LOCKTAG_PAGE(locktag,dboid,reloid,blocknum) \
    ((locktag).locktag_field1 = (dboid), \