]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Rethink the delay-checkpoint-end mechanism in the back-branches.
authorRobert Haas <rhaas@postgresql.org>
Thu, 14 Apr 2022 15:10:13 +0000 (11:10 -0400)
committerRobert Haas <rhaas@postgresql.org>
Thu, 14 Apr 2022 15:10:13 +0000 (11:10 -0400)
The back-patch of commit bbace5697df12398e87ffd9879171c39d27f5b33 had
the unfortunate effect of changing the layout of PGPROC in the
back-branches, which could break extensions. This happened because it
changed the delayChkpt from type bool to type int. So, change it back,
and add a new bool delayChkptEnd field instead. The new field should
fall within what used to be padding space within the struct, and so
hopefully won't cause any extensions to break.

Per report from Markus Wanner and discussion with Tom Lane and others.

Patch originally by me, somewhat revised by Markus Wanner per a
suggestion from Michael Paquier. A very similar patch was developed
by Kyotaro Horiguchi, but I failed to see the email in which that was
posted before writing one of my own.

Discussion: http://postgr.es/m/CA+Tgmoao-kUD9c5nG5sub3F7tbo39+cdr8jKaOVEs_1aBWcJ3Q@mail.gmail.com
Discussion: http://postgr.es/m/20220406.164521.17171257901083417.horikyota.ntt@gmail.com

src/backend/access/transam/multixact.c
src/backend/access/transam/twophase.c
src/backend/access/transam/xact.c
src/backend/access/transam/xlog.c
src/backend/access/transam/xloginsert.c
src/backend/catalog/storage.c
src/backend/storage/buffer/bufmgr.c
src/backend/storage/ipc/procarray.c
src/include/storage/proc.h
src/include/storage/procarray.h

index 757346cbbb566c8e76a595494cc94f14490212a9..09748905a8c848ee596bedc3c73f680dddf38370 100644 (file)
@@ -3069,8 +3069,8 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
         * crash/basebackup, even though the state of the data directory would
         * require it.
         */
-       Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) == 0);
-       MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+       Assert(!MyPgXact->delayChkpt);
+       MyPgXact->delayChkpt = true;
 
        /* WAL log truncation */
        WriteMTruncateXlogRec(newOldestMultiDB,
@@ -3096,7 +3096,7 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
        /* Then offsets */
        PerformOffsetsTruncation(oldestMulti, newOldestMulti);
 
-       MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+       MyPgXact->delayChkpt = false;
 
        END_CRIT_SECTION();
        LWLockRelease(MultiXactTruncationLock);
index 602ca410540226d0dd01340c08ce575816b6d81c..50dbc50e1b62a14fe2d73cff7489f4e5d1da448a 100644 (file)
@@ -477,8 +477,9 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
        }
        pgxact->xid = xid;
        pgxact->xmin = InvalidTransactionId;
-       pgxact->delayChkpt = 0;
+       pgxact->delayChkpt = false;
        pgxact->vacuumFlags = 0;
+       proc->delayChkptEnd = false;
        proc->pid = 0;
        proc->databaseId = databaseid;
        proc->roleId = owner;
@@ -1187,8 +1188,8 @@ EndPrepare(GlobalTransaction gxact)
 
        START_CRIT_SECTION();
 
-       Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) == 0);
-       MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+       Assert(!MyPgXact->delayChkpt);
+       MyPgXact->delayChkpt = true;
 
        XLogBeginInsert();
        for (record = records.head; record != NULL; record = record->next)
@@ -1231,7 +1232,7 @@ EndPrepare(GlobalTransaction gxact)
         * checkpoint starting after this will certainly see the gxact as a
         * candidate for fsyncing.
         */
-       MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+       MyPgXact->delayChkpt = false;
 
        /*
         * Remember that we have this GlobalTransaction entry locked for us.  If
@@ -2338,8 +2339,8 @@ RecordTransactionCommitPrepared(TransactionId xid,
        START_CRIT_SECTION();
 
        /* See notes in RecordTransactionCommit */
-       Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) == 0);
-       MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+       Assert(!MyPgXact->delayChkpt);
+       MyPgXact->delayChkpt = true;
 
        /*
         * Emit the XLOG commit record. Note that we mark 2PC commits as
@@ -2387,7 +2388,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
        TransactionIdCommitTree(xid, nchildren, children);
 
        /* Checkpoint can proceed now */
-       MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+       MyPgXact->delayChkpt = false;
 
        END_CRIT_SECTION();
 
index 9d23298b2b635a47af9f55add4bd5a14bb91a0c1..a86ad1cec41e29f2bacd53f043e7f3ebff51a4c8 100644 (file)
@@ -1306,9 +1306,9 @@ RecordTransactionCommit(void)
                 * This makes checkpoint's determination of which xacts are delayChkpt
                 * a bit fuzzy, but it doesn't matter.
                 */
-               Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) == 0);
+               Assert(!MyPgXact->delayChkpt);
                START_CRIT_SECTION();
-               MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+               MyPgXact->delayChkpt = true;
 
                SetCurrentTransactionStopTimestamp();
 
@@ -1409,7 +1409,7 @@ RecordTransactionCommit(void)
         */
        if (markXidCommitted)
        {
-               MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+               MyPgXact->delayChkpt = false;
                END_CRIT_SECTION();
        }
 
index 7141e5dca8303c724e31a7f88683e35c0bf52b50..889606517cf012c656f6825c87ba0c1a8315d74c 100644 (file)
@@ -8920,27 +8920,25 @@ CreateCheckPoint(int flags)
         * and we will correctly flush the update below.  So we cannot miss any
         * xacts we need to wait for.
         */
-       vxids = GetVirtualXIDsDelayingChkpt(&nvxids, DELAY_CHKPT_START);
+       vxids = GetVirtualXIDsDelayingChkpt(&nvxids);
        if (nvxids > 0)
        {
                do
                {
                        pg_usleep(10000L);      /* wait for 10 msec */
-               } while (HaveVirtualXIDsDelayingChkpt(vxids, nvxids,
-                                                                                         DELAY_CHKPT_START));
+               } while (HaveVirtualXIDsDelayingChkpt(vxids, nvxids));
        }
        pfree(vxids);
 
        CheckPointGuts(checkPoint.redo, flags);
 
-       vxids = GetVirtualXIDsDelayingChkpt(&nvxids, DELAY_CHKPT_COMPLETE);
+       vxids = GetVirtualXIDsDelayingChkptEnd(&nvxids);
        if (nvxids > 0)
        {
                do
                {
                        pg_usleep(10000L);      /* wait for 10 msec */
-               } while (HaveVirtualXIDsDelayingChkpt(vxids, nvxids,
-                                                                                         DELAY_CHKPT_COMPLETE));
+               } while (HaveVirtualXIDsDelayingChkptEnd(vxids, nvxids));
        }
        pfree(vxids);
 
index b51b0edd67c5d32143061e8952128e6de111458e..24a6f3148b119e7e325c6abfae05c7220ff0c6dc 100644 (file)
@@ -899,7 +899,7 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std)
        /*
         * Ensure no checkpoint can change our view of RedoRecPtr.
         */
-       Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) != 0);
+       Assert(MyPgXact->delayChkpt);
 
        /*
         * Update RedoRecPtr so that we can make the right decision
index 5a6324fec4c215d46fb9238722133b96b9a93f90..5c76e7a39baf1c76bde686fc0c12fc2b156634b1 100644 (file)
@@ -266,8 +266,8 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
         * the blocks to not exist on disk at all, but not for them to have the
         * wrong contents.
         */
-       Assert((MyPgXact->delayChkpt & DELAY_CHKPT_COMPLETE) == 0);
-       MyPgXact->delayChkpt |= DELAY_CHKPT_COMPLETE;
+       Assert(!MyProc->delayChkptEnd);
+       MyProc->delayChkptEnd = true;
 
        /*
         * We WAL-log the truncation before actually truncating, which means
@@ -315,7 +315,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
        smgrtruncate(rel->rd_smgr, MAIN_FORKNUM, nblocks);
 
        /* We've done all the critical work, so checkpoints are OK now. */
