pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedAPI stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
23+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2022-04-14 10520f4346 d18c913b78 68e605b9ef 6270ee4450 48b398943b Rethink the delay-checkpoint-end mechanism in the back-branches.
2022-04-08 f37015a161 Rename delayChkpt to delayChkptFlags.
* API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-05 13:02 Markus Wanner <markus.wanner@enterprisedb.com>
0 siblings, 1 reply; 23+ messages in thread
From: Markus Wanner @ 2022-04-05 13:02 UTC (permalink / raw)
To: Robert Haas <rhaas@postgresql.org>; pgsql-hackers
On 24.03.22 20:32, Robert Haas wrote:
> Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.
This patch changed the delayChkpt field of struct PGPROC from bool to
int. Back-porting this change could be considered an API breaking
change for extensions using this field.
I'm not certain about padding behavior of compilers in general (or
standards requirements around that), but at least on my machine, it
seems sizeof(PGPROC) did not change, so padding led to subsequent fields
still having the same offset.
Nonetheless, the meaning of the field itself changed. And the
additional assert now also triggers for the following pseudo-code of the
extension I'm concerned about:
/*
* Prevent checkpoints being emitted in between additional
* information in the logical message and the following
* prepare record.
*/
MyProc->delayChkpt = true;
LogLogicalMessage(...);
/* Note that this will also reset the delayChkpt flag. */
PrepareTransaction(...);
Now, I'm well aware this is not an official API, it just happens to be
accessible for extensions. So I guess the underlying question is: What
can extension developers expect? Which parts are okay to change even in
stable branches and which can be relied upon to remain stable?
And for this specific case: Is it worth reverting this change and
applying a fully backwards compatible fix, instead?
Regards
Markus Wanner
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-05 14:01 Robert Haas <robertmhaas@gmail.com>
parent: Markus Wanner <markus.wanner@enterprisedb.com>
0 siblings, 1 reply; 23+ messages in thread
From: Robert Haas @ 2022-04-05 14:01 UTC (permalink / raw)
To: Markus Wanner <markus.wanner@enterprisedb.com>; +Cc: pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Tue, Apr 5, 2022 at 9:02 AM Markus Wanner
<markus.wanner@enterprisedb.com> wrote:
> And for this specific case: Is it worth reverting this change and
> applying a fully backwards compatible fix, instead?
I think it's normally our policy to avoid changing definitions of
accessible structs in back branches, except that we allow ourselves
the indulgence of adding new members at the end or in padding space.
So what would probably be best is if, in the back-branches, we changed
"delayChkpt" back to a boolean, renamed it to delayChkptStart, and
added a separate Boolean called delayChkptEnd. Maybe that could be
added just after statusFlags, where I think it would fall into padding
space.
I think as the person who committed that patch I'm on the hook to fix
this if nobody else would like to do it, but let me ask whether
Kyotaro Horiguchi would like to propose a patch, since the original
patch did, and/or whether you would like to propose a patch, as the
person reporting the issue.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-05 14:17 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 2 replies; 23+ messages in thread
From: Tom Lane @ 2022-04-05 14:17 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 5, 2022 at 9:02 AM Markus Wanner
> <markus.wanner@enterprisedb.com> wrote:
>> And for this specific case: Is it worth reverting this change and
>> applying a fully backwards compatible fix, instead?
> I think it's normally our policy to avoid changing definitions of
> accessible structs in back branches, except that we allow ourselves
> the indulgence of adding new members at the end or in padding space.
> So what would probably be best is if, in the back-branches, we changed
> "delayChkpt" back to a boolean, renamed it to delayChkptStart, and
> added a separate Boolean called delayChkptEnd. Maybe that could be
> added just after statusFlags, where I think it would fall into padding
> space.
Renaming it would constitute an API break, which is if anything worse
than an ABI break.
While we're complaining at you, let me point out that changing a field's
content and semantics while not changing its name is a time bomb waiting
to break any third-party code that looks at (or modifies...) the field.
What I think you need to do is:
1. In the back branches, revert delayChkpt to its previous type and
semantics. Squeeze a separate delayChkptEnd bool in somewhere
(you can't change the struct size either ...).
2. In HEAD, rename the field to something like delayChkptFlags,
to ensure that any code touching it has to be inspected and updated.
In other words, this is already an API break in HEAD, and that's
fine, but it didn't break it hard enough to draw anyone's attention,
which is not fine.
regards, tom lane
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-05 14:29 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 23+ messages in thread
From: Robert Haas @ 2022-04-05 14:29 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Tue, Apr 5, 2022 at 10:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Renaming it would constitute an API break, which is if anything worse
> than an ABI break.
I don't think so, because an API break will cause a compilation
failure, which an extension author can easily fix.
> While we're complaining at you, let me point out that changing a field's
> content and semantics while not changing its name is a time bomb waiting
> to break any third-party code that looks at (or modifies...) the field.
>
> What I think you need to do is:
>
> 1. In the back branches, revert delayChkpt to its previous type and
> semantics. Squeeze a separate delayChkptEnd bool in somewhere
> (you can't change the struct size either ...).
>
> 2. In HEAD, rename the field to something like delayChkptFlags,
> to ensure that any code touching it has to be inspected and updated.
Well, we can do it that way, I suppose.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-05 14:32 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 23+ messages in thread
From: Tom Lane @ 2022-04-05 14:32 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 5, 2022 at 10:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Renaming it would constitute an API break, which is if anything worse
>> than an ABI break.
> I don't think so, because an API break will cause a compilation
> failure, which an extension author can easily fix.
My point is that we want that to happen in HEAD, but it's not okay
for it to happen in a minor release of a stable branch.
regards, tom lane
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-05 14:51 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 23+ messages in thread
From: Robert Haas @ 2022-04-05 14:51 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Tue, Apr 5, 2022 at 10:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Tue, Apr 5, 2022 at 10:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Renaming it would constitute an API break, which is if anything worse
> >> than an ABI break.
>
> > I don't think so, because an API break will cause a compilation
> > failure, which an extension author can easily fix.
>
> My point is that we want that to happen in HEAD, but it's not okay
> for it to happen in a minor release of a stable branch.
I understand, but I am not sure that I agree. I think that if an
extension stops compiling against a back-branch, someone will notice
the next time they try to compile it and will fix it. Maybe that's not
amazing, but I don't think it's a huge deal either. On the other hand,
if existing builds that someone has already shipped stop working with
a new server release, that's a much larger issue. The extension
packager can't go back and retroactively add a dependency on the
server version to the already-shipped package. A new package can be
shipped and specify a minimum minor version with which it will work,
but an old package is what it is.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-05 19:16 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 23+ messages in thread
From: Tom Lane @ 2022-04-05 19:16 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 5, 2022 at 10:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> My point is that we want that to happen in HEAD, but it's not okay
>> for it to happen in a minor release of a stable branch.
> I understand, but I am not sure that I agree. I think that if an
> extension stops compiling against a back-branch, someone will notice
> the next time they try to compile it and will fix it. Maybe that's not
> amazing, but I don't think it's a huge deal either.
Well, perhaps it's not the end of the world, but it's still a large
PITA for the maintainer of such an extension. They can't "just fix it"
because some percentage of their userbase will still need to compile
against older minor releases. Nor have you provided any way to handle
that requirement via conditional compilation.
regards, tom lane
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-07 06:28 Michael Paquier <michael@paquier.xyz>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 23+ messages in thread
From: Michael Paquier @ 2022-04-07 06:28 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Tue, Apr 05, 2022 at 03:16:20PM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Apr 5, 2022 at 10:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> My point is that we want that to happen in HEAD, but it's not okay
>>> for it to happen in a minor release of a stable branch.
>
>> I understand, but I am not sure that I agree. I think that if an
>> extension stops compiling against a back-branch, someone will notice
>> the next time they try to compile it and will fix it. Maybe that's not
>> amazing, but I don't think it's a huge deal either.
I agree with Tom's argument. The internals of this structure should
not have changed in a stable branch.
> Well, perhaps it's not the end of the world, but it's still a large
> PITA for the maintainer of such an extension. They can't "just fix it"
> because some percentage of their userbase will still need to compile
> against older minor releases. Nor have you provided any way to handle
> that requirement via conditional compilation.
For example, I recall that some external extensions make use of
sizeof(PGPROC) for their own business. Isn't 412ad7a going to be a
problem to change this structure's internals for already-compiled code
on stable branches?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-07 14:04 Robert Haas <robertmhaas@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 23+ messages in thread
From: Robert Haas @ 2022-04-07 14:04 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Thu, Apr 7, 2022 at 2:28 AM Michael Paquier <michael@paquier.xyz> wrote:
> > Well, perhaps it's not the end of the world, but it's still a large
> > PITA for the maintainer of such an extension. They can't "just fix it"
> > because some percentage of their userbase will still need to compile
> > against older minor releases. Nor have you provided any way to handle
> > that requirement via conditional compilation.
>
> For example, I recall that some external extensions make use of
> sizeof(PGPROC) for their own business. Isn't 412ad7a going to be a
> problem to change this structure's internals for already-compiled code
> on stable branches?
I don't think that commit changed sizeof(PGPROC), but it did affect
the position of the delayChkpt and statusFlags members within the
struct, which is what we now need to fix. Since I don't hear anyone
else volunteering to take care of that, I'll go work on it.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-07 15:19 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 3 replies; 23+ messages in thread
From: Robert Haas @ 2022-04-07 15:19 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Tue, Apr 5, 2022 at 10:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> What I think you need to do is:
>
> 1. In the back branches, revert delayChkpt to its previous type and
> semantics. Squeeze a separate delayChkptEnd bool in somewhere
> (you can't change the struct size either ...).
>
> 2. In HEAD, rename the field to something like delayChkptFlags,
> to ensure that any code touching it has to be inspected and updated.
Here are patches for master and v14 to do things this way. Comments?
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments: download all (2 files, .tar.gz)
0001-Rename-delayChkpt-to-delayChkptFlags.patch (application/octet-stream, 15.9K) download
From eb4a3be2735f19d700c9687b2ec28c3587b576c1 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 7 Apr 2022 10:24:26 -0400
Subject: [PATCH] Rename delayChkpt to delayChkptFlags.
Before commit 412ad7a55639516f284cd0ef9757d6ae5c7abd43, delayChkpt
was a Boolean. Now it's an integer. Extensions using it need to be
appropriately updated, so let's rename the field to make sure that
a hard compilation failure occurs.
Replacing delayChkpt with delayChkptFlags made a few comments extend
past 80 characters, so I reflowed them and changed some wording very
slightly.
Per suggestion from Tom Lane.
---
src/backend/access/transam/multixact.c | 6 ++---
src/backend/access/transam/twophase.c | 28 ++++++++++++------------
src/backend/access/transam/xact.c | 14 ++++++------
src/backend/access/transam/xlog.c | 10 ++++-----
src/backend/access/transam/xloginsert.c | 2 +-
src/backend/catalog/storage.c | 6 ++---
src/backend/storage/buffer/bufmgr.c | 12 +++++-----
src/backend/storage/ipc/procarray.c | 29 +++++++++++++------------
src/backend/storage/lmgr/proc.c | 4 ++--
src/include/storage/proc.h | 2 +-
10 files changed, 57 insertions(+), 56 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 9f65c600d0..45907d1b44 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -3088,8 +3088,8 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
* crash/basebackup, even though the state of the data directory would
* require it.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ Assert((MyProc->delayChkptFlags & DELAY_CHKPT_START) == 0);
+ MyProc->delayChkptFlags |= DELAY_CHKPT_START;
/* WAL log truncation */
WriteMTruncateXlogRec(newOldestMultiDB,
@@ -3115,7 +3115,7 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
/* Then offsets */
PerformOffsetsTruncation(oldestMulti, newOldestMulti);
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkptFlags &= ~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 b35da6f1aa..7632596008 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -479,7 +479,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
}
proc->xid = xid;
Assert(proc->xmin == InvalidTransactionId);
- proc->delayChkpt = 0;
+ proc->delayChkptFlags = 0;
proc->statusFlags = 0;
proc->pid = 0;
proc->databaseId = databaseid;
@@ -1173,11 +1173,11 @@ EndPrepare(GlobalTransaction gxact)
* Now writing 2PC state data to WAL. We let the WAL's CRC protection
* cover us, so no need to calculate a separate CRC.
*
- * We have to set delayChkpt here, too; otherwise a checkpoint starting
- * immediately after the WAL record is inserted could complete without
- * fsync'ing our state file. (This is essentially the same kind of race
- * condition as the COMMIT-to-clog-write case that RecordTransactionCommit
- * uses delayChkpt for; see notes there.)
+ * We have to set DELAY_CHKPT_START here, too; otherwise a checkpoint
+ * starting immediately after the WAL record is inserted could complete
+ * without fsync'ing our state file. (This is essentially the same kind
+ * of race condition as the COMMIT-to-clog-write case that
+ * RecordTransactionCommit uses DELAY_CHKPT_START for; see notes there.)
*
* We save the PREPARE record's location in the gxact for later use by
* CheckPointTwoPhase.
@@ -1186,8 +1186,8 @@ EndPrepare(GlobalTransaction gxact)
START_CRIT_SECTION();
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ Assert((MyProc->delayChkptFlags & DELAY_CHKPT_START) == 0);
+ MyProc->delayChkptFlags |= DELAY_CHKPT_START;
XLogBeginInsert();
for (record = records.head; record != NULL; record = record->next)
@@ -1230,7 +1230,7 @@ EndPrepare(GlobalTransaction gxact)
* checkpoint starting after this will certainly see the gxact as a
* candidate for fsyncing.
*/
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkptFlags &= ~DELAY_CHKPT_START;
/*
* Remember that we have this GlobalTransaction entry locked for us. If
@@ -1817,7 +1817,7 @@ CheckPointTwoPhase(XLogRecPtr redo_horizon)
*
* Note that it isn't possible for there to be a GXACT with a
* prepare_end_lsn set prior to the last checkpoint yet is marked invalid,
- * because of the efforts with delayChkpt.
+ * because of the efforts with delayChkptFlags.
*/
LWLockAcquire(TwoPhaseStateLock, LW_SHARED);
for (i = 0; i < TwoPhaseState->numPrepXacts; i++)
@@ -2275,7 +2275,7 @@ ProcessTwoPhaseBuffer(TransactionId xid,
* RecordTransactionCommitPrepared
*
* This is basically the same as RecordTransactionCommit (q.v. if you change
- * this function): in particular, we must set the delayChkpt flag to avoid a
+ * this function): in particular, we must set DELAY_CHKPT_START to avoid a
* race condition.
*
* We know the transaction made at least one XLOG entry (its PREPARE),
@@ -2308,8 +2308,8 @@ RecordTransactionCommitPrepared(TransactionId xid,
START_CRIT_SECTION();
/* See notes in RecordTransactionCommit */
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ Assert((MyProc->delayChkptFlags & DELAY_CHKPT_START) == 0);
+ MyProc->delayChkptFlags |= DELAY_CHKPT_START;
/*
* Emit the XLOG commit record. Note that we mark 2PC commits as
@@ -2358,7 +2358,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
TransactionIdCommitTree(xid, nchildren, children);
/* Checkpoint can proceed now */
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkptFlags &= ~DELAY_CHKPT_START;
END_CRIT_SECTION();
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index bf2fc08d94..53f3e7fd1a 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1387,14 +1387,14 @@ RecordTransactionCommit(void)
* RecordTransactionAbort. That's because loss of a transaction abort
* is noncritical; the presumption would be that it aborted, anyway.
*
- * It's safe to change the delayChkpt flag of our own backend without
- * holding the ProcArrayLock, since we're the only one modifying it.
- * This makes checkpoint's determination of which xacts are delayChkpt
- * a bit fuzzy, but it doesn't matter.
+ * It's safe to change the delayChkptFlags flag of our own backend
+ * without holding the ProcArrayLock, since we're the only one
+ * modifying it. This makes checkpoint's determination of which xacts
+ * are delaying the checkpoint a bit fuzzy, but it doesn't matter.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
+ Assert((MyProc->delayChkptFlags & DELAY_CHKPT_START) == 0);
START_CRIT_SECTION();
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ MyProc->delayChkptFlags |= DELAY_CHKPT_START;
SetCurrentTransactionStopTimestamp();
@@ -1496,7 +1496,7 @@ RecordTransactionCommit(void)
*/
if (markXidCommitted)
{
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkptFlags &= ~DELAY_CHKPT_START;
END_CRIT_SECTION();
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6770c3ddba..a7814d4019 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6505,11 +6505,11 @@ CreateCheckPoint(int flags)
* protected by different locks, but again that seems best on grounds of
* minimizing lock contention.)
*
- * A transaction that has not yet set delayChkpt when we look cannot be at
- * risk, since he's not inserted his commit record yet; and one that's
- * already cleared it is not at risk either, since he's done fixing clog
- * and we will correctly flush the update below. So we cannot miss any
- * xacts we need to wait for.
+ * A transaction that has not yet set delayChkptFlags when we look cannot
+ * be at risk, since it has not inserted its commit record yet; and one
+ * that's already cleared it is not at risk either, since it's done fixing
+ * clog 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);
if (nvxids > 0)
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 462e23503e..2ce9be2cc7 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -1011,7 +1011,7 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std)
/*
* Ensure no checkpoint can change our view of RedoRecPtr.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) != 0);
+ Assert((MyProc->delayChkptFlags & 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 9898701a43..e4d000d4fe 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -348,8 +348,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((MyProc->delayChkpt & DELAY_CHKPT_COMPLETE) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_COMPLETE;
+ Assert((MyProc->delayChkptFlags & DELAY_CHKPT_COMPLETE) == 0);
+ MyProc->delayChkptFlags |= DELAY_CHKPT_COMPLETE;
/*
* We WAL-log the truncation before actually truncating, which means
@@ -397,7 +397,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks);
/* We've done all the critical work, so checkpoints are OK now. */
- MyProc->delayChkpt &= ~DELAY_CHKPT_COMPLETE;
+ MyProc->delayChkptFlags &= ~DELAY_CHKPT_COMPLETE;
/*
* Update upper-level FSM pages to account for the truncation. This is
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 93c1ea2d9f..c12028ca0f 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4021,7 +4021,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
{
XLogRecPtr lsn = InvalidXLogRecPtr;
bool dirtied = false;
- bool delayChkpt = false;
+ bool delayChkptFlags = false;
uint32 buf_state;
/*
@@ -4071,9 +4071,9 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
* essential that CreateCheckPoint waits for virtual transactions
* rather than full transactionids.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
- delayChkpt = true;
+ Assert((MyProc->delayChkptFlags & DELAY_CHKPT_START) == 0);
+ MyProc->delayChkptFlags |= DELAY_CHKPT_START;
+ delayChkptFlags = true;
lsn = XLogSaveBufferForHint(buffer, buffer_std);
}
@@ -4105,8 +4105,8 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
buf_state |= BM_DIRTY | BM_JUST_DIRTIED;
UnlockBufHdr(bufHdr, buf_state);
- if (delayChkpt)
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ if (delayChkptFlags)
+ MyProc->delayChkptFlags &= ~DELAY_CHKPT_START;
if (dirtied)
{
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 735763cc24..603f6aba71 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -700,7 +700,7 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
proc->xmin = InvalidTransactionId;
/* be sure this is cleared in abort */
- proc->delayChkpt = 0;
+ proc->delayChkptFlags = 0;
proc->recoveryConflictPending = false;
@@ -742,7 +742,7 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid)
proc->xmin = InvalidTransactionId;
/* be sure this is cleared in abort */
- proc->delayChkpt = 0;
+ proc->delayChkptFlags = 0;
proc->recoveryConflictPending = false;
@@ -929,7 +929,7 @@ ProcArrayClearTransaction(PGPROC *proc)
proc->recoveryConflictPending = false;
Assert(!(proc->statusFlags & PROC_VACUUM_STATE_MASK));
- Assert(!proc->delayChkpt);
+ Assert(!proc->delayChkptFlags);
/*
* Need to increment completion count even though transaction hasn't
@@ -3059,19 +3059,20 @@ 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 specified delayChkpt bits set in their
- * PGPROC.
+ * critical sections, as shown by having specified delayChkptFlags bits set
+ * in their PGPROC.
*
* 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 delayChkptFlags 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 delayChkptFlags will propagate to shared memory
+ * when the backend takes a lock, so we cannot fail to see a virtual xact as
+ * delayChkptFlags if it's already inserted its commit record. Whether it
+ * takes a little while for clearing of delayChkptFlags to propagate is
+ * unimportant for correctness.
*/
VirtualTransactionId *
GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
@@ -3094,7 +3095,7 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
int pgprocno = arrayP->pgprocnos[index];
PGPROC *proc = &allProcs[pgprocno];
- if ((proc->delayChkpt & type) != 0)
+ if ((proc->delayChkptFlags & type) != 0)
{
VirtualTransactionId vxid;
@@ -3138,7 +3139,7 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
GET_VXID_FROM_PGPROC(vxid, *proc);
- if ((proc->delayChkpt & type) != 0 &&
+ if ((proc->delayChkptFlags & type) != 0 &&
VirtualTransactionIdIsValid(vxid))
{
int i;
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index df080cd332..93d082c45e 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -393,7 +393,7 @@ InitProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyProc->delayChkpt = 0;
+ MyProc->delayChkptFlags = 0;
MyProc->statusFlags = 0;
/* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
if (IsAutoVacuumWorkerProcess())
@@ -578,7 +578,7 @@ InitAuxiliaryProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyProc->delayChkpt = 0;
+ MyProc->delayChkptFlags = 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 36ecf7d005..21bef4c6dc 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -226,7 +226,7 @@ struct PGPROC
pg_atomic_uint64 waitStart; /* time at which wait for lock acquisition
* started */
- int delayChkpt; /* for DELAY_CHKPT_* flags */
+ int delayChkptFlags; /* for DELAY_CHKPT_* flags */
uint8 statusFlags; /* this backend's status flags, see PROC_*
* above. mirrored in
--
2.24.3 (Apple Git-128)
0001-Rethink-the-delay-checkpoint-end-mechanism-in-the-ba.patch (application/octet-stream, 16.5K) download
From b2f67581fb55bea5bbf740208394ba6a8346fd19 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 7 Apr 2022 11:15:07 -0400
Subject: [PATCH] Rethink the delay-checkpoint-end mechanism in the
back-branches.
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.
---
src/backend/access/transam/multixact.c | 6 +-
src/backend/access/transam/twophase.c | 13 ++-
src/backend/access/transam/xact.c | 6 +-
src/backend/access/transam/xlog.c | 10 +--
src/backend/access/transam/xloginsert.c | 2 +-
src/backend/catalog/storage.c | 6 +-
src/backend/storage/buffer/bufmgr.c | 6 +-
src/backend/storage/ipc/procarray.c | 104 +++++++++++++++++++++---
src/include/storage/proc.h | 38 +--------
src/include/storage/procarray.h | 7 +-
10 files changed, 121 insertions(+), 77 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 50d8bab9e2..b643564f16 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -3075,8 +3075,8 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
* crash/basebackup, even though the state of the data directory would
* require it.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ Assert(!MyProc->delayChkpt);
+ MyProc->delayChkpt = true;
/* WAL log truncation */
WriteMTruncateXlogRec(newOldestMultiDB,
@@ -3102,7 +3102,7 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
/* Then offsets */
PerformOffsetsTruncation(oldestMulti, newOldestMulti);
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
LWLockRelease(MultiXactTruncationLock);
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index dea3f485f7..633a6f1747 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -474,8 +474,9 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
}
proc->xid = xid;
Assert(proc->xmin == InvalidTransactionId);
- proc->delayChkpt = 0;
+ proc->delayChkpt = false;
proc->statusFlags = 0;
+ proc->delayChkptEnd = false;
proc->pid = 0;
proc->databaseId = databaseid;
proc->roleId = owner;
@@ -1165,8 +1166,7 @@ EndPrepare(GlobalTransaction gxact)
START_CRIT_SECTION();
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
XLogBeginInsert();
for (record = records.head; record != NULL; record = record->next)
@@ -1209,7 +1209,7 @@ EndPrepare(GlobalTransaction gxact)
* checkpoint starting after this will certainly see the gxact as a
* candidate for fsyncing.
*/
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
/*
* Remember that we have this GlobalTransaction entry locked for us. If
@@ -2276,8 +2276,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
START_CRIT_SECTION();
/* See notes in RecordTransactionCommit */
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
/*
* Emit the XLOG commit record. Note that we mark 2PC commits as
@@ -2325,7 +2324,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
TransactionIdCommitTree(xid, nchildren, children);
/* Checkpoint can proceed now */
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index c5e7261921..e8523693c0 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1335,9 +1335,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);
+ Assert(!MyProc->delayChkpt);
START_CRIT_SECTION();
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
SetCurrentTransactionStopTimestamp();
@@ -1438,7 +1438,7 @@ RecordTransactionCommit(void)
*/
if (markXidCommitted)
{
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d889d69387..8edf0fa646 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9228,27 +9228,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);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 1af4a90c41..b153fad594 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -925,7 +925,7 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std)
/*
* Ensure no checkpoint can change our view of RedoRecPtr.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) != 0);
+ Assert(MyProc->delayChkpt);
/*
* 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 fa5682dce8..b7fc491a68 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -338,8 +338,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((MyProc->delayChkpt & DELAY_CHKPT_COMPLETE) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_COMPLETE;
+ Assert(!MyProc->delayChkptEnd);
+ MyProc->delayChkptEnd = true;
/*
* We WAL-log the truncation before actually truncating, which means
@@ -387,7 +387,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
smgrtruncate(rel->rd_smgr, forks, nforks, blocks);
/* We've done all the critical work, so checkpoints are OK now. */
- MyProc->delayChkpt &= ~DELAY_CHKPT_COMPLETE;
+ MyProc->delayChkptEnd = false;
/*
* Update upper-level FSM pages to account for the truncation. This is
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index a55545a187..49561ae794 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3946,8 +3946,8 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
* essential that CreateCheckpoint waits for virtual transactions
* rather than full transactionids.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ Assert(!MyProc->delayChkpt);
+ MyProc->delayChkpt = true;
delayChkpt = true;
lsn = XLogSaveBufferForHint(buffer, buffer_std);
}
@@ -3981,7 +3981,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
UnlockBufHdr(bufHdr, buf_state);
if (delayChkpt)
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
if (dirtied)
{
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index ae71d7538b..0ea74f2a3c 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -690,8 +690,9 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
proc->lxid = InvalidLocalTransactionId;
proc->xmin = InvalidTransactionId;
- /* be sure this is cleared in abort */
- proc->delayChkpt = 0;
+ /* be sure these are cleared in abort */
+ proc->delayChkpt = false;
+ proc->delayChkptEnd = false;
proc->recoveryConflictPending = false;
@@ -732,8 +733,9 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid)
proc->lxid = InvalidLocalTransactionId;
proc->xmin = InvalidTransactionId;
- /* be sure this is cleared in abort */
- proc->delayChkpt = 0;
+ /* be sure these are cleared in abort */
+ proc->delayChkpt = false;
+ proc->delayChkptEnd = false;
proc->recoveryConflictPending = false;
@@ -3049,8 +3051,7 @@ 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 specified delayChkpt bits set in their
- * PGPROC.
+ * critical sections, as shown by having delayChkpt set in their PGPROC.
*
* Returns a palloc'd array that should be freed by the caller.
* *nvxids is the number of valid entries.
@@ -3064,14 +3065,51 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly)
* for clearing of delayChkpt to propagate is unimportant for correctness.
*/
VirtualTransactionId *
-GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
+GetVirtualXIDsDelayingChkpt(int *nvxids)
{
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);
+
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+
+ for (index = 0; index < arrayP->numProcs; index++)
+ {
+ int pgprocno = arrayP->pgprocnos[index];
+ PGPROC *proc = &allProcs[pgprocno];
+
+ if (proc->delayChkpt)
+ {
+ VirtualTransactionId vxid;
+
+ GET_VXID_FROM_PGPROC(vxid, *proc);
+ if (VirtualTransactionIdIsValid(vxid))
+ vxids[count++] = vxid;
+ }
+ }
+
+ LWLockRelease(ProcArrayLock);
+
+ *nvxids = count;
+ return vxids;
+}
+
+/*
+ * GetVirtualXIDsDelayingChkptEnd - like GetVirtualXIDsDelayingChkpt,
+ * but for delayChkptEnd
+ */
+VirtualTransactionId *
+GetVirtualXIDsDelayingChkptEnd(int *nvxids)
+{
+ VirtualTransactionId *vxids;
+ ProcArrayStruct *arrayP = procArray;
+ int count = 0;
+ int index;
/* allocate what's certainly enough result space */
vxids = (VirtualTransactionId *)
@@ -3084,7 +3122,7 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
int pgprocno = arrayP->pgprocnos[index];
PGPROC *proc = &allProcs[pgprocno];
- if ((proc->delayChkpt & type) != 0)
+ if (proc->delayChkptEnd)
{
VirtualTransactionId vxid;
@@ -3110,13 +3148,54 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
* those numbers should be small enough for it not to be a problem.
*/
bool
-HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
+HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids)
{
bool result = false;
ProcArrayStruct *arrayP = procArray;
int index;
- Assert(type != 0);
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+
+ for (index = 0; index < arrayP->numProcs; index++)
+ {
+ int pgprocno = arrayP->pgprocnos[index];
+ PGPROC *proc = &allProcs[pgprocno];
+ VirtualTransactionId vxid;
+
+ GET_VXID_FROM_PGPROC(vxid, *proc);
+
+ if (proc->delayChkpt && VirtualTransactionIdIsValid(vxid))
+ {
+ int i;
+
+ for (i = 0; i < nvxids; i++)
+ {
+ if (VirtualTransactionIdEquals(vxid, vxids[i]))
+ {
+ result = true;
+ break;
+ }
+ }
+ if (result)
+ break;
+ }
+ }
+
+ LWLockRelease(ProcArrayLock);
+
+ return result;
+}
+
+/*
+ * HaveVirtualXIDsDelayingChkptEnd -- like HaveVirtualXIDsDelayingChkpt
+ * but for delayChkptEnd
+ */
+bool
+HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids, int nvxids)
+{
+ bool result = false;
+ ProcArrayStruct *arrayP = procArray;
+ int index;
LWLockAcquire(ProcArrayLock, LW_SHARED);
@@ -3128,8 +3207,7 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
GET_VXID_FROM_PGPROC(vxid, *proc);
- if ((proc->delayChkpt & type) != 0 &&
- VirtualTransactionIdIsValid(vxid))
+ if (proc->delayChkptEnd && VirtualTransactionIdIsValid(vxid))
{
int i;
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index b78012ec2b..0ea8f7a785 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -86,41 +86,6 @@ 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.
- */
-#define DELAY_CHKPT_START (1<<0)
-#define DELAY_CHKPT_COMPLETE (1<<1)
-
typedef enum
{
PROC_WAIT_STATUS_OK,
@@ -226,11 +191,12 @@ struct PGPROC
pg_atomic_uint64 waitStart; /* time at which wait for lock acquisition
* started */
- int delayChkpt; /* for DELAY_CHKPT_* flags */
+ bool delayChkpt; /* true if this proc delays checkpoint start */
uint8 statusFlags; /* this backend's status flags, see PROC_*
* above. mirrored in
* ProcGlobal->statusFlags[pgxactoff] */
+ bool delayChkptEnd; /* true if this proc delays checkpoint end */
/*
* Info to allow us to wait for synchronous replication, if needed.
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index 93de230a32..f5d7d41098 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -59,9 +59,12 @@ extern TransactionId GetOldestActiveTransactionId(void);
extern TransactionId GetOldestSafeDecodingTransactionId(bool catalogOnly);
extern void GetReplicationHorizons(TransactionId *slot_xmin, TransactionId *catalog_xmin);
-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);
--
2.24.3 (Apple Git-128)
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-07 15:51 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
2 siblings, 0 replies; 23+ messages in thread
From: Tom Lane @ 2022-04-07 15:51 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Robert Haas <robertmhaas@gmail.com> writes:
> Here are patches for master and v14 to do things this way. Comments?
WFM.
regards, tom lane
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-07 18:07 Petr Jelinek <petr.jelinek@enterprisedb.com>
parent: Robert Haas <robertmhaas@gmail.com>
2 siblings, 0 replies; 23+ messages in thread
From: Petr Jelinek @ 2022-04-07 18:07 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> On 7. 4. 2022, at 17:19, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Apr 5, 2022 at 10:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What I think you need to do is:
>>
>> 1. In the back branches, revert delayChkpt to its previous type and
>> semantics. Squeeze a separate delayChkptEnd bool in somewhere
>> (you can't change the struct size either ...).
>>
>> 2. In HEAD, rename the field to something like delayChkptFlags,
>> to ensure that any code touching it has to be inspected and updated.
>
> Here are patches for master and v14 to do things this way. Comments?
Yeah I think this should do it (compilers should warn on master even without the rename, but who notices that right? :) )
Thanks,
Petr
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-07 23:47 Michael Paquier <michael@paquier.xyz>
parent: Robert Haas <robertmhaas@gmail.com>
2 siblings, 2 replies; 23+ messages in thread
From: Michael Paquier @ 2022-04-07 23:47 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Thu, Apr 07, 2022 at 11:19:15AM -0400, Robert Haas wrote:
> Here are patches for master and v14 to do things this way. Comments?
Thanks for the patches. They look correct. For ~14, I'd rather avoid
the code duplication done by GetVirtualXIDsDelayingChkptEnd() and
HaveVirtualXIDsDelayingChkpt() that could be avoided with an extra
bool argument to the existing routine. The same kind of duplication
happens with GetVirtualXIDsDelayingChkpt() and
GetVirtualXIDsDelayingChkptEnd().
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-07 23:52 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 23+ messages in thread
From: Tom Lane @ 2022-04-07 23:52 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Robert Haas <robertmhaas@gmail.com>; Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Apr 07, 2022 at 11:19:15AM -0400, Robert Haas wrote:
>> Here are patches for master and v14 to do things this way. Comments?
> Thanks for the patches. They look correct. For ~14, I'd rather avoid
> the code duplication done by GetVirtualXIDsDelayingChkptEnd() and
> HaveVirtualXIDsDelayingChkpt() that could be avoided with an extra
> bool argument to the existing routine.
Isn't adding another argument an API break? (If there's any outside
code calling GetVirtualXIDsDelayingChkpt, which it seems like there
might be.)
regards, tom lane
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-08 02:19 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 23+ messages in thread
From: Robert Haas @ 2022-04-08 02:19 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Thu, Apr 7, 2022 at 7:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > On Thu, Apr 07, 2022 at 11:19:15AM -0400, Robert Haas wrote:
> >> Here are patches for master and v14 to do things this way. Comments?
>
> > Thanks for the patches. They look correct. For ~14, I'd rather avoid
> > the code duplication done by GetVirtualXIDsDelayingChkptEnd() and
> > HaveVirtualXIDsDelayingChkpt() that could be avoided with an extra
> > bool argument to the existing routine.
>
> Isn't adding another argument an API break? (If there's any outside
> code calling GetVirtualXIDsDelayingChkpt, which it seems like there
> might be.)
Yeah, that's exactly why I didn't do what Michael proposes. If we're
going to go to this trouble to avoid changing the layout of a PGPROC,
we must be doing that on the theory that extension code cares about
delayChkpt. And if that is so, it seems reasonable to suppose that it
might also want to call the associated functions.
Honestly, I wouldn't have thought that this mattered, because I
wouldn't have guessed that any non-core code cared about delayChkpt.
But I would have been wrong.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-08 07:13 Michael Paquier <michael@paquier.xyz>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 0 replies; 23+ messages in thread
From: Michael Paquier @ 2022-04-08 07:13 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Markus Wanner <markus.wanner@enterprisedb.com>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Thu, Apr 07, 2022 at 10:19:35PM -0400, Robert Haas wrote:
> Yeah, that's exactly why I didn't do what Michael proposes. If we're
> going to go to this trouble to avoid changing the layout of a PGPROC,
> we must be doing that on the theory that extension code cares about
> delayChkpt. And if that is so, it seems reasonable to suppose that it
> might also want to call the associated functions.
Compatibility does not strike me as a problem with two static inline
functions used as wrappers of their common logic.
> Honestly, I wouldn't have thought that this mattered, because I
> wouldn't have guessed that any non-core code cared about delayChkpt.
> But I would have been wrong.
That's a minor point. If you wish to keep this code as you are
proposing, that's fine as well by me.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-08 08:47 Markus Wanner <markus.wanner@enterprisedb.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 23+ messages in thread
From: Markus Wanner @ 2022-04-08 08:47 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Fri, 2022-04-08 at 08:47 +0900, Michael Paquier wrote:
> On Thu, Apr 07, 2022 at 11:19:15AM -0400, Robert Haas wrote:
> > Here are patches for master and v14 to do things this way.
> > Comments?
>
> Thanks for the patches. They look correct.
+1, looks good to me and addresses my specific original concern.
> For ~14, I'd rather avoid
> the code duplication done by GetVirtualXIDsDelayingChkptEnd() and
> HaveVirtualXIDsDelayingChkpt() that could be avoided with an extra
> bool argument to the existing routine. The same kind of duplication
> happens with GetVirtualXIDsDelayingChkpt() and
> GetVirtualXIDsDelayingChkptEnd().
I agree with Michael, it would be nice to not duplicate the code, but
use a common underlying method. A modified patch is attached.
Best Regards
Markus
Attachments:
0001-Rethink-the-delay-v2.patch (text/x-patch, 18.4K) download
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 7 Apr 2022 11:15:07 -0400
Subject: [PATCH] Rethink the delay-checkpoint-end mechanism in the
back-branches.
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.
---
backend/access/transam/multixact.c | 6 +-
backend/access/transam/twophase.c | 13 ++--
backend/access/transam/xact.c | 6 +-
backend/access/transam/xlog.c | 10 +--
backend/access/transam/xloginsert.c | 2
backend/catalog/storage.c | 6 +-
backend/storage/buffer/bufmgr.c | 6 +-
backend/storage/ipc/procarray.c | 100 +++++++++++++++++++++++++++---------
include/storage/proc.h | 35 +-----------
include/storage/procarray.h | 7 +-
10 files changed, 107 insertions(+), 84 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 50d8bab9e21..b643564f16a 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -3075,8 +3075,8 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
* crash/basebackup, even though the state of the data directory would
* require it.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ Assert(!MyProc->delayChkpt);
+ MyProc->delayChkpt = true;
/* WAL log truncation */
WriteMTruncateXlogRec(newOldestMultiDB,
@@ -3102,7 +3102,7 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
/* Then offsets */
PerformOffsetsTruncation(oldestMulti, newOldestMulti);
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
LWLockRelease(MultiXactTruncationLock);
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index dea3f485f7a..633a6f1747f 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -474,8 +474,9 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
}
proc->xid = xid;
Assert(proc->xmin == InvalidTransactionId);
- proc->delayChkpt = 0;
+ proc->delayChkpt = false;
proc->statusFlags = 0;
+ proc->delayChkptEnd = false;
proc->pid = 0;
proc->databaseId = databaseid;
proc->roleId = owner;
@@ -1165,8 +1166,7 @@ EndPrepare(GlobalTransaction gxact)
START_CRIT_SECTION();
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
XLogBeginInsert();
for (record = records.head; record != NULL; record = record->next)
@@ -1209,7 +1209,7 @@ EndPrepare(GlobalTransaction gxact)
* checkpoint starting after this will certainly see the gxact as a
* candidate for fsyncing.
*/
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
/*
* Remember that we have this GlobalTransaction entry locked for us. If
@@ -2276,8 +2276,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
START_CRIT_SECTION();
/* See notes in RecordTransactionCommit */
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
/*
* Emit the XLOG commit record. Note that we mark 2PC commits as
@@ -2325,7 +2324,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
TransactionIdCommitTree(xid, nchildren, children);
/* Checkpoint can proceed now */
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index c5e7261921d..e8523693c0a 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1335,9 +1335,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);
+ Assert(!MyProc->delayChkpt);
START_CRIT_SECTION();
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
SetCurrentTransactionStopTimestamp();
@@ -1438,7 +1438,7 @@ RecordTransactionCommit(void)
*/
if (markXidCommitted)
{
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d889d69387e..8edf0fa6461 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9228,27 +9228,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);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 1af4a90c419..b153fad594d 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -925,7 +925,7 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std)
/*
* Ensure no checkpoint can change our view of RedoRecPtr.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) != 0);
+ Assert(MyProc->delayChkpt);
/*
* 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 fa5682dce80..b7fc491a684 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -338,8 +338,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((MyProc->delayChkpt & DELAY_CHKPT_COMPLETE) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_COMPLETE;
+ Assert(!MyProc->delayChkptEnd);
+ MyProc->delayChkptEnd = true;
/*
* We WAL-log the truncation before actually truncating, which means
@@ -387,7 +387,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
smgrtruncate(rel->rd_smgr, forks, nforks, blocks);
/* We've done all the critical work, so checkpoints are OK now. */
- MyProc->delayChkpt &= ~DELAY_CHKPT_COMPLETE;
+ MyProc->delayChkptEnd = false;
/*
* Update upper-level FSM pages to account for the truncation. This is
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index a55545a1875..49561ae794d 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3946,8 +3946,8 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
* essential that CreateCheckpoint waits for virtual transactions
* rather than full transactionids.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ Assert(!MyProc->delayChkpt);
+ MyProc->delayChkpt = true;
delayChkpt = true;
lsn = XLogSaveBufferForHint(buffer, buffer_std);
}
@@ -3981,7 +3981,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
UnlockBufHdr(bufHdr, buf_state);
if (delayChkpt)
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
if (dirtied)
{
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index ae71d7538be..54fd8615ad9 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -330,6 +330,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,
@@ -690,8 +695,9 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
proc->lxid = InvalidLocalTransactionId;
proc->xmin = InvalidTransactionId;
- /* be sure this is cleared in abort */
- proc->delayChkpt = 0;
+ /* be sure these are cleared in abort */
+ proc->delayChkpt = false;
+ proc->delayChkptEnd = false;
proc->recoveryConflictPending = false;
@@ -732,8 +738,9 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid)
proc->lxid = InvalidLocalTransactionId;
proc->xmin = InvalidTransactionId;
- /* be sure this is cleared in abort */
- proc->delayChkpt = 0;
+ /* be sure these are cleared in abort */
+ proc->delayChkpt = false;
+ proc->delayChkptEnd = false;
proc->recoveryConflictPending = false;
@@ -3045,34 +3052,34 @@ 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
- * PGPROC.
+ * critical sections, as shown by having delayChkpt or delayChkptEnd set in
+ * their PGPROC.
*
* 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;
int count = 0;
int index;
- Assert(type != 0);
-
/* allocate what's certainly enough result space */
vxids = (VirtualTransactionId *)
palloc(sizeof(VirtualTransactionId) * arrayP->maxProcs);
@@ -3084,7 +3091,8 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
int pgprocno = arrayP->pgprocnos[index];
PGPROC *proc = &allProcs[pgprocno];
- if ((proc->delayChkpt & type) != 0)
+ if (((type & DELAY_CHKPT_START) && proc->delayChkpt) ||
+ ((type & DELAY_CHKPT_COMPLETE) && proc->delayChkptEnd))
{
VirtualTransactionId vxid;
@@ -3100,6 +3108,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?
*
@@ -3109,15 +3137,14 @@ 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;
int index;
- Assert(type != 0);
-
LWLockAcquire(ProcArrayLock, LW_SHARED);
for (index = 0; index < arrayP->numProcs; index++)
@@ -3128,7 +3155,8 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
GET_VXID_FROM_PGPROC(vxid, *proc);
- if ((proc->delayChkpt & type) != 0 &&
+ if ((((type & DELAY_CHKPT_START) && proc->delayChkpt) ||
+ ((type & DELAY_CHKPT_COMPLETE) && proc->delayChkptEnd)) &&
VirtualTransactionIdIsValid(vxid))
{
int i;
@@ -3151,6 +3179,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
*
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index b78012ec2b6..11b514c9ae8 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -87,36 +87,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)
@@ -226,11 +198,12 @@ struct PGPROC
pg_atomic_uint64 waitStart; /* time at which wait for lock acquisition
* started */
- int delayChkpt; /* for DELAY_CHKPT_* flags */
+ bool delayChkpt; /* true if this proc delays checkpoint start */
uint8 statusFlags; /* this backend's status flags, see PROC_*
* above. mirrored in
* ProcGlobal->statusFlags[pgxactoff] */
+ bool delayChkptEnd; /* true if this proc delays checkpoint end */
/*
* Info to allow us to wait for synchronous replication, if needed.
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index 93de230a329..f5d7d410986 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -59,9 +59,12 @@ extern TransactionId GetOldestActiveTransactionId(void);
extern TransactionId GetOldestSafeDecodingTransactionId(bool catalogOnly);
extern void GetReplicationHorizons(TransactionId *slot_xmin, TransactionId *catalog_xmin);
-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);
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-08 15:50 Robert Haas <robertmhaas@gmail.com>
parent: Markus Wanner <markus.wanner@enterprisedb.com>
0 siblings, 1 reply; 23+ messages in thread
From: Robert Haas @ 2022-04-08 15:50 UTC (permalink / raw)
To: Markus Wanner <markus.wanner@enterprisedb.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Fri, Apr 8, 2022 at 4:47 AM Markus Wanner
<markus.wanner@enterprisedb.com> wrote:
> I agree with Michael, it would be nice to not duplicate the code, but
> use a common underlying method. A modified patch is attached.
I don't think this is better, but I don't think it's worth arguing
about, either, so I'll do it this way if nobody objects.
Meanwhile, I've committed the patch for master to master.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-11 19:21 Robert Haas <robertmhaas@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 23+ messages in thread
From: Robert Haas @ 2022-04-11 19:21 UTC (permalink / raw)
To: Markus Wanner <markus.wanner@enterprisedb.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Fri, Apr 8, 2022 at 11:50 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Apr 8, 2022 at 4:47 AM Markus Wanner
> <markus.wanner@enterprisedb.com> wrote:
> > I agree with Michael, it would be nice to not duplicate the code, but
> > use a common underlying method. A modified patch is attached.
>
> I don't think this is better, but I don't think it's worth arguing
> about, either, so I'll do it this way if nobody objects.
>
> Meanwhile, I've committed the patch for master to master.
Well, I've just realized that Kyotaro Horiguchi volunteered to fix
this on an email thread I did not see because of the way Gmail breaks
the thread if you change the subject line. And he developed a very
similar patch to what we have here. I'm going to use this one as the
basis for going forward because I've already studied it in detail and
it's less work for me to stick with what I know than to go study
something else. But, he also noticed something which we didn't notice
here, which is that before v13, the commit in question actually
changed the size of PGXACT, which is really quite bad -- it needs to
be 12 bytes for performance reasons. And there's no spare bytes
available, so I think we should follow one of the suggestions that he
had over in that email thread, and put delayChkptEnd in PGPROC even
though delayChkpt is in PGXACT.
Comments?
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-11 19:45 Markus Wanner <markus.wanner@enterprisedb.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 23+ messages in thread
From: Markus Wanner @ 2022-04-11 19:45 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers; Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Mon, 2022-04-11 at 15:21 -0400, Robert Haas wrote:
> ... before v13, the commit in question actually
> changed the size of PGXACT, which is really quite bad -- it needs to
> be 12 bytes for performance reasons. And there's no spare bytes
> available, so I think we should follow one of the suggestions that he
> had over in that email thread, and put delayChkptEnd in PGPROC even
> though delayChkpt is in PGXACT.
This makes sense to me. Kudos to Kyotaro for considering this.
At first read, this sounded like a trade-off between compatibility and
performance for PG 12 and older. But I realize leaving delayChkpt in
PGXACT and adding just delayChkptEnd to PGPROC is compatible and leaves
PGXACT at a size of 12 bytes. So this sounds like a good approach to
me.
Best Regards
Markus
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-12 10:54 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Markus Wanner <markus.wanner@enterprisedb.com>
0 siblings, 1 reply; 23+ messages in thread
From: Kyotaro Horiguchi @ 2022-04-12 10:54 UTC (permalink / raw)
To: markus.wanner@enterprisedb.com; +Cc: robertmhaas@gmail.com; michael@paquier.xyz; tgl@sss.pgh.pa.us; pgsql-hackers
(My mailer has been fixed.)
At Mon, 11 Apr 2022 21:45:59 +0200, Markus Wanner <markus.wanner@enterprisedb.com> wrote in
> On Mon, 2022-04-11 at 15:21 -0400, Robert Haas wrote:
> > ... before v13, the commit in question actually
> > changed the size of PGXACT, which is really quite bad -- it needs to
> > be 12 bytes for performance reasons. And there's no spare bytes
> > available, so I think we should follow one of the suggestions that he
> > had over in that email thread, and put delayChkptEnd in PGPROC even
> > though delayChkpt is in PGXACT.
>
> This makes sense to me. Kudos to Kyotaro for considering this.
>
> At first read, this sounded like a trade-off between compatibility and
> performance for PG 12 and older. But I realize leaving delayChkpt in
> PGXACT and adding just delayChkptEnd to PGPROC is compatible and leaves
> PGXACT at a size of 12 bytes. So this sounds like a good approach to
> me.
Thanks!
So, I created the patches for back-patching from 10 to 14. (With
fixed a silly bug of the v1-pg14 that HaveVirtualXIDsDelayingChkpt and
HaveVirtualXIDsDelayingChkptEnd are inverted..)
They revert delayChkpt-related changes made by the patch and add
delayChkptEnd stuff. I compared among every pair of the patches for
neighbouring versions, to make sure not making the same change in
different way and they have the same set of hunks.
This version takes the way sharing the common static function
(*ChkptGuts) between the functions *Chkpt() and *ChkptEnd().
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments: download all (5 files, .tar.gz)
v2-0001-Fix-ABI-API-break_14.patch (text/x-patch, 17.5K) download
From 471ca4aa38a92f837ed4e4aeda40648c3b0b0a9b Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Wed, 6 Apr 2022 15:47:50 +0900
Subject: [PATCH v2] Fix ABI/API break
Commit bbace5697d caused ABI break by changing the type and width of
PGPROC.delayChkpt and API break by changing the signature of some
public functions. Restore the member and function signature to
previous state and add delayChkptEnd to an existing gap in PGPROC
struct.
Backpatch to 10, all supported branches.
---
src/backend/access/transam/multixact.c | 6 +--
src/backend/access/transam/twophase.c | 13 +++---
src/backend/access/transam/xact.c | 5 +--
src/backend/access/transam/xlog.c | 10 ++---
src/backend/access/transam/xloginsert.c | 2 +-
src/backend/catalog/storage.c | 6 +--
src/backend/storage/buffer/bufmgr.c | 6 +--
src/backend/storage/ipc/procarray.c | 60 +++++++++++++++++++------
src/backend/storage/lmgr/proc.c | 6 ++-
src/include/storage/proc.h | 42 +++--------------
src/include/storage/procarray.h | 8 ++--
11 files changed, 82 insertions(+), 82 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 50d8bab9e2..b643564f16 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -3075,8 +3075,8 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
* crash/basebackup, even though the state of the data directory would
* require it.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ Assert(!MyProc->delayChkpt);
+ MyProc->delayChkpt = true;
/* WAL log truncation */
WriteMTruncateXlogRec(newOldestMultiDB,
@@ -3102,7 +3102,7 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
/* Then offsets */
PerformOffsetsTruncation(oldestMulti, newOldestMulti);
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
LWLockRelease(MultiXactTruncationLock);
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index dea3f485f7..911d93fbf4 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -474,7 +474,8 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
}
proc->xid = xid;
Assert(proc->xmin == InvalidTransactionId);
- proc->delayChkpt = 0;
+ proc->delayChkpt = false;
+ proc->delayChkptEnd = false;
proc->statusFlags = 0;
proc->pid = 0;
proc->databaseId = databaseid;
@@ -1165,8 +1166,7 @@ EndPrepare(GlobalTransaction gxact)
START_CRIT_SECTION();
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
XLogBeginInsert();
for (record = records.head; record != NULL; record = record->next)
@@ -1209,7 +1209,7 @@ EndPrepare(GlobalTransaction gxact)
* checkpoint starting after this will certainly see the gxact as a
* candidate for fsyncing.
*/
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
/*
* Remember that we have this GlobalTransaction entry locked for us. If
@@ -2276,8 +2276,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
START_CRIT_SECTION();
/* See notes in RecordTransactionCommit */
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
/*
* Emit the XLOG commit record. Note that we mark 2PC commits as
@@ -2325,7 +2324,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
TransactionIdCommitTree(xid, nchildren, children);
/* Checkpoint can proceed now */
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index c5e7261921..514044f3db 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1335,9 +1335,8 @@ 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 |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
SetCurrentTransactionStopTimestamp();
@@ -1438,7 +1437,7 @@ RecordTransactionCommit(void)
*/
if (markXidCommitted)
{
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d889d69387..8edf0fa646 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9228,27 +9228,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);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 1af4a90c41..b153fad594 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -925,7 +925,7 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std)
/*
* Ensure no checkpoint can change our view of RedoRecPtr.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) != 0);
+ Assert(MyProc->delayChkpt);
/*
* 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 fa5682dce8..b7fc491a68 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -338,8 +338,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((MyProc->delayChkpt & DELAY_CHKPT_COMPLETE) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_COMPLETE;
+ Assert(!MyProc->delayChkptEnd);
+ MyProc->delayChkptEnd = true;
/*
* We WAL-log the truncation before actually truncating, which means
@@ -387,7 +387,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
smgrtruncate(rel->rd_smgr, forks, nforks, blocks);
/* We've done all the critical work, so checkpoints are OK now. */
- MyProc->delayChkpt &= ~DELAY_CHKPT_COMPLETE;
+ MyProc->delayChkptEnd = false;
/*
* Update upper-level FSM pages to account for the truncation. This is
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index a55545a187..ffc6056c60 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3946,9 +3946,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
* essential that CreateCheckpoint waits for virtual transactions
* rather than full transactionids.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
- delayChkpt = true;
+ MyProc->delayChkpt = delayChkpt = true;
lsn = XLogSaveBufferForHint(buffer, buffer_std);
}
@@ -3981,7 +3979,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
UnlockBufHdr(bufHdr, buf_state);
if (delayChkpt)
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
if (dirtied)
{
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index ae71d7538b..f250dbcce5 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -355,6 +355,10 @@ static void MaintainLatestCompletedXidRecovery(TransactionId latestXid);
static inline FullTransactionId FullXidRelativeTo(FullTransactionId rel,
TransactionId xid);
static void GlobalVisUpdateApply(ComputeXidHorizonsResult *horizons);
+static VirtualTransactionId *GetVirtualXIDsDelayingChkptGuts(int *nvxids,
+ bool start);
+static bool HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids,
+ int nvxids, bool start);
/*
* Report shared-memory space needed by CreateSharedProcArray.
@@ -690,8 +694,9 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
proc->lxid = InvalidLocalTransactionId;
proc->xmin = InvalidTransactionId;
- /* be sure this is cleared in abort */
- proc->delayChkpt = 0;
+ /* be sure these are cleared in abort */
+ proc->delayChkpt = false;
+ proc->delayChkptEnd = false;
proc->recoveryConflictPending = false;
@@ -732,8 +737,9 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid)
proc->lxid = InvalidLocalTransactionId;
proc->xmin = InvalidTransactionId;
- /* be sure this is cleared in abort */
- proc->delayChkpt = 0;
+ /* be sure these are cleared in abort */
+ proc->delayChkpt = false;
+ proc->delayChkptEnd = false;
proc->recoveryConflictPending = false;
@@ -921,6 +927,7 @@ ProcArrayClearTransaction(PGPROC *proc)
Assert(!(proc->statusFlags & PROC_VACUUM_STATE_MASK));
Assert(!proc->delayChkpt);
+ Assert(!proc->delayChkptEnd);
/*
* Need to increment completion count even though transaction hasn't
@@ -3049,8 +3056,9 @@ 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 specified delayChkpt bits set in their
- * PGPROC.
+ * critical sections, as shown by having the corresponding flag set in
+ * their PGPROC. GetVirtualXIDsDelayingChkpt checks delayChkpt, and
+ * GetVirtualXIDsDelayingChkptEnd checks delayChkptEnd.
*
* Returns a palloc'd array that should be freed by the caller.
* *nvxids is the number of valid entries.
@@ -3064,15 +3072,25 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly)
* for clearing of delayChkpt to propagate is unimportant for correctness.
*/
VirtualTransactionId *
-GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
+GetVirtualXIDsDelayingChkpt(int *nvxids)
+{
+ return GetVirtualXIDsDelayingChkptGuts(nvxids, true);
+}
+
+VirtualTransactionId *
+GetVirtualXIDsDelayingChkptEnd(int *nvxids)
+{
+ return GetVirtualXIDsDelayingChkptGuts(nvxids, false);
+}
+
+static VirtualTransactionId *
+GetVirtualXIDsDelayingChkptGuts(int *nvxids, bool start)
{
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);
@@ -3084,7 +3102,7 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
int pgprocno = arrayP->pgprocnos[index];
PGPROC *proc = &allProcs[pgprocno];
- if ((proc->delayChkpt & type) != 0)
+ if (start ? proc->delayChkpt : proc->delayChkptEnd)
{
VirtualTransactionId vxid;
@@ -3106,18 +3124,32 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
* This is used with the results of GetVirtualXIDsDelayingChkpt to see if any
* of the specified VXIDs are still in critical sections of code.
*
+ * HaveVirtualXIDsDelayingChkptEnd corresponds to
+ * GetVirtualXIDsDelayingChkptEnd above.
+ *
* 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)
+HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids)
+{
+ return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids, true);
+}
+
+bool
+HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids, int nvxids)
+{
+ return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids, false);
+}
+
+static bool
+HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids, int nvxids,
+ bool start)
{
bool result = false;
ProcArrayStruct *arrayP = procArray;
int index;
- Assert(type != 0);
-
LWLockAcquire(ProcArrayLock, LW_SHARED);
for (index = 0; index < arrayP->numProcs; index++)
@@ -3128,7 +3160,7 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
GET_VXID_FROM_PGPROC(vxid, *proc);
- if ((proc->delayChkpt & type) != 0 &&
+ if ((start ? proc->delayChkpt : proc->delayChkptEnd) &&
VirtualTransactionIdIsValid(vxid))
{
int i;
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index c50a419a54..4d2db6dff2 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -394,7 +394,8 @@ InitProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyProc->delayChkpt = 0;
+ MyProc->delayChkpt = false;
+ MyProc->delayChkptEnd = false;
MyProc->statusFlags = 0;
/* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
if (IsAutoVacuumWorkerProcess())
@@ -579,7 +580,8 @@ InitAuxiliaryProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyProc->delayChkpt = 0;
+ MyProc->delayChkpt = false;
+ MyProc->delayChkptEnd = false;
MyProc->statusFlags = 0;
MyProc->lwWaiting = false;
MyProc->lwWaitMode = 0;
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index b78012ec2b..5000818ca4 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -86,41 +86,6 @@ 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.
- */
-#define DELAY_CHKPT_START (1<<0)
-#define DELAY_CHKPT_COMPLETE (1<<1)
-
typedef enum
{
PROC_WAIT_STATUS_OK,
@@ -226,11 +191,16 @@ struct PGPROC
pg_atomic_uint64 waitStart; /* time at which wait for lock acquisition
* started */
- int delayChkpt; /* for DELAY_CHKPT_* flags */
+ bool delayChkpt; /* true if this proc delays checkpoint start */
uint8 statusFlags; /* this backend's status flags, see PROC_*
* above. mirrored in
* ProcGlobal->statusFlags[pgxactoff] */
+ /*
+ * delayChkptEnd is placed here to prevent ABI breakage by moving other
+ * struct members around.
+ */
+ bool delayChkptEnd; /* true if this proc delays checkpoint end */
/*
* Info to allow us to wait for synchronous replication, if needed.
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index 93de230a32..30fd7438a9 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -59,9 +59,11 @@ extern TransactionId GetOldestActiveTransactionId(void);
extern TransactionId GetOldestSafeDecodingTransactionId(bool catalogOnly);
extern void GetReplicationHorizons(TransactionId *slot_xmin, TransactionId *catalog_xmin);
-extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids, int type);
-extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids,
- int nvxids, int type);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids);
+extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkptEnd(int *nvxids);
+extern bool HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids,
+ int nvxids);
extern PGPROC *BackendPidGetProc(int pid);
extern PGPROC *BackendPidGetProcWithLock(int pid);
--
2.27.0
v2-0001-Fix-ABI-API-break_13.patch (text/x-patch, 17.3K) download
From 670fe40df53b44056845045b757d0d04f2fe28de Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 12 Apr 2022 18:00:07 +0900
Subject: [PATCH v2] Fix ABI/API break
Commit 1ce14b6b2f caused ABI break by changing the type and width of
PGPROC.delayChkpt and API break by changing the signature of some
public functions. Restore the member and function signature to
previous state and add delayChkptEnd to an existing gap in PGPROC
struct.
Backpatch to 10, all supported branches.
---
src/backend/access/transam/multixact.c | 6 +--
src/backend/access/transam/twophase.c | 13 +++---
src/backend/access/transam/xact.c | 5 +--
src/backend/access/transam/xlog.c | 10 ++---
src/backend/access/transam/xloginsert.c | 2 +-
src/backend/catalog/storage.c | 6 +--
src/backend/storage/buffer/bufmgr.c | 6 +--
src/backend/storage/ipc/procarray.c | 59 +++++++++++++++++++------
src/backend/storage/lmgr/proc.c | 6 ++-
src/include/storage/proc.h | 43 +++---------------
src/include/storage/procarray.h | 9 ++--
11 files changed, 82 insertions(+), 83 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 3e6443fd41..7990b5e5dd 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -3071,8 +3071,8 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
* crash/basebackup, even though the state of the data directory would
* require it.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ Assert(!MyProc->delayChkpt);
+ MyProc->delayChkpt = true;
/* WAL log truncation */
WriteMTruncateXlogRec(newOldestMultiDB,
@@ -3098,7 +3098,7 @@ TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB)
/* Then offsets */
PerformOffsetsTruncation(oldestMulti, newOldestMulti);
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
LWLockRelease(MultiXactTruncationLock);
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 716c17c98f..4c58c7579a 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -476,7 +476,8 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
}
pgxact->xid = xid;
pgxact->xmin = InvalidTransactionId;
- proc->delayChkpt = 0;
+ proc->delayChkpt = false;
+ proc->delayChkptEnd = false;
pgxact->vacuumFlags = 0;
proc->pid = 0;
proc->databaseId = databaseid;
@@ -1170,8 +1171,7 @@ EndPrepare(GlobalTransaction gxact)
START_CRIT_SECTION();
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
XLogBeginInsert();
for (record = records.head; record != NULL; record = record->next)
@@ -1214,7 +1214,7 @@ EndPrepare(GlobalTransaction gxact)
* checkpoint starting after this will certainly see the gxact as a
* candidate for fsyncing.
*/
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
/*
* Remember that we have this GlobalTransaction entry locked for us. If
@@ -2287,8 +2287,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
START_CRIT_SECTION();
/* See notes in RecordTransactionCommit */
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
/*
* Emit the XLOG commit record. Note that we mark 2PC commits as
@@ -2336,7 +2335,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
TransactionIdCommitTree(xid, nchildren, children);
/* Checkpoint can proceed now */
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index da6ce5a09e..fb6220e491 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1308,9 +1308,8 @@ 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 |= DELAY_CHKPT_START;
+ MyProc->delayChkpt = true;
SetCurrentTransactionStopTimestamp();
@@ -1411,7 +1410,7 @@ RecordTransactionCommit(void)
*/
if (markXidCommitted)
{
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
END_CRIT_SECTION();
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5ac45e612e..ffc1b8e995 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9028,27 +9028,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);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 5cff486d9e..b21679f09e 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -904,7 +904,7 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std)
/*
* Ensure no checkpoint can change our view of RedoRecPtr.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) != 0);
+ Assert(MyProc->delayChkpt);
/*
* 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 0eb14cc885..d7c74105a4 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -338,8 +338,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((MyProc->delayChkpt & DELAY_CHKPT_COMPLETE) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_COMPLETE;
+ Assert(!MyProc->delayChkptEnd);
+ MyProc->delayChkptEnd = true;
/*
* We WAL-log the truncation before actually truncating, which means
@@ -387,7 +387,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
smgrtruncate(rel->rd_smgr, forks, nforks, blocks);
/* We've done all the critical work, so checkpoints are OK now. */
- MyProc->delayChkpt &= ~DELAY_CHKPT_COMPLETE;
+ MyProc->delayChkptEnd = false;
/*
* Update upper-level FSM pages to account for the truncation. This is
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 033ef46811..597afedef7 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3647,9 +3647,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
* essential that CreateCheckpoint waits for virtual transactions
* rather than full transactionids.
*/
- Assert((MyProc->delayChkpt & DELAY_CHKPT_START) == 0);
- MyProc->delayChkpt |= DELAY_CHKPT_START;
- delayChkpt = true;
+ MyProc->delayChkpt = delayChkpt = true;
lsn = XLogSaveBufferForHint(buffer, buffer_std);
}
@@ -3682,7 +3680,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
UnlockBufHdr(bufHdr, buf_state);
if (delayChkpt)
- MyProc->delayChkpt &= ~DELAY_CHKPT_START;
+ MyProc->delayChkpt = false;
if (dirtied)
{
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 725680f34f..88853d25c6 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -175,6 +175,10 @@ static void KnownAssignedXidsReset(void);
static inline void ProcArrayEndTransactionInternal(PGPROC *proc,
PGXACT *pgxact, TransactionId latestXid);
static void ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid);
+static VirtualTransactionId *GetVirtualXIDsDelayingChkptGuts(int *nvxids,
+ bool start);
+static bool HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids,
+ int nvxids, bool start);
/*
* Report shared-memory space needed by CreateSharedProcArray.
@@ -435,8 +439,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 */
- proc->delayChkpt = 0;
+ /* be sure these are cleared in abort */
+ proc->delayChkpt = false;
+ proc->delayChkptEnd = false;
proc->recoveryConflictPending = false;
@@ -460,8 +465,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 */
- proc->delayChkpt = 0;
+ /* be sure these are cleared in abort */
+ proc->delayChkpt = false;
+ proc->delayChkptEnd = false;
proc->recoveryConflictPending = false;
@@ -2278,8 +2284,9 @@ 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 specified delayChkpt bits set in their
- * PGPROC.
+ * critical sections, as shown by having the corresponding flag set in
+ * their PGPROC. GetVirtualXIDsDelayingChkpt checks delayChkpt, and
+ * GetVirtualXIDsDelayingChkptEnd checks delayChkptEnd.
*
* Returns a palloc'd array that should be freed by the caller.
* *nvxids is the number of valid entries.
@@ -2293,15 +2300,25 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly)
* for clearing of delayChkpt to propagate is unimportant for correctness.
*/
VirtualTransactionId *
-GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
+GetVirtualXIDsDelayingChkpt(int *nvxids)
+{
+ return GetVirtualXIDsDelayingChkptGuts(nvxids, true);
+}
+
+VirtualTransactionId *
+GetVirtualXIDsDelayingChkptEnd(int *nvxids)
+{
+ return GetVirtualXIDsDelayingChkptGuts(nvxids, false);
+}
+
+static VirtualTransactionId *
+GetVirtualXIDsDelayingChkptGuts(int *nvxids, bool start)
{
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);
@@ -2313,7 +2330,7 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
int pgprocno = arrayP->pgprocnos[index];
PGPROC *proc = &allProcs[pgprocno];
- if ((proc->delayChkpt & type) != 0)
+ if (start ? proc->delayChkpt : proc->delayChkptEnd)
{
VirtualTransactionId vxid;
@@ -2335,18 +2352,32 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
* This is used with the results of GetVirtualXIDsDelayingChkpt to see if any
* of the specified VXIDs are still in critical sections of code.
*
+ * HaveVirtualXIDsDelayingChkptEnd corresponds to
+ * GetVirtualXIDsDelayingChkptEnd above.
+ *
* 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)
+HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids)
+{
+ return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids, true);
+}
+
+bool
+HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids, int nvxids)
+{
+ return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids, false);
+}
+
+static bool
+HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids, int nvxids,
+ bool start)
{
bool result = false;
ProcArrayStruct *arrayP = procArray;
int index;
- Assert(type != 0);
-
LWLockAcquire(ProcArrayLock, LW_SHARED);
for (index = 0; index < arrayP->numProcs; index++)
@@ -2357,7 +2388,7 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
GET_VXID_FROM_PGPROC(vxid, *proc);
- if ((proc->delayChkpt & type) != 0 &&
+ if ((start ? proc->delayChkpt : proc->delayChkptEnd) &&
VirtualTransactionIdIsValid(vxid))
{
int i;
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index f3a6c598bf..3d8b0c7098 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -396,7 +396,8 @@ InitProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyProc->delayChkpt = 0;
+ MyProc->delayChkpt = false;
+ MyProc->delayChkptEnd = false;
MyPgXact->vacuumFlags = 0;
/* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
if (IsAutoVacuumWorkerProcess())
@@ -578,7 +579,8 @@ InitAuxiliaryProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyProc->delayChkpt = 0;
+ MyProc->delayChkpt = false;
+ MyProc->delayChkptEnd = false;
MyPgXact->vacuumFlags = 0;
MyProc->lwWaiting = false;
MyProc->lwWaitMode = 0;
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 5798b91186..878a6b12e8 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -83,41 +83,6 @@ 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.
- */
-#define DELAY_CHKPT_START (1<<0)
-#define DELAY_CHKPT_COMPLETE (1<<1)
-
/*
* Each backend has a PGPROC struct in shared memory. There is also a list of
* currently-unused PGPROC structs that will be reallocated to new backends.
@@ -184,7 +149,13 @@ struct PGPROC
LOCKMASK heldLocks; /* bitmask for lock types already held on this
* lock object by this backend */
- int delayChkpt; /* for DELAY_CHKPT_* flags */
+ bool delayChkpt; /* true if this proc delays checkpoint start */
+
+ /*
+ * delayChkptEnd is placed here to prevent ABI breakage by moving other
+ * struct members around.
+ */
+ bool delayChkptEnd; /* true if this proc delays checkpoint end */
/*
* Info to allow us to wait for synchronous replication, if needed.
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index 4dee2dab10..e6b5fe6153 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -91,10 +91,11 @@ extern bool TransactionIdIsActive(TransactionId xid);
extern TransactionId GetOldestXmin(Relation rel, int flags);
extern TransactionId GetOldestActiveTransactionId(void);
extern TransactionId GetOldestSafeDecodingTransactionId(bool catalogOnly);
-
-extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids, int type);
-extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids,
- int nvxids, int type);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids);
+extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkptEnd(int *nvxids);
+extern bool HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids,
+ int nvxids);
extern PGPROC *BackendPidGetProc(int pid);
extern PGPROC *BackendPidGetProcWithLock(int pid);
--
2.27.0
v2-0001-Fix-ABI-API-break_12.patch (text/x-patch, 18.0K) download
From 5f25ca8356ebcb2f31eef434f25322ac92a26778 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 12 Apr 2022 17:50:36 +0900
Subject: [PATCH v2] Fix ABI/API break
Commit 3821d66a7b caused ABI break by changing the type and width of
PGXACT.delayChkpt and API break by changing the signature of some
public functions. Restore the member and function signature to
previous state, and add delayChkptEnd to an existing gap in PGPROC
struct instead of PGXACT since there's no room for the new variable in
PGXACT.
Backpatch to 10, all supported branches.
---
src/backend/access/transam/multixact.c | 6 +--
src/backend/access/transam/twophase.c | 13 +++---
src/backend/access/transam/xact.c | 5 +-
src/backend/access/transam/xlog.c | 10 ++--
src/backend/access/transam/xloginsert.c | 2 +-
src/backend/catalog/storage.c | 6 +--
src/backend/storage/buffer/bufmgr.c | 6 +--
src/backend/storage/ipc/procarray.c | 61 +++++++++++++++++++------
src/backend/storage/lmgr/proc.c | 6 ++-
src/include/storage/proc.h | 46 ++++---------------
src/include/storage/procarray.h | 8 ++--
11 files changed, 87 insertions(+), 82 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 757346cbbb..09748905a8 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -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);
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 602ca41054..44f7349920 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -477,7 +477,8 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
}
pgxact->xid = xid;
pgxact->xmin = InvalidTransactionId;
- pgxact->delayChkpt = 0;
+ pgxact->delayChkpt = false;
+ proc->delayChkptEnd = false;
pgxact->vacuumFlags = 0;
proc->pid = 0;
proc->databaseId = databaseid;
@@ -1187,8 +1188,7 @@ EndPrepare(GlobalTransaction gxact)
START_CRIT_SECTION();
- Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) == 0);
- MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = true;
XLogBeginInsert();
for (record = records.head; record != NULL; record = record->next)
@@ -1231,7 +1231,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 +2338,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
START_CRIT_SECTION();
/* See notes in RecordTransactionCommit */
- Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) == 0);
- MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = true;
/*
* Emit the XLOG commit record. Note that we mark 2PC commits as
@@ -2387,7 +2386,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
TransactionIdCommitTree(xid, nchildren, children);
/* Checkpoint can proceed now */
- MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = false;
END_CRIT_SECTION();
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 9d23298b2b..9c6b87c6ec 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1306,9 +1306,8 @@ 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);
START_CRIT_SECTION();
- MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = true;
SetCurrentTransactionStopTimestamp();
@@ -1409,7 +1408,7 @@ RecordTransactionCommit(void)
*/
if (markXidCommitted)
{
- MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = false;
END_CRIT_SECTION();
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7141e5dca8..889606517c 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -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);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index b51b0edd67..24a6f3148b 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -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
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 5a6324fec4..5c76e7a39b 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -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;
}
/*
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 7d11b0963f..01c09fd532 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3514,9 +3514,7 @@ 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;
- delayChkpt = true;
+ MyPgXact->delayChkpt = delayChkpt = true;
lsn = XLogSaveBufferForHint(buffer, buffer_std);
}
@@ -3549,7 +3547,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
UnlockBufHdr(bufHdr, buf_state);
if (delayChkpt)
- MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = false;
if (dirtied)
{
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 39093253fe..43456e36ae 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -172,6 +172,10 @@ static void KnownAssignedXidsReset(void);
static inline void ProcArrayEndTransactionInternal(PGPROC *proc,
PGXACT *pgxact, TransactionId latestXid);
static void ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid);
+static VirtualTransactionId *GetVirtualXIDsDelayingChkptGuts(int *nvxids,
+ bool start);
+static bool HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids,
+ int nvxids, bool start);
/*
* Report shared-memory space needed by CreateSharedProcArray.
@@ -435,8 +439,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 +465,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;
@@ -621,6 +627,7 @@ ProcArrayClearTransaction(PGPROC *proc)
/* redundant, but just in case */
pgxact->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
pgxact->delayChkpt = false;
+ proc->delayChkptEnd = false;
/* Clear the subtransaction-XID cache too */
pgxact->nxids = 0;
@@ -2267,8 +2274,10 @@ 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 specified delayChkpt bits set in their
- * PGXACT.
+ * critical sections, as shown by having the corresponding flag set in their
+ * PGPROC or PGXACT. GetVirtualXIDsDelayingChkpt checks PGXACT.delayChkpt, and
+ * GetVirtualXIDsDelayingChkptEnd checks PGPROC.delayChkptEnd. See PGPROC for
+ * the reason for the placement.
*
* Returns a palloc'd array that should be freed by the caller.
* *nvxids is the number of valid entries.
@@ -2282,15 +2291,25 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly)
* for clearing of delayChkpt to propagate is unimportant for correctness.
*/
VirtualTransactionId *
-GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
+GetVirtualXIDsDelayingChkpt(int *nvxids)
+{
+ return GetVirtualXIDsDelayingChkptGuts(nvxids, true);
+}
+
+VirtualTransactionId *
+GetVirtualXIDsDelayingChkptEnd(int *nvxids)
+{
+ return GetVirtualXIDsDelayingChkptGuts(nvxids, false);
+}
+
+static VirtualTransactionId *
+GetVirtualXIDsDelayingChkptGuts(int *nvxids, bool start)
{
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);
@@ -2303,7 +2322,7 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
PGPROC *proc = &allProcs[pgprocno];
PGXACT *pgxact = &allPgXact[pgprocno];
- if ((pgxact->delayChkpt & type) != 0)
+ if (start ? pgxact->delayChkpt : proc->delayChkptEnd)
{
VirtualTransactionId vxid;
@@ -2325,18 +2344,32 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
* This is used with the results of GetVirtualXIDsDelayingChkpt to see if any
* of the specified VXIDs are still in critical sections of code.
*
+ * HaveVirtualXIDsDelayingChkptEnd corresponds to
+ * GetVirtualXIDsDelayingChkptEnd above.
+ *
* 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)
+HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids)
+{
+ return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids, true);
+}
+
+bool
+HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids, int nvxids)
+{
+ return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids, false);
+}
+
+static bool
+HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids, int nvxids,
+ bool start)
{
bool result = false;
ProcArrayStruct *arrayP = procArray;
int index;
- Assert(type != 0);
-
LWLockAcquire(ProcArrayLock, LW_SHARED);
for (index = 0; index < arrayP->numProcs; index++)
@@ -2348,7 +2381,7 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
GET_VXID_FROM_PGPROC(vxid, *proc);
- if ((pgxact->delayChkpt & type) != 0 &&
+ if ((start ? pgxact->delayChkpt : proc->delayChkptEnd) &&
VirtualTransactionIdIsValid(vxid))
{
int i;
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 59291e01f4..cfb33199d5 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -397,7 +397,8 @@ InitProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyPgXact->delayChkpt = 0;
+ MyPgXact->delayChkpt = false;
+ MyProc->delayChkptEnd = false;
MyPgXact->vacuumFlags = 0;
/* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
if (IsAutoVacuumWorkerProcess())
@@ -579,7 +580,8 @@ InitAuxiliaryProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyPgXact->delayChkpt = 0;
+ MyPgXact->delayChkpt = false;
+ MyProc->delayChkptEnd = false;
MyPgXact->vacuumFlags = 0;
MyProc->lwWaiting = false;
MyProc->lwWaitMode = 0;
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 2a16fd23d4..de9326baa2 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -76,41 +76,6 @@ 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.
- */
-#define DELAY_CHKPT_START (1<<0)
-#define DELAY_CHKPT_COMPLETE (1<<1)
-
/*
* Each backend has a PGPROC struct in shared memory. There is also a list of
* currently-unused PGPROC structs that will be reallocated to new backends.
@@ -185,6 +150,14 @@ struct PGPROC
*/
XLogRecPtr waitLSN; /* waiting for this LSN or higher */
int syncRepState; /* wait state for sync rep */
+
+ /*
+ * This is a variable in duality with PGXACT.delayChkpt, but placed here to
+ * prevent ABI breakage by moving other struct members of the two structs
+ * around.
+ */
+ bool delayChkptEnd; /* true if this proc delays checkpoint end */
+
SHM_QUEUE syncRepLinks; /* list link if process is in syncrep queue */
/*
@@ -267,7 +240,8 @@ 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;
+ * previously called InCommit */
uint8 nxids;
} PGXACT;
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index d9ca460efc..88161e280a 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -92,9 +92,11 @@ extern TransactionId GetOldestXmin(Relation rel, int flags);
extern TransactionId GetOldestActiveTransactionId(void);
extern TransactionId GetOldestSafeDecodingTransactionId(bool catalogOnly);
-extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids, int type);
-extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids,
- int nvxids, int type);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids);
+extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkptEnd(int *nvxids);
+extern bool HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids,
+ int nvxids);
extern PGPROC *BackendPidGetProc(int pid);
extern PGPROC *BackendPidGetProcWithLock(int pid);
--
2.27.0
v2-0001-Fix-ABI-API-break_11.patch (text/x-patch, 18.0K) download
From f1b507270ceb6d4ab4a4e86ad9fcba473a788ec1 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 12 Apr 2022 17:53:01 +0900
Subject: [PATCH v2] Fix ABI/API break
Commit 118f1a332b caused ABI break by changing the type and width of
PGXACT.delayChkpt and API break by changing the signature of some
public functions. Restore the member and function signature to
previous state, and add delayChkptEnd to an existing gap in PGPROC
struct instead of PGXACT since there's no room for the new variable in
PGXACT.
Backpatch to 10, all supported branches.
---
src/backend/access/transam/multixact.c | 6 +--
src/backend/access/transam/twophase.c | 13 +++---
src/backend/access/transam/xact.c | 5 +-
src/backend/access/transam/xlog.c | 10 ++--
src/backend/access/transam/xloginsert.c | 2 +-
src/backend/catalog/storage.c | 6 +--
src/backend/storage/buffer/bufmgr.c | 6 +--
src/backend/storage/ipc/procarray.c | 61 +++++++++++++++++++------
src/backend/storage/lmgr/proc.c | 6 ++-
src/include/storage/proc.h | 46 ++++---------------
src/include/storage/procarray.h | 8 ++--
11 files changed, 87 insertions(+), 82 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 5612db0e21..ad9e7ff8f0 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -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);
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 769a5fd714..9f1e00608c 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -476,7 +476,8 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
}
pgxact->xid = xid;
pgxact->xmin = InvalidTransactionId;
- pgxact->delayChkpt = 0;
+ pgxact->delayChkpt = false;
+ proc->delayChkptEnd = false;
pgxact->vacuumFlags = 0;
proc->pid = 0;
proc->databaseId = databaseid;
@@ -1175,8 +1176,7 @@ EndPrepare(GlobalTransaction gxact)
START_CRIT_SECTION();
- Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) == 0);
- MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = true;
XLogBeginInsert();
for (record = records.head; record != NULL; record = record->next)
@@ -1219,7 +1219,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
@@ -2353,8 +2353,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
START_CRIT_SECTION();
/* See notes in RecordTransactionCommit */
- Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) == 0);
- MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = true;
/*
* Emit the XLOG commit record. Note that we mark 2PC commits as
@@ -2402,7 +2401,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
TransactionIdCommitTree(xid, nchildren, children);
/* Checkpoint can proceed now */
- MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = false;
END_CRIT_SECTION();
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5a86b6575e..e32b05d17f 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1239,9 +1239,8 @@ 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);
START_CRIT_SECTION();
- MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = true;
SetCurrentTransactionStopTimestamp();
@@ -1342,7 +1341,7 @@ RecordTransactionCommit(void)
*/
if (markXidCommitted)
{
- MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = false;
END_CRIT_SECTION();
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index cc0229b934..5de7143f65 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9064,27 +9064,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);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index a8c140b06f..c033e7bd4c 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -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
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 5d6f456c70..76a75a46b8 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -262,8 +262,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
@@ -311,7 +311,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;
}
/*
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 027d5067a0..459151519a 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3471,9 +3471,7 @@ 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;
- delayChkpt = true;
+ MyPgXact->delayChkpt = delayChkpt = true;
lsn = XLogSaveBufferForHint(buffer, buffer_std);
}
@@ -3506,7 +3504,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
UnlockBufHdr(bufHdr, buf_state);
if (delayChkpt)
- MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = false;
if (dirtied)
{
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index d88d955091..99ba68b4a3 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -171,6 +171,10 @@ static void KnownAssignedXidsReset(void);
static inline void ProcArrayEndTransactionInternal(PGPROC *proc,
PGXACT *pgxact, TransactionId latestXid);
static void ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid);
+static VirtualTransactionId *GetVirtualXIDsDelayingChkptGuts(int *nvxids,
+ bool start);
+static bool HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids,
+ int nvxids, bool start);
/*
* Report shared-memory space needed by CreateSharedProcArray.
@@ -434,8 +438,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;
@@ -459,8 +464,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;
@@ -626,6 +632,7 @@ ProcArrayClearTransaction(PGPROC *proc)
/* redundant, but just in case */
pgxact->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
pgxact->delayChkpt = false;
+ proc->delayChkptEnd = false;
/* Clear the subtransaction-XID cache too */
pgxact->nxids = 0;
@@ -2273,8 +2280,10 @@ 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 specified delayChkpt bits set in their
- * PGXACT.
+ * critical sections, as shown by having the corresponding flag set in their
+ * PGPROC or PGXACT. GetVirtualXIDsDelayingChkpt checks PGXACT.delayChkpt, and
+ * GetVirtualXIDsDelayingChkptEnd checks PGPROC.delayChkptEnd. See PGPROC for
+ * the reason for the placement.
*
* Returns a palloc'd array that should be freed by the caller.
* *nvxids is the number of valid entries.
@@ -2288,15 +2297,25 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly)
* for clearing of delayChkpt to propagate is unimportant for correctness.
*/
VirtualTransactionId *
-GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
+GetVirtualXIDsDelayingChkpt(int *nvxids)
+{
+ return GetVirtualXIDsDelayingChkptGuts(nvxids, true);
+}
+
+VirtualTransactionId *
+GetVirtualXIDsDelayingChkptEnd(int *nvxids)
+{
+ return GetVirtualXIDsDelayingChkptGuts(nvxids, false);
+}
+
+static VirtualTransactionId *
+GetVirtualXIDsDelayingChkptGuts(int *nvxids, bool start)
{
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);
@@ -2309,7 +2328,7 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
volatile PGPROC *proc = &allProcs[pgprocno];
volatile PGXACT *pgxact = &allPgXact[pgprocno];
- if ((pgxact->delayChkpt & type) != 0)
+ if (start ? pgxact->delayChkpt : proc->delayChkptEnd)
{
VirtualTransactionId vxid;
@@ -2331,18 +2350,32 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
* This is used with the results of GetVirtualXIDsDelayingChkpt to see if any
* of the specified VXIDs are still in critical sections of code.
*
+ * HaveVirtualXIDsDelayingChkptEnd corresponds to
+ * GetVirtualXIDsDelayingChkptEnd above.
+ *
* 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)
+HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids)
+{
+ return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids, true);
+}
+
+bool
+HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids, int nvxids)
+{
+ return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids, false);
+}
+
+static bool
+HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids, int nvxids,
+ bool start)
{
bool result = false;
ProcArrayStruct *arrayP = procArray;
int index;
- Assert(type != 0);
-
LWLockAcquire(ProcArrayLock, LW_SHARED);
for (index = 0; index < arrayP->numProcs; index++)
@@ -2354,7 +2387,7 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
GET_VXID_FROM_PGPROC(vxid, *proc);
- if ((pgxact->delayChkpt & type) != 0 &&
+ if ((start ? pgxact->delayChkpt : proc->delayChkptEnd) &&
VirtualTransactionIdIsValid(vxid))
{
int i;
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index aaecfa67b7..b0b63d4c99 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -380,7 +380,8 @@ InitProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyPgXact->delayChkpt = 0;
+ MyPgXact->delayChkpt = false;
+ MyProc->delayChkptEnd = false;
MyPgXact->vacuumFlags = 0;
/* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
if (IsAutoVacuumWorkerProcess())
@@ -562,7 +563,8 @@ InitAuxiliaryProcess(void)
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyPgXact->delayChkpt = 0;
+ MyPgXact->delayChkpt = false;
+ MyProc->delayChkptEnd = false;
MyPgXact->vacuumFlags = 0;
MyProc->lwWaiting = false;
MyProc->lwWaitMode = 0;
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index e76ca8a11e..8cec3d74f4 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -76,41 +76,6 @@ 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.
- */
-#define DELAY_CHKPT_START (1<<0)
-#define DELAY_CHKPT_COMPLETE (1<<1)
-
/*
* Each backend has a PGPROC struct in shared memory. There is also a list of
* currently-unused PGPROC structs that will be reallocated to new backends.
@@ -185,6 +150,14 @@ struct PGPROC
*/
XLogRecPtr waitLSN; /* waiting for this LSN or higher */
int syncRepState; /* wait state for sync rep */
+
+ /*
+ * This is a variable in duality with PGXACT.delayChkpt, but placed here to
+ * prevent ABI breakage by moving other struct members of the two structs
+ * around.
+ */
+ bool delayChkptEnd; /* true if this proc delays checkpoint end */
+
SHM_QUEUE syncRepLinks; /* list link if process is in syncrep queue */
/*
@@ -267,7 +240,8 @@ 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;
+ * previously called InCommit */
uint8 nxids;
} PGXACT;
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index a69632a70c..0c669c7965 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -92,9 +92,11 @@ extern TransactionId GetOldestXmin(Relation rel, int flags);
extern TransactionId GetOldestActiveTransactionId(void);
extern TransactionId GetOldestSafeDecodingTransactionId(bool catalogOnly);
-extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids, int type);
-extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids,
- int nvxids, int type);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids);
+extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkptEnd(int *nvxids);
+extern bool HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids,
+ int nvxids);
extern PGPROC *BackendPidGetProc(int pid);
extern PGPROC *BackendPidGetProcWithLock(int pid);
--
2.27.0
v2-0001-Fix-ABI-API-break_10.patch (text/x-patch, 17.9K) download
From 9656334d2dd6a85fde9c79736bd6615bb060e371 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Tue, 12 Apr 2022 17:54:25 +0900
Subject: [PATCH v2] Fix ABI/API break
Commit 57f618310f caused ABI break by changing the type and width of
PGXACT.delayChkpt and API break by changing the signature of some
public functions. Restore the member and function signature to
previous state, and add delayChkptEnd to an existing gap in PGPROC
struct instead of PGXACT since there's no room for the new variable in
PGXACT.
Backpatch to 10, all supported branches.
---
src/backend/access/transam/multixact.c | 6 +--
src/backend/access/transam/twophase.c | 13 +++---
src/backend/access/transam/xact.c | 5 +-
src/backend/access/transam/xlog.c | 10 ++--
src/backend/access/transam/xloginsert.c | 2 +-
src/backend/catalog/storage.c | 6 +--
src/backend/storage/buffer/bufmgr.c | 6 +--
src/backend/storage/ipc/procarray.c | 61 +++++++++++++++++++------
src/backend/storage/lmgr/proc.c | 6 ++-
src/include/storage/proc.h | 46 ++++---------------
src/include/storage/procarray.h | 8 ++--
11 files changed, 87 insertions(+), 82 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 1e52972bbf..cdaf499348 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -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);
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index c61b2736a1..26a4faf388 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -478,7 +478,8 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
}
pgxact->xid = xid;
pgxact->xmin = InvalidTransactionId;
- pgxact->delayChkpt = 0;
+ pgxact->delayChkpt = false;
+ proc->delayChkptEnd = false;
pgxact->vacuumFlags = 0;
proc->pid = 0;
proc->databaseId = databaseid;
@@ -1159,8 +1160,7 @@ EndPrepare(GlobalTransaction gxact)
START_CRIT_SECTION();
- Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) == 0);
- MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = true;
XLogBeginInsert();
for (record = records.head; record != NULL; record = record->next)
@@ -1192,7 +1192,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
@@ -2285,8 +2285,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
START_CRIT_SECTION();
/* See notes in RecordTransactionCommit */
- Assert((MyPgXact->delayChkpt & DELAY_CHKPT_START) == 0);
- MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = true;
/*
* Emit the XLOG commit record. Note that we mark 2PC commits as
@@ -2334,7 +2333,7 @@ RecordTransactionCommitPrepared(TransactionId xid,
TransactionIdCommitTree(xid, nchildren, children);
/* Checkpoint can proceed now */
- MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = false;
END_CRIT_SECTION();
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index ccd99c38c2..25a3a4f97e 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1247,9 +1247,8 @@ 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);
START_CRIT_SECTION();
- MyPgXact->delayChkpt |= DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = true;
SetCurrentTransactionStopTimestamp();
@@ -1350,7 +1349,7 @@ RecordTransactionCommit(void)
*/
if (markXidCommitted)
{
- MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = false;
END_CRIT_SECTION();
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 51efc106a2..be6d9b239a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9022,27 +9022,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);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 6ff19814d4..579d8de775 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -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
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 729fb92c5f..9497e1d88d 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -263,8 +263,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
@@ -312,7 +312,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;
}
/*
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 0b7bdb8634..bafe91ab0d 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3469,9 +3469,7 @@ 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;
- delayChkpt = true;
+ MyPgXact->delayChkpt = delayChkpt = true;
lsn = XLogSaveBufferForHint(buffer, buffer_std);
}
@@ -3504,7 +3502,7 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
UnlockBufHdr(bufHdr, buf_state);
if (delayChkpt)
- MyPgXact->delayChkpt &= ~DELAY_CHKPT_START;
+ MyPgXact->delayChkpt = false;
if (dirtied)
{
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 134b63f28b..9f5b26f201 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -171,6 +171,10 @@ static void KnownAssignedXidsReset(void);
static inline void ProcArrayEndTransactionInternal(PGPROC *proc,
PGXACT *pgxact, TransactionId latestXid);
static void ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid);
+static VirtualTransactionId *GetVirtualXIDsDelayingChkptGuts(int *nvxids,
+ bool start);
+static bool HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids,
+ int nvxids, bool start);
/*
* Report shared-memory space needed by CreateSharedProcArray.
@@ -434,8 +438,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;
@@ -459,8 +464,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;
@@ -626,6 +632,7 @@ ProcArrayClearTransaction(PGPROC *proc)
/* redundant, but just in case */
pgxact->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
pgxact->delayChkpt = false;
+ proc->delayChkptEnd = false;
/* Clear the subtransaction-XID cache too */
pgxact->nxids = 0;
@@ -2265,8 +2272,10 @@ 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 specified delayChkpt bits set in their
- * PGXACT.
+ * critical sections, as shown by having the corresponding flag set in their
+ * PGPROC or PGXACT. GetVirtualXIDsDelayingChkpt checks PGXACT.delayChkpt, and
+ * GetVirtualXIDsDelayingChkptEnd checks PGPROC.delayChkptEnd. See PGPROC for
+ * the reason for the placement.
*
* Returns a palloc'd array that should be freed by the caller.
* *nvxids is the number of valid entries.
@@ -2280,15 +2289,25 @@ GetOldestSafeDecodingTransactionId(bool catalogOnly)
* for clearing of delayChkpt to propagate is unimportant for correctness.
*/
VirtualTransactionId *
-GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
+GetVirtualXIDsDelayingChkpt(int *nvxids)
+{
+ return GetVirtualXIDsDelayingChkptGuts(nvxids, true);
+}
+
+VirtualTransactionId *
+GetVirtualXIDsDelayingChkptEnd(int *nvxids)
+{
+ return GetVirtualXIDsDelayingChkptGuts(nvxids, false);
+}
+
+static VirtualTransactionId *
+GetVirtualXIDsDelayingChkptGuts(int *nvxids, bool start)
{
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);
@@ -2301,7 +2320,7 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
volatile PGPROC *proc = &allProcs[pgprocno];
volatile PGXACT *pgxact = &allPgXact[pgprocno];
- if ((pgxact->delayChkpt & type) != 0)
+ if (start ? pgxact->delayChkpt : proc->delayChkptEnd)
{
VirtualTransactionId vxid;
@@ -2323,18 +2342,32 @@ GetVirtualXIDsDelayingChkpt(int *nvxids, int type)
* This is used with the results of GetVirtualXIDsDelayingChkpt to see if any
* of the specified VXIDs are still in critical sections of code.
*
+ * HaveVirtualXIDsDelayingChkptEnd corresponds to
+ * GetVirtualXIDsDelayingChkptEnd above.
+ *
* 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)
+HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids)
+{
+ return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids, true);
+}
+
+bool
+HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids, int nvxids)
+{
+ return HaveVirtualXIDsDelayingChkptGuts(vxids, nvxids, false);
+}
+
+static bool
+HaveVirtualXIDsDelayingChkptGuts(VirtualTransactionId *vxids, int nvxids,
+ bool start)
{
bool result = false;
ProcArrayStruct *arrayP = procArray;
int index;
- Assert(type != 0);
-
LWLockAcquire(ProcArrayLock, LW_SHARED);
for (index = 0; index < arrayP->numProcs; index++)
@@ -2346,7 +2379,7 @@ HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids, int type)
GET_VXID_FROM_PGPROC(vxid, *proc);
- if ((pgxact->delayChkpt & type) != 0 &&
+ if ((start ? pgxact->delayChkpt : proc->delayChkptEnd) &&
VirtualTransactionIdIsValid(vxid))
{
int i;
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index e5370df019..0efee8411e 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -377,7 +377,8 @@ InitProcess(void)
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyPgXact->delayChkpt = 0;
+ MyPgXact->delayChkpt = false;
+ MyProc->delayChkptEnd = false;
MyPgXact->vacuumFlags = 0;
/* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
if (IsAutoVacuumWorkerProcess())
@@ -550,7 +551,8 @@ InitAuxiliaryProcess(void)
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
MyProc->isBackgroundWorker = IsBackgroundWorker;
- MyPgXact->delayChkpt = 0;
+ MyPgXact->delayChkpt = false;
+ MyProc->delayChkptEnd = false;
MyPgXact->vacuumFlags = 0;
MyProc->lwWaiting = false;
MyProc->lwWaitMode = 0;
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index d8dd7bf5e1..e6d0f74458 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -75,41 +75,6 @@ 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.
- */
-#define DELAY_CHKPT_START (1<<0)
-#define DELAY_CHKPT_COMPLETE (1<<1)
-
/*
* Each backend has a PGPROC struct in shared memory. There is also a list of
* currently-unused PGPROC structs that will be reallocated to new backends.
@@ -181,6 +146,14 @@ struct PGPROC
*/
XLogRecPtr waitLSN; /* waiting for this LSN or higher */
int syncRepState; /* wait state for sync rep */
+
+ /*
+ * This is a variable in duality with PGXACT.delayChkpt, but placed here to
+ * prevent ABI breakage by moving other struct members of the two structs
+ * around.
+ */
+ bool delayChkptEnd; /* true if this proc delays checkpoint end */
+
SHM_QUEUE syncRepLinks; /* list link if process is in syncrep queue */
/*
@@ -252,7 +225,8 @@ 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;
+ * previously called InCommit */
uint8 nxids;
} PGXACT;
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index 2b60b27604..9a139c5f61 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -92,9 +92,11 @@ extern TransactionId GetOldestXmin(Relation rel, int flags);
extern TransactionId GetOldestActiveTransactionId(void);
extern TransactionId GetOldestSafeDecodingTransactionId(bool catalogOnly);
-extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids, int type);
-extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids,
- int nvxids, int type);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkpt(int *nvxids);
+extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids);
+extern VirtualTransactionId *GetVirtualXIDsDelayingChkptEnd(int *nvxids);
+extern bool HaveVirtualXIDsDelayingChkptEnd(VirtualTransactionId *vxids,
+ int nvxids);
extern PGPROC *BackendPidGetProc(int pid);
extern PGPROC *BackendPidGetProcWithLock(int pid);
--
2.27.0
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-14 15:13 Robert Haas <robertmhaas@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 1 reply; 23+ messages in thread
From: Robert Haas @ 2022-04-14 15:13 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: Markus Wanner <markus.wanner@enterprisedb.com>; Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Tue, Apr 12, 2022 at 6:55 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> (My mailer has been fixed.)
Cool.
> So, I created the patches for back-patching from 10 to 14. (With
> fixed a silly bug of the v1-pg14 that HaveVirtualXIDsDelayingChkpt and
> HaveVirtualXIDsDelayingChkptEnd are inverted..)
I am very sorry not to use these, but as I said in my previous post on
this thread, I prefer to commit what I wrote and Markus revised rather
than these versions. I have done that now.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.]
@ 2022-04-15 01:20 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 0 replies; 23+ messages in thread
From: Kyotaro Horiguchi @ 2022-04-15 01:20 UTC (permalink / raw)
To: robertmhaas@gmail.com; +Cc: markus.wanner@enterprisedb.com; michael@paquier.xyz; tgl@sss.pgh.pa.us; pgsql-hackers
At Thu, 14 Apr 2022 11:13:01 -0400, Robert Haas <robertmhaas@gmail.com> wrote in
> On Tue, Apr 12, 2022 at 6:55 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > (My mailer has been fixed.)
>
> Cool.
>
> > So, I created the patches for back-patching from 10 to 14. (With
> > fixed a silly bug of the v1-pg14 that HaveVirtualXIDsDelayingChkpt and
> > HaveVirtualXIDsDelayingChkptEnd are inverted..)
>
> I am very sorry not to use these, but as I said in my previous post on
> this thread, I prefer to commit what I wrote and Markus revised rather
> than these versions. I have done that now.
Not at all. It's just an unfortunate crossing.
Thanks for fixing this.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 23+ messages in thread
end of thread, other threads:[~2022-04-15 01:20 UTC | newest]
Thread overview: 23+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-05 13:02 API stability [was: pgsql: Fix possible recovery trouble if TRUNCATE overlaps a checkpoint.] Markus Wanner <markus.wanner@enterprisedb.com>
2022-04-05 14:01 ` Robert Haas <robertmhaas@gmail.com>
2022-04-05 14:17 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-04-05 14:29 ` Robert Haas <robertmhaas@gmail.com>
2022-04-05 14:32 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-04-05 14:51 ` Robert Haas <robertmhaas@gmail.com>
2022-04-05 19:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-04-07 06:28 ` Michael Paquier <michael@paquier.xyz>
2022-04-07 14:04 ` Robert Haas <robertmhaas@gmail.com>
2022-04-07 15:19 ` Robert Haas <robertmhaas@gmail.com>
2022-04-07 15:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-04-07 18:07 ` Petr Jelinek <petr.jelinek@enterprisedb.com>
2022-04-07 23:47 ` Michael Paquier <michael@paquier.xyz>
2022-04-07 23:52 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-04-08 02:19 ` Robert Haas <robertmhaas@gmail.com>
2022-04-08 07:13 ` Michael Paquier <michael@paquier.xyz>
2022-04-08 08:47 ` Markus Wanner <markus.wanner@enterprisedb.com>
2022-04-08 15:50 ` Robert Haas <robertmhaas@gmail.com>
2022-04-11 19:21 ` Robert Haas <robertmhaas@gmail.com>
2022-04-11 19:45 ` Markus Wanner <markus.wanner@enterprisedb.com>
2022-04-12 10:54 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2022-04-14 15:13 ` Robert Haas <robertmhaas@gmail.com>
2022-04-15 01:20 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox