At Thu, 4 Mar 2021 22:37:23 +0500, Ibrar Ahmed <[email protected]> wrote in
> The regression is failing for this patch, do you mind look at that and send
> the updated patch?
>
> https://api.cirrus-ci.com/v1/task/6313174510075904/logs/test.log
>
> ...
> t/006_logical_decoding.pl ............ ok
> t/007_sync_rep.pl .................... ok
> Bailout called. Further testing stopped: system pg_ctl failed
> FAILED--Further testing stopped: system pg_ctl failed
> make[2]: *** [Makefile:19: check] Error 255
> make[1]: *** [Makefile:49: check-recovery-recurse] Error 2
> make: *** [GNUmakefile:71: check-world-src/test-recurse] Error 2
> ...
(I regret that I sent this as .patch file..)
Thaks for pointing that!
The patch assumed that CHKPT_START/COMPLETE barrier are exclusively
used each other, but MarkBufferDirtyHint which delays checkpoint start
is called in RelationTruncate while delaying checkpoint completion.
That is not a strange nor harmful behavior. I changed delayChkpt to a
bitmap integer from an enum so that both barrier are separately
triggered.
I'm not sure this is the way to go here, though. This fixes the issue
of a crash during RelationTruncate, but the issue of smgrtruncate
failure during RelationTruncate still remains (unless we treat that
failure as PANIC?).
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/transam/multixact.c
b/src/backend/access/transam/multixact.c
index 1f9f1a1fa1..c1b0b48362 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -3072,8 +3072,8 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid
newOldestMultiDB)
* crash/basebackup, even though the state of the data directory would
* require it.
*/
- Assert(!MyProc->delayChkpt);
- MyProc->delayChkpt = true;
+ Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
+ MyProc->delayChkpt |= DELAY_CHKPT_START;
/* WAL log truncation */
WriteMTruncateXlogRec(newOldestMultiDB,
@@ -3099,7 +3099,7 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid
newOldestMultiDB)
/* Then offsets */
PerformOffsetsTruncation(oldestMulti, newOldestMulti);
- MyProc->delayChkpt = false;
+ MyProc->delayChkpt &= ~DELAY_CHKPT_START;
END_CRIT_SECTION();
LWLockRelease(MultiXactTruncationLock);
diff --git a/src/backend/access/transam/twophase.c
b/src/backend/access/transam/twophase.c
index 80d2d20d6c..85c720491b 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -463,7 +463,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId
xid, const char *gid,
proc->lxid = (LocalTransactionId) xid;
proc->xid = xid;
Assert(proc->xmin == InvalidTransactionId);
- proc->delayChkpt = false;
+ proc->delayChkpt = 0;
proc->statusFlags = 0;
proc->pid = 0;
proc->backendId = InvalidBackendId;
@@ -1109,7 +1109,8 @@ EndPrepare(GlobalTransaction gxact)
START_CRIT_SECTION();
- MyProc->delayChkpt = true;
+ Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
+ MyProc->delayChkpt |= DELAY_CHKPT_START;
XLogBeginInsert();
for (record = records.head; record != NULL; record = record->next)
@@ -1152,7 +1153,7 @@ EndPrepare(GlobalTransaction gxact)
* checkpoint starting after this will certainly see the gxact as a
* candidate for fsyncing.
*/
- MyProc->delayChkpt = false;
+ MyProc->delayChkpt &= ~DELAY_CHKPT_START;
/*
* Remember that we have this GlobalTransaction entry locked for us. If
@@ -2198,7 +2199,8 @@ RecordTransactionCommitPrepared(TransactionId xid,
START_CRIT_SECTION();
/* See notes in RecordTransactionCommit */
- MyProc->delayChkpt = true;
+ Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
+ MyProc->delayChkpt |= DELAY_CHKPT_START;
/*
* Emit the XLOG commit record. Note that we mark 2PC commits as
@@ -2246,7 +2248,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
TransactionIdCommitTree(xid, nchildren, children);
/* Checkpoint can proceed now */
- MyProc->delayChkpt = false;
+ MyProc->delayChkpt &= ~DELAY_CHKPT_START;
END_CRIT_SECTION();
diff --git a/src/backend/access/transam/xact.c
b/src/backend/access/transam/xact.c
index 4e6a3df6b8..f033e8940a 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1334,8 +1334,9 @@ RecordTransactionCommit(void)
* This makes checkpoint's determination of which xacts are
delayChkpt
* a bit fuzzy, but it doesn't matter.
*/
+ Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
START_CRIT_SECTION();
- MyProc->delayChkpt = true;
+ MyProc->delayChkpt |= DELAY_CHKPT_START;
SetCurrentTransactionStopTimestamp();
@@ -1436,7 +1437,7 @@ RecordTransactionCommit(void)
*/
if (markXidCommitted)
{
- MyProc->delayChkpt = false;
+ MyProc->delayChkpt &= ~DELAY_CHKPT_START;
END_CRIT_SECTION();
}
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 377afb8732..5f5703bd57 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9065,18 +9065,30 @@ 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);
+ vxids = GetVirtualXIDsDelayingChkpt(&nvxids, DELAY_CHKPT_START);
if (nvxids > 0)
{
do
{
pg_usleep(10000L); /* wait for 10 msec */
- } while (HaveVirtualXIDsDelayingChkpt(vxids, nvxids));
+ } while (HaveVirtualXIDsDelayingChkpt(vxids, nvxids,
+
DELAY_CHKPT_START));
}
pfree(vxids);
CheckPointGuts(checkPoint.redo, flags);
+ vxids = GetVirtualXIDsDelayingChkpt(&nvxids, DELAY_CHKPT_COMPLETE);
+ if (nvxids > 0)
+ {
+ do
+ {
+ pg_usleep(10000L); /* wait for 10 msec */
+ } while (HaveVirtualXIDsDelayingChkpt(vxids, nvxids,
+
DELAY_CHKPT_COMPLETE));
+ }
+ pfree(vxids);
+
/*
* Take a snapshot of running transactions and write this to WAL. This
* allows us to reconstruct the state of running transactions during
diff --git a/src/backend/access/transam/xloginsert.c
b/src/backend/access/transam/xloginsert.c
index 7052dc245e..1edd1b67ff 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -923,7 +923,7 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std)
/*
* Ensure no checkpoint can change our view of RedoRecPtr.
*/
- Assert(MyProc->delayChkpt);
+ Assert((MyProc->delayChkpt & DELAY_CHKPT_START) != 0);
/*
* Update RedoRecPtr so that we can make the right decision
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index cba7a9ada0..579f23c991 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -325,6 +325,16 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
RelationPreTruncate(rel);
+ /*
+ * If the file truncation fails but the concurrent checkpoint completes
+ * just before that, the next crash recovery can fail due to WAL records
+ * inconsistent with the untruncated pages. To avoid that situation we
+ * delay the checkpoint completion until we confirm the truncation to be
+ * successful.
+ */
+ Assert((MyProc->delayChkpt & DELAY_CHKPT_COMPLETE) == 0);
+ MyProc->delayChkpt |= DELAY_CHKPT_COMPLETE;
+
/*
* We WAL-log the truncation before actually truncating, which means
* trouble if the truncation fails. If we then crash, the WAL replay
@@ -373,6 +383,8 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
*/
if (need_fsm_vacuum)
FreeSpaceMapVacuumRange(rel, nblocks, InvalidBlockNumber);
+
+ MyProc->delayChkpt &= ~DELAY_CHKPT_COMPLETE;
}
/*
diff --git a/src/backend/storage/buffer/bufmgr.c
b/src/backend/storage/buffer/bufmgr.c
index 561c212092..1c9e971b31 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3803,7 +3803,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
{
XLogRecPtr lsn = InvalidXLogRecPtr;
bool dirtied = false;
- bool delayChkpt = false;
+ int delayChkptMask = ~0;
uint32 buf_state;
/*
@@ -3853,7 +3853,9 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
* essential that CreateCheckpoint waits for virtual
transactions
* rather than full transactionids.
*/
- MyProc->delayChkpt = delayChkpt = true;
+ Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
+ MyProc->delayChkpt |= DELAY_CHKPT_START;
+ delayChkptMask = ~DELAY_CHKPT_START;
lsn = XLogSaveBufferForHint(buffer, buffer_std);
}
@@ -3885,8 +3887,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
buf_state |= BM_DIRTY | BM_JUST_DIRTIED;
UnlockBufHdr(bufHdr, buf_state);
- if (delayChkpt)
- MyProc->delayChkpt = false;
+ MyProc->delayChkpt &= delayChkptMask;
if (dirtied)
{
diff --git a/src/backend/storage/ipc/procarray.c
b/src/backend/storage/ipc/procarray.c
index 4fc6ffb917..3e6759886a 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -655,7 +655,10 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId
latestXid)
proc->lxid = InvalidLocalTransactionId;
proc->xmin = InvalidTransactionId;
- proc->delayChkpt = false; /* be sure this is cleared in
abort */
+
+ /* be sure this is cleared in abort */
+ proc->delayChkpt = 0;
+
proc->recoveryConflictPending = false;
/* must be cleared with xid/xmin: */
@@ -694,7 +697,10 @@ ProcArrayEndTransactionInternal(PGPROC *proc,
TransactionId latestXid)
proc->xid = InvalidTransactionId;
proc->lxid = InvalidLocalTransactionId;
proc->xmin = InvalidTransactionId;
- proc->delayChkpt = false; /* be sure this is cleared in abort */
+
+ /* be sure this is cleared in abort */
+ proc->delayChkpt = 0;
+
proc->recoveryConflictPending = false;
/* must be cleared with xid/xmin: */
@@ -2955,7 +2961,8 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly)
* delaying 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 delayChkpt set in their PGPROC.
+ * critical sections, as shown by having delayChkpt set to the specified value
+ * in their PGPROC.
*
* Returns a palloc'd array that should be freed by the caller.
* *nvxids is the number of valid entries.
@@ -2969,13 +2976,15 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly)
* for clearing of delayChkpt to propagate is unimportant for correctness.
*/
VirtualTransactionId *
-GetVirtualXIDsDelayingChkpt(int *nvxids)
+GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
{
VirtualTransactionId *vxids;
ProcArrayStruct *arrayP = procArray;
int count = 0;
int index;
+ Assert(type != 0);
+
/* allocate what's certainly enough result space */
vxids = (VirtualTransactionId *)
palloc(sizeof(VirtualTransactionId) * arrayP->maxProcs);
@@ -2987,7 +2996,7 @@ GetVirtualXIDsDelayingChkpt(int *nvxids)
int pgprocno = arrayP->pgprocnos[index];
PGPROC *proc = &allProcs[pgprocno];
- if (proc->delayChkpt)
+ if ((proc->delayChkpt & type) != 0)
{
VirtualTransactionId vxid;
@@ -3013,12 +3022,14 @@ GetVirtualXIDsDelayingChkpt(int *nvxids)
* those numbers should be small enough for it not to be a problem.
*/
bool
-HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids)
+HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
{
bool result = false;
ProcArrayStruct *arrayP = procArray;
int index;
+ Assert(type != 0);
+
LWLockAcquire(ProcArrayLock, LW_SHARED);
for (index = 0; index < arrayP->numProcs; index++)
@@ -3029,7 +3040,8 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids,
int nvxids)
GET_VXID_FROM_PGPROC(vxid, *proc);
- if (proc->delayChkpt && VirtualTransactionIdIsValid(vxid))
+ if ((proc->delayChkpt & type) != 0 &&
+ VirtualTransactionIdIsValid(vxid))
{
int i;
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 897045ee27..7915cdd484 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -394,7 +394,7 @@ InitProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyProc->delayChkpt = false;
+ MyProc->delayChkpt = 0;
MyProc->statusFlags = 0;
/* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
if (IsAutoVacuumWorkerProcess())
@@ -576,7 +576,7 @@ InitAuxiliaryProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyProc->delayChkpt = false;
+ MyProc->delayChkpt = 0;
MyProc->statusFlags = 0;
MyProc->lwWaiting = false;
MyProc->lwWaitMode = 0;
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index a777cb64a1..2799debdaf 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -79,6 +79,10 @@ struct XidCache
*/
#define INVALID_PGPROCNO PG_INT32_MAX
+/* symbols for PGPROC.delayChkpt */
+#define DELAY_CHKPT_START (1<<0)
+#define DELAY_CHKPT_COMPLETE (1<<1)
+
typedef enum
{
PROC_WAIT_STATUS_OK,
@@ -184,7 +188,8 @@ struct PGPROC
pg_atomic_uint64 waitStart; /* time at which wait for lock acquisition
* started */
- bool delayChkpt; /* true if this proc delays
checkpoint start */
+ int delayChkpt; /* if this proc delays
checkpoint start and/or
+ * completion.
*/
uint8 statusFlags; /* this backend's status flags, see
PROC_*
* above.
mirrored in
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index b01fa52139..ec40130466 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -15,11 +15,11 @@
#define PROCARRAY_H
#include "storage/lock.h"
+#include "storage/proc.h"
#include "storage/standby.h"
#include "utils/relcache.h"
#include "utils/snapshot.h"
-
extern Size ProcArrayShmemSize(void);
extern void CreateSharedProcArray(void);
extern void ProcArrayAdd(PGPROC *proc);
@@ -59,8 +59,9 @@ extern TransactionId GetOldestActiveTransactionId(void);
extern TransactionId GetOldestSafeDecodingTransactionId(bool catalogOnly);
extern void GetReplicationHorizons(TransactionId *slot_xmin, TransactionId
*catalog_xmin);
-extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids);
-extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int
nvxids);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids, int
type);
+extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids,
+
int nvxids, int type);
extern PGPROC *BackendPidGetProc(int pid);
extern PGPROC *BackendPidGetProcWithLock(int pid);