-       MyPgXact->delayChkpt &= ~DELAY_CHKPT_COMPLETE;
+       MyProc->delayChkptEnd = false;
 }
 
 /*
index 7d11b0963f334b99e53a89327f99a917004c481b..303f82aa233c7a640edf601256a6d76e1c2478d1 100644 (file)
@@ -3514,8 +3514,8 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
                         * essential that CreateCheckpoint waits for virtual transactions
                         * rather than full transactionids.
                         */
-                       Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) == 0);
-                       MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+                       Assert(!MyPgXact->delayChkpt);
+                       MyPgXact->delayChkpt = true;
                        delayChkpt = true;
                        lsn = XLogSaveBufferForHint(buffer, buffer_std);
                }
@@ -3549,7 +3549,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
                UnlockBufHdr(bufHdr, buf_state);
 
                if (delayChkpt)
-                       MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+                       MyPgXact->delayChkpt = false;
 
                if (dirtied)
                {
index 39093253fe1afb57e97b525b88778a3fedf755a1..96b22d3881e7b8b3b4f5d310dd4efec41822f424 100644 (file)
@@ -152,6 +152,11 @@ static void DisplayXidCache(void);
 #define xc_slow_answer_inc()           ((void) 0)
 #endif                                                 /* XIDCACHE_DEBUG */
 
+static VirtualTransactionId *GetVirtualXIDsDelayingChkptGuts(int *nvxids,
+                                                                                                                        int type);
+static bool HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids,
+                                                                                        int nvxids, int type);
+
 /* Primitives for KnownAssignedXids array handling for standby */
 static void KnownAssignedXidsCompress(bool force);
 static void KnownAssignedXidsAdd(TransactionId from_xid, TransactionId to_xid,
@@ -435,8 +440,9 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
                /* must be cleared with xid/xmin: */
                pgxact->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
 
-               /* be sure this is cleared in abort */
-               pgxact->delayChkpt = 0;
+               /* be sure these are cleared in abort */
+               pgxact->delayChkpt = false;
+               proc->delayChkptEnd = false;
 
                proc->recoveryConflictPending = false;
 
@@ -460,8 +466,9 @@ ProcArrayEndTransactionInternal(PGPROC *proc, PGXACT *pgxact,
        /* must be cleared with xid/xmin: */
        pgxact->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
 
-       /* be sure this is cleared in abort */
-       pgxact->delayChkpt = 0;
+       /* be sure these are cleared in abort */
+       pgxact->delayChkpt = false;
+       proc->delayChkptEnd = false;
 
        proc->recoveryConflictPending = false;
 
@@ -2263,26 +2270,28 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly)
 }
 
 /*
- * GetVirtualXIDsDelayingChkpt -- Get the VXIDs of transactions that are
- * delaying checkpoint because they have critical actions in progress.
+ * GetVirtualXIDsDelayingChkptGuts -- Get the VXIDs of transactions that are
+ * delaying the start or end of a checkpoint because they have critical
+ * actions in progress.
  *
  * Constructs an array of VXIDs of transactions that are currently in commit
- * critical sections, as shown by having specified delayChkpt bits set in their
- * PGXACT.
+ * critical sections, as shown by having specified delayChkpt or delayChkptEnd
+ * set.
  *
  * Returns a palloc'd array that should be freed by the caller.
  * *nvxids is the number of valid entries.
  *
- * Note that because backends set or clear delayChkpt without holding any lock,
- * the result is somewhat indeterminate, but we don't really care.  Even in
- * a multiprocessor with delayed writes to shared memory, it should be certain
- * that setting of delayChkpt will propagate to shared memory when the backend
- * takes a lock, so we cannot fail to see a virtual xact as delayChkpt if
- * it's already inserted its commit record.  Whether it takes a little while
- * for clearing of delayChkpt to propagate is unimportant for correctness.
+ * Note that because backends set or clear delayChkpt and delayChkptEnd
+ * without holding any lock, the result is somewhat indeterminate, but we
+ * don't really care.  Even in a multiprocessor with delayed writes to
+ * shared memory, it should be certain that setting of delayChkpt will
+ * propagate to shared memory when the backend takes a lock, so we cannot
+ * fail to see a virtual xact as delayChkpt if it's already inserted its
+ * commit record.  Whether it takes a little while for clearing of
+ * delayChkpt to propagate is unimportant for correctness.
  */
-VirtualTransactionId *
-GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
+static VirtualTransactionId *
+GetVirtualXIDsDelayingChkptGuts(int *nvxids, int type)
 {
        VirtualTransactionId *vxids;
        ProcArrayStruct *arrayP = procArray;
@@ -2303,7 +2312,8 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
                PGPROC     *proc = &allProcs[pgprocno];
                PGXACT     *pgxact = &allPgXact[pgprocno];
 
-               if ((pgxact->delayChkpt & type) != 0)
+               if (((type & DELAY_CHKPT_START) && pgxact->delayChkpt) ||
+                       ((type & DELAY_CHKPT_COMPLETE) && proc->delayChkptEnd))
                {
                        VirtualTransactionId vxid;
 
@@ -2319,6 +2329,26 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
        return vxids;
 }
 
+/*
+ * GetVirtualXIDsDelayingChkpt - Get the VXIDs of transactions that are
+ * delaying the start of a checkpoint.
+ */
+VirtualTransactionId *
+GetVirtualXIDsDelayingChkpt(int *nvxids)
+{
+       return GetVirtualXIDsDelayingChkptGuts(nvxids, DELAY_CHKPT_START);
+}
+
+/*
+ * GetVirtualXIDsDelayingChkptEnd - Get the VXIDs of transactions that are
+ * delaying the end of a checkpoint.
+ */
+VirtualTransactionId *
+GetVirtualXIDsDelayingChkptEnd(int *nvxids)
+{
+       return GetVirtualXIDsDelayingChkptGuts(nvxids, DELAY_CHKPT_COMPLETE);
+}
+
 /*
  * HaveVirtualXIDsDelayingChkpt -- Are any of the specified VXIDs delaying?
  *
@@ -2328,8 +2358,9 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
  * Note: this is O(N^2) in the number of vxacts that are/were delaying, but
  * those numbers should be small enough for it not to be a problem.
  */
-bool
-HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
+static bool
+HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids, int nvxids,
+                                                                int type)
 {
        bool            result = false;
        ProcArrayStruct *arrayP = procArray;
@@ -2348,7 +2379,8 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
 
                GET_VXID_FROM_PGPROC(vxid, *proc);
 
-               if ((pgxact->delayChkpt & type) != 0 &&
+               if ((((type & DELAY_CHKPT_START) && pgxact->delayChkpt) ||
+                        ((type & DELAY_CHKPT_COMPLETE) && proc->delayChkptEnd)) &&
                        VirtualTransactionIdIsValid(vxid))
                {
                        int                     i;
@@ -2371,6 +2403,28 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
        return result;
 }
 
+/*
+ * HaveVirtualXIDsDelayingChkpt -- Are any of the specified VXIDs delaying
+ * the start of a checkpoint?
+ */
+bool
+HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids)
+{
+       return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids,
+                                                                                       DELAY_CHKPT_START);
+}
+
+/*
+ * HaveVirtualXIDsDelayingChkptEnd -- Are any of the specified VXIDs delaying
+ * the end of a checkpoint?
+ */
+bool
+HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids, int nvxids)
+{
+       return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids,
+                                                                                       DELAY_CHKPT_COMPLETE);
+}
+
 /*
  * BackendPidGetProc -- get a backend's PGPROC given its PID
  *
index 2a16fd23d4bfa90db79dcfcf315c0a32c7586c3d..7024df206db8a3fc451d46b2eddf29cf3cca3fdb 100644 (file)
@@ -77,36 +77,8 @@ struct XidCache
 #define INVALID_PGPROCNO               PG_INT32_MAX
 
 /*
- * Flags for PGPROC.delayChkpt
- *
- * These flags can be used to delay the start or completion of a checkpoint
- * for short periods. A flag is in effect if the corresponding bit is set in
- * the PGPROC of any backend.
- *
- * For our purposes here, a checkpoint has three phases: (1) determine the
- * location to which the redo pointer will be moved, (2) write all the
- * data durably to disk, and (3) WAL-log the checkpoint.
- *
- * Setting DELAY_CHKPT_START prevents the system from moving from phase 1
- * to phase 2. This is useful when we are performing a WAL-logged modification
- * of data that will be flushed to disk in phase 2. By setting this flag
- * before writing WAL and clearing it after we've both written WAL and
- * performed the corresponding modification, we ensure that if the WAL record
- * is inserted prior to the new redo point, the corresponding data changes will
- * also be flushed to disk before the checkpoint can complete. (In the
- * extremely common case where the data being modified is in shared buffers
- * and we acquire an exclusive content lock on the relevant buffers before
- * writing WAL, this mechanism is not needed, because phase 2 will block
- * until we release the content lock and then flush the modified data to
- * disk.)
- *
- * Setting DELAY_CHKPT_COMPLETE prevents the system from moving from phase 2
- * to phase 3. This is useful if we are performing a WAL-logged operation that
- * might invalidate buffers, such as relation truncation. In this case, we need
- * to ensure that any buffers which were invalidated and thus not flushed by
- * the checkpoint are actaully destroyed on disk. Replay can cope with a file
- * or block that doesn't exist, but not with a block that has the wrong
- * contents.
+ * Flags used only for type of internal functions
+ * GetVirtualXIDsDelayingChkptGuts and HaveVirtualXIDsDelayingChkptGuts.
  */
 #define DELAY_CHKPT_START              (1<<0)
 #define DELAY_CHKPT_COMPLETE   (1<<1)
@@ -185,6 +157,12 @@ struct PGPROC
         */
        XLogRecPtr      waitLSN;                /* waiting for this LSN or higher */
        int                     syncRepState;   /* wait state for sync rep */
+       bool            delayChkptEnd;  /* true if this proc delays checkpoint end;
+                                                                * this doesn't have anything to do with
+                                                                * sync rep but we don't want to change
+                                                                * the size of PGPROC in released branches
+                                                                * and thus must fit this new field into
+                                                                * existing padding space  */
        SHM_QUEUE       syncRepLinks;   /* list link if process is in syncrep queue */
 
        /*
@@ -267,7 +245,7 @@ typedef struct PGXACT
 
        uint8           vacuumFlags;    /* vacuum-related flags, see above */
        bool            overflowed;
-       int                     delayChkpt;             /* for DELAY_CHKPT_* flags */
+       bool            delayChkpt;             /* true if this proc delays checkpoint start */
 
        uint8           nxids;
 } PGXACT;
index d9ca460efc45c915f573a707b9f9609886f74190..4b4f2c3cb88603187fe907a6ae1db7e3cf1c0625 100644 (file)
@@ -92,9 +92,12 @@ extern TransactionId GetOldestXmin(Relation rel, int flags);
 extern TransactionId GetOldestActiveTransactionId(void);
 extern TransactionId GetOldestSafeDecodingTransactionId(bool catalogOnly);
 
-extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids, int type);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkptEnd(int *nvxids);
 extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids,
-                                                                                int nvxids, int type);
+                                                                                int nvxids);
+extern bool HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids,
+                                                                                       int nvxids);
 
 extern PGPROC *BackendPidGetProc(int pid);
 extern PGPROC *BackendPidGetProcWithLock(int pid);