Issues with 2PC at recovery: CLOG lookups and GlobalTransactionData

On Thu, Jan 30, 2025 at 03:36:20PM +0900, Michael Paquier wrote:

This needs more eyes, so I'll go add an entry in the CF. Patches for
stable branches are labelled with .txt, to avoid any interference with
the CF bot.

Thoughts and comments are welcome.

After sleeping over it, I've been able to come up with a split of the
patches for v17 and HEAD: a first one to refactor the 2PC code to
integrate more FullTransactionIds (down to 17) and a second one to
implement a fix. That should make reviews easier. The second patch
for 17 and HEAD matches with the fixes for v13~v16, all are labelled
with a 0002.
--
Michael

On Friday, January 31, 2025 03:21 MSK, Michael Paquier <michael@paquier.xyz>
wrote:

Thoughts and comments are welcome.

I'm looking at the v13 patch. I see, there is the only file for v13:
v2-0002-Fix-issues-with-2PC-file-handling-at-recovery-13.txt

There are two points I would like to highlight:

#1. In RecoverPreparedTransactions we iterave over all in-memory two phase
states and check every xid state in CLOG unconditionally. Image, we have
a very old transaction that is close to the oldestXid. Will CLOG state be
available for such transaction? I'm not sure about it.

#2. In restoreTwoPhaseData we load all the twostate files that are in
the valid xid range (from oldestXid to nextFullXid in terms of logical
comparision of xids with epoch). The question - should we load files
which xids greater than ControlFile->checkPointCopy.nextXid
(xids after last checkpoint). In general, all twophase files should belong
to xids before the last checkpoint. I guess, we should probably ignore
files which xid is equal or greater than the xid of the last checkpoint -
twostate data should be in the WAL. If not, I guess, we may see error messages
like show below when doing xact_redo -> PrepareRedoAdd:

Two-phase state file has been found in WAL record %X/%X, but this transaction
has already been restored from disk

I'm not sure about the logic related to this message in PrepareRedoAdd.

P.S. Thank you for responses on my emails. I also apologize for the
formatting of my emails. I will check what is wrong and fix.

With best regards,
Vitaly

On Fri, Jan 31, 2025 at 04:54:17PM +0300, Vitaly Davydov wrote:

I'm looking at the v13 patch. I see, there is the only file for v13:
v2-0002-Fix-issues-with-2PC-file-handling-at-recovery-13.txt

Yes. The fixes in 13~16 are simpler. There are so many conflicts
across all the branches that I'm planning to maintain branches for
each stable versions, depending on the discussion going on. The
refactoring of 17~ and HEAD could be done first, I suppose.

#1. In RecoverPreparedTransactions we iterave over all in-memory two phase
states and check every xid state in CLOG unconditionally. Image, we have
a very old transaction that is close to the oldestXid. Will CLOG state be
available for such transaction? I'm not sure about it.

Yes. RecoverPreparedTransactions() where the cluster is officially at
the end of recovery, and we've checked that we are consistent.

#2. In restoreTwoPhaseData we load all the twostate files that are in
the valid xid range (from oldestXid to nextFullXid in terms of logical
comparision of xids with epoch). The question - should we load files
which xids greater than ControlFile->checkPointCopy.nextXid
(xids after last checkpoint). In general, all twophase files should belong
to xids before the last checkpoint. I guess, we should probably ignore
files which xid is equal or greater than the xid of the last checkpoint -
twostate data should be in the WAL. If not, I guess, we may see error messages
like show below when doing xact_redo -> PrepareRedoAdd:

We are already using the nextFullXid from ShmemVariableCache, which is
something that StartupXLOG() fetches from the checkpoint record we
define as the starting point of recovery, which provides a protection
good enough. Grabbing this information from checkPointCopy.nextXid
would be actually worse when recovering from a backup_label, no? The
copy of the checkpoint record in the control file would be easily
newer than the checkpoint record retrieved from the LSN in the
backup_label, leading to less files considered as in the future. I'd
rather trust what we have in WAL a maximum for this data.

Note that there is a minor release next week on the roadmap, and for
clarity I am not planning to rush this stuff in the tree this week:
https://www.postgresql.org/developer/roadmap/

Let's give it more time to find a solution we're all OK with. I'd
love to hear from Noah here as well, as he got involved in the
discussion of the other thread.
--
Michael

On Thu, Jan 30, 2025 at 03:36:20PM +0900, Michael Paquier wrote:

And I am beginning a new thread about going through an issue that Noah
has mentioned at [1], which is that the 2PC code may attempt to do
CLOG lookups at very early stage of recovery, where the cluster is not
in a consistent state.

It's broader than CLOG lookups. I wrote in [1], "We must not read the old
pg_twophase file, which may contain an unfinished write." Until recovery
reaches consistency, none of the checks in ProcessTwoPhaseBuffer() or its
callee ReadTwoPhaseFile() are safe.

[1]: /messages/by-id/20250117005221.05.nmisch@google.com
[2]: /messages/by-id/20250116205254.65.nmisch@google.com

On Fri, Jan 31, 2025 at 09:21:53AM +0900, Michael Paquier wrote:

--- a/src/test/recovery/t/009_twophase.pl
+++ b/src/test/recovery/t/009_twophase.pl

+	log_like => [
+		qr/removing stale two-phase state from memory for transaction $commit_prepared_xid of epoch 0/,
+		qr/removing stale two-phase state from memory for transaction $abort_prepared_xid of epoch 0/
+	]);

+	log_like => [
+		qr/removing past two-phase state file for transaction 4095 of epoch 238/,
+		qr/removing future two-phase state file for transaction 4095 of epoch 511/
+	]);

As I wrote in [1], "By the time we reach consistency, every file in
pg_twophase will be applicable (not committed or aborted)." If we find
otherwise, the user didn't follow the backup protocol (or there's another
bug). Hence, long-term, we should stop these removals and just fail recovery.
We can't fix all data loss consequences of not following the backup protocol,
so the biggest favor we can do the user is draw their attention to the
problem. How do you see it?

For back branches, the ideal is less clear. If we can convince ourselves that
enough of these events will indicate damaging problems (user error, hardware
failure, or PostgreSQL bugs), the long-term ideal of failing recovery is also
right for back branches. However, it could be too hard to convince ourselves
of that. If so, that could justify keeping these removals in back branches.

On Tue, Feb 18, 2025 at 04:57:47PM -0800, Noah Misch wrote:

As I wrote in [1], "By the time we reach consistency, every file in
pg_twophase will be applicable (not committed or aborted)." If we find
otherwise, the user didn't follow the backup protocol (or there's another
bug). Hence, long-term, we should stop these removals and just fail recovery.
We can't fix all data loss consequences of not following the backup protocol,
so the biggest favor we can do the user is draw their attention to the
problem. How do you see it?

Deciding to not trust at all any of the contents of pg_twophase/ until
consistency is reached is not something we should aim for, IMO. Going
in this direction would mean to delay restoreTwoPhaseData() until
consistency is reached, but there are cases where we can read that
safely, and where we should do so. For example, this flow is
perfectly OK to do in the wasShutdown case, where
PrescanPreparedTransactions() would be able to do its initialization
job before performing WAL recovery to get a clean list of running
XIDs.

I agree that moving towards a solution where we get rid entirely of
the CLOG lookups in ProcessTwoPhaseBuffer() is what we should aim for,
and actually is there a reason to not just nuke and replace them
something based on the checkpoint record itself? I have to admit that
I don't quite see the issue with ReadTwoPhaseFile() when it comes to
crash recovery. For example, in the case of a partial write, doesn't
the CRC32 check offer some protection about the contents of the file?
Wouldn't it be OK in this case to assume that the contents of this
file will be in WAL anyway? We have one case based on
reachedConsistency in PrepareRedoAdd(), should this happen, for
crashes happening during checkpoints where we know that the files
could be found in WAL but they could have been loaded at the beginning
of recovery.

The base backup issue is a different one, of course, and I think that
we are going to require more data in the 2PC file to provide a better
cross-check barrier, which would be the addition to the 2PC file of
the end LSN where the 2PC file record has been inserted. Then we
could cross-check that with the redo location, and see that it's
actually safe to discard the file because we know it will be in WAL.
This seems like a hefty cost to pay for, though, meaning 8 bytes in
each 2PC file because base backups were done wrong. Bleh.

One extra thing that I have mentioned is that we could replace the
CLOG safeguards based on what we know from the checkpoint record based
on the oldest XID horizon of the checkpoint record and its next XID:
- If we have a 2PC file older than the oldest XID horizon, we know
that it should not exist.
- If we have a 2PC file newer than the next XID, same, or we'll know
about it while replaying.

WDYT?

For back branches, the ideal is less clear. If we can convince ourselves that
enough of these events will indicate damaging problems (user error, hardware
failure, or PostgreSQL bugs), the long-term ideal of failing recovery is also
right for back branches. However, it could be too hard to convince ourselves
of that. If so, that could justify keeping these removals in back branches.

While I would like to see something in back-branches, I am not seeing
occurences of the current behavior being hit by users in the wild, so
I'd rather consider that as material only for v19 at this stage. If
we figure out a clear picture on HEAD, perhaps it will be possible to
salvage some of it for the back branches, but I'm not clear if this
would be possible yet. That would be nice, but we'll see.
--
Michael

On Fri, May 09, 2025 at 02:08:26PM +0900, Michael Paquier wrote:

One extra thing that I have mentioned is that we could replace the
CLOG safeguards based on what we know from the checkpoint record based
on the oldest XID horizon of the checkpoint record and its next XID:
- If we have a 2PC file older than the oldest XID horizon, we know
that it should not exist.
- If we have a 2PC file newer than the next XID, same, or we'll know
about it while replaying.

WDYT?

I have been going back and forth on this patch set for the last few
weeks. Please find a refreshed version as of the attached. These are
aimed only for v19 for the time being.

Patch 0001 is a refactoring of the existing 2PC code to integrate full
XIDs deeper into these code paths, easing the evaluation of the 2PC
files by not having to guess from which epoch they are. This is worth
improving on its own, and I'm hoing that this is acceptable as-is for
HEAD.

To summarize, patch 0002 provides fixes in the lines of what I have
mentioned upthread, based on the following lines:
- restoreTwoPhaseData() is in charge of checking the 2PC files in
pg_twophase/ at the beginning of recovery based on the oldest and
newest XID horizons retrieved from the checkpoint record, discarding
files seen as too new or too old.
- CLOG lookups are delayed at the end of recovery, handled by
RecoverPreparedTransactions() when we retore the 2PC data in shmem
filled during recovery. At this stage, checking for aborted and
committed transactions should be safe, the cluster is moved as ready
for WAL activity in the steps after this call.
- ProcessTwoPhaseBuffer() can never return NULL, does not include any
sanity checks anymore.

Thoughts or comments are welcome.
--
Michael

On Fri, May 09, 2025 at 02:08:26PM +0900, Michael Paquier wrote:

On Tue, Feb 18, 2025 at 04:57:47PM -0800, Noah Misch wrote:

As I wrote in [1], "By the time we reach consistency, every file in
pg_twophase will be applicable (not committed or aborted)." If we find
otherwise, the user didn't follow the backup protocol (or there's another
bug). Hence, long-term, we should stop these removals and just fail recovery.
We can't fix all data loss consequences of not following the backup protocol,
so the biggest favor we can do the user is draw their attention to the
problem. How do you see it?

Deciding to not trust at all any of the contents of pg_twophase/ until
consistency is reached is not something we should aim for, IMO. Going
in this direction would mean to delay restoreTwoPhaseData() until
consistency is reached, but there are cases where we can read that
safely, and where we should do so. For example, this flow is
perfectly OK to do in the wasShutdown case, where
PrescanPreparedTransactions() would be able to do its initialization
job before performing WAL recovery to get a clean list of running
XIDs.

The wasShutdown case reaches consistency from the beginning, so I don't see
that as an example of a time we benefit from reading pg_twophase before
reaching consistency. Can you elaborate on that?

What's the benefit you're trying to get by reading pg_twophase before reaching
consistency?

Before reaching consistency, our normal approach is to let WAL tell us what to
read, not explore the data directory for files of interest. That's a good
principle, because there are few bounds on the chaos that may exist in the
files of the data directory before reaching consistency. Today's twophase
departs from that principle. In light of this thread's problems, we should
have a strong reason for keeping that departure. The default should be to
align with the rest of recovery in this respect.

I can think of one benefit of attempting to read pg_twophase before reaching
consistency. Suppose we can prove that a pg_twophase file will cause an error
by end of recovery, regardless of what WAL contains. It's nice to fail
recovery immediately instead of failing recovery when we reach consistency.
However, I doubt that benefit is important enough to depart from our usual
principle and incur additional storage seeks in order to achieve that benefit.
If recovery will certainly fail, you are going to have a bad day anyway.
Accelerating recovery failure is a small benefit, particularly when we'd
accelerate failure for only a small slice of recovery failure causes.

I agree that moving towards a solution where we get rid entirely of
the CLOG lookups in ProcessTwoPhaseBuffer() is what we should aim for,
and actually is there a reason to not just nuke and replace them
something based on the checkpoint record itself?

I don't know what this means.

I have to admit that
I don't quite see the issue with ReadTwoPhaseFile() when it comes to
crash recovery. For example, in the case of a partial write, doesn't
the CRC32 check offer some protection about the contents of the file?

Not the protection we want. If we've not reached consistency, we must not
ERROR "calculated CRC checksum does not match value stored in file" for a file
that later WAL may recreate. That might be what you're saying:

Wouldn't it be OK in this case to assume that the contents of this
file will be in WAL anyway?

Sure. Meanwhile, if a twophase file is going to be in later WAL, what's the
value in opening the file before we get to that WAL?

The base backup issue is a different one, of course, and I think that
we are going to require more data in the 2PC file to provide a better
cross-check barrier, which would be the addition to the 2PC file of
the end LSN where the 2PC file record has been inserted. Then we
could cross-check that with the redo location, and see that it's
actually safe to discard the file because we know it will be in WAL.
This seems like a hefty cost to pay for, though, meaning 8 bytes in
each 2PC file because base backups were done wrong. Bleh.

I'm not saying we should go out of our way to detect base backup protocol
violations. Weakened detection of base backup protocol violations is one
drawback of acting on pg_twophase before consistency, but it's less important
than the deviation from standard recovery principles.

On Mon, Jun 02, 2025 at 06:48:46PM -0700, Noah Misch wrote:

The wasShutdown case reaches consistency from the beginning, so I don't see
that as an example of a time we benefit from reading pg_twophase before
reaching consistency. Can you elaborate on that?

What's the benefit you're trying to get by reading pg_twophase before reaching
consistency?

My point is mostly about code complicity and consistency, by using the
same logic (aka reading the contents of pg_twophase) at the same point
in the recovery process and perform some filtering of the contents we
know we will not be able to trust. So I mean to do that once at its
beginning of recovery, where we only compare the names of the 2PC file
names with the XID boundaries in the checkpoint record:
- Discard any files with an XID newer than the next XID. We know that
these would be in WAL anyway, if they replay from a timeline where it
matters.
- Discard any files that are older than the oldest XID. We know that
these files don't matter as 2PC transactions hold the XID horizon.
- Keep the others for later evaluation.

So there's no actual need to check the contents of the files, still
that implies trusting the names of the 2PC files in pg_twophase/.

I can think of one benefit of attempting to read pg_twophase before reaching
consistency. Suppose we can prove that a pg_twophase file will cause an error
by end of recovery, regardless of what WAL contains. It's nice to fail
recovery immediately instead of failing recovery when we reach consistency.
However, I doubt that benefit is important enough to depart from our usual
principle and incur additional storage seeks in order to achieve that benefit.
If recovery will certainly fail, you are going to have a bad day anyway.
Accelerating recovery failure is a small benefit, particularly when we'd
accelerate failure for only a small slice of recovery failure causes.

Well, I kind of disagree here. Failing recovery faster can be
beneficial. It perhaps has less merit since 7ff23c6d277d, meaning
that we should replay less after a failure during crash recovery,
still that seems useful to me if we can do that. That depends on the
amount of trust put in the data read, of course, and if only WAL is
trusted, there's not much that can be done at the beginning of
recovery.

I agree that moving towards a solution where we get rid entirely of
the CLOG lookups in ProcessTwoPhaseBuffer() is what we should aim for,
and actually is there a reason to not just nuke and replace them
something based on the checkpoint record itself?

I don't know what this means.

As of the contents of the last patch posted on this thread, I am
referring to the calls of TransactionIdDidCommit() and
TransactionIdDidAbort() moved from ProcessTwoPhaseBuffer(), which is
called mostly always when WAL is replayed (consistency may or may not
be reached), to RecoverPreparedTransactions(), which is run just
before consistency is marked as such in the system. When
RecoverPreparedTransactions() is called, we're ready to mark the
cluster as OK for writes and WAL has been already fully replayed with
all sanity checks done. CLOG accesses at this stage would not be an
issue.

Wouldn't it be OK in this case to assume that the contents of this
file will be in WAL anyway?

Sure. Meanwhile, if a twophase file is going to be in later WAL, what's the
value in opening the file before we get to that WAL?

True. We could avoid loading some 2PC files if we know that these
could be in WAL when replaying.

There is still one point that I'm really unclear about. Some 2PC
transactions are flushed at checkpoint time, so we will have to trust
the contents of pg_twophase/ at some point. Do you mean to delay that
to happen always when consistency is reached or if we know that we're
starting from a clean state? What I'd really prefer to avoid is
having two code paths in charge of reading the contents of
pg_twophase.

The point I am trying to make is that there has to be a certain level
of trust in the contents of pg_twophase, at some point during replay.
Or, we invent a new mechanism where all the twophase files go through
WAL and remove the need for pg_twophase/ when recovering. For
example, we could have an extra record generated at each checkpoint
with the contents of the 2PC files still pending for commit
(potentially costly if the same transaction persists across multiple
checkpoints as this gets repeated), or something entirely different.
Something like that would put WAL as the sole source of trust by
design.

Or are you seeing things differently? In which case, I am not sure
what's the line you think would be "correct" here (well, you did say
only WAL until consistency is reached), nor do I completely understand
how much 2PC transaction state we should have in shared memory until
consistency is reached. Or perhaps you mean to somehow eliminate more
that? I'm unsure how much this would imply for the existing recovery
mechanisms (replication origin advancing at replay for prepared
transactions may be one area to look at, for example).
--
Michael

On Thu, Jun 05, 2025 at 04:22:48PM +0900, Michael Paquier wrote:

On Mon, Jun 02, 2025 at 06:48:46PM -0700, Noah Misch wrote:

The wasShutdown case reaches consistency from the beginning, so I don't see
that as an example of a time we benefit from reading pg_twophase before
reaching consistency. Can you elaborate on that?

What's the benefit you're trying to get by reading pg_twophase before reaching
consistency?

My point is mostly about code complicity and consistency, by using the
same logic (aka reading the contents of pg_twophase) at the same point
in the recovery process

That's a nice goal.

and perform some filtering of the contents we
know we will not be able to trust. So I mean to do that once at its
beginning of recovery, where we only compare the names of the 2PC file
names with the XID boundaries in the checkpoint record:

- Discard any files with an XID newer than the next XID. We know that
these would be in WAL anyway, if they replay from a timeline where it
matters.

v3-0002-Improve-handling-of-2PC-files-during-recovery.patch has this continue
to emit a WARNING. However, this can happen without any exceptional events in
the backup's history. The message should be LOG or DEBUG if done this early.
This is not new in the patch, and it would be okay to not change it as part of
$SUBJECT. I mention it because, if we scanned pg_twophase after reaching
consistency, an ERROR would be appropriate. (A too-new pg_twophase file then
signifies the backup creator having copied files after the backup stop, a
violation of the backup protocol.)

- Discard any files that are older than the oldest XID. We know that
these files don't matter as 2PC transactions hold the XID horizon.

In contrast, a WARNING is fine here. A too-old file implies one of these
three, each of which counts as an exceptional event:

- an OS crash revived a file after unlink(), since RemoveTwoPhaseFile() does not fsync
- unlink() failed in RemoveTwoPhaseFile()
- backup protocol violation

I agree start-of-recovery can correctly do your list of two discard actions;
they do not require a consistent state. If the patch brings us to the point
that recovery does only those two things with pg_twophase before reaching
consistency, that sounds fine. Doing them that early doesn't sound optimal to
me, but I've not edited that area as much as you have. If it gets the
user-visible behaviors right and doesn't look fragile, I'll be fine with it.

- Keep the others for later evaluation.

So there's no actual need to check the contents of the files, still
that implies trusting the names of the 2PC files in pg_twophase/.

Trusting file names is fine.

I can think of one benefit of attempting to read pg_twophase before reaching
consistency. Suppose we can prove that a pg_twophase file will cause an error
by end of recovery, regardless of what WAL contains.

Thinking about that more, few errors are detectable at that time. Here too,
if your patch achieves some of this while getting the user-visible behaviors
right and not looking fragile, I'll be fine with it.

Wouldn't it be OK in this case to assume that the contents of this
file will be in WAL anyway?

Sure. Meanwhile, if a twophase file is going to be in later WAL, what's the
value in opening the file before we get to that WAL?

True. We could avoid loading some 2PC files if we know that these
could be in WAL when replaying.

If the 2PC data is in future WAL, the file on disk may be a partially-written
file that fails the CRC check. That's a key benefit of not reading the file.

There is still one point that I'm really unclear about. Some 2PC
transactions are flushed at checkpoint time, so we will have to trust
the contents of pg_twophase/ at some point. Do you mean to delay that
to happen always when consistency is reached

Yep.

or if we know that we're
starting from a clean state?

I don't know what "clean slate" means. If "clean state"=wasShutdown, that's
just a special case of "consistency is reached".

What I'd really prefer to avoid is
having two code paths in charge of reading the contents of
pg_twophase.

That's a good goal. I suspect you'll achieve that.

The point I am trying to make is that there has to be a certain level
of trust in the contents of pg_twophase, at some point during replay.
Or, we invent a new mechanism where all the twophase files go through
WAL and remove the need for pg_twophase/ when recovering. For
example, we could have an extra record generated at each checkpoint
with the contents of the 2PC files still pending for commit
(potentially costly if the same transaction persists across multiple
checkpoints as this gets repeated), or something entirely different.
Something like that would put WAL as the sole source of trust by
design.

Adding such a record would be nonstandard and unnecessary. Before reaching
consistency, the standard is that recovery reads and writes what WAL directs
it to read and write. Upon reaching consistency, the server can trust the
entire data directory, including parts that WAL never referenced.

Or are you seeing things differently? In which case, I am not sure
what's the line you think would be "correct" here (well, you did say
only WAL until consistency is reached), nor do I completely understand
how much 2PC transaction state we should have in shared memory until
consistency is reached.

Shared memory should (not "must") omit a GXACT whose prepare_start_lsn lies
beyond the point recovery has reached. In other words, recovery should keep
anachronistic information out of shared memory. For example, one could
imagine a straw man implementation in which, before reaching consistency, we
load each pg_twophase file that does pass its CRC check. I'd dislike that,
because it would facilitate anachronisms involving the
GlobalTransactionData.gid field. We could end up with two GXACT having the
same gid, one being the present (according to recovery progress) instance of
that gid and another being a future instance. The system might not
malfunction today, but I'd consider that fragile. Anachronistic entries might
cause recovery to need more shared memory than max_prepared_transactions has
allocated.

You might find some more-important goal preempts that no-anachronisms goal.

On Fri, Jun 06, 2025 at 03:34:13PM -0700, Noah Misch wrote:

On Thu, Jun 05, 2025 at 04:22:48PM +0900, Michael Paquier wrote:

On Mon, Jun 02, 2025 at 06:48:46PM -0700, Noah Misch wrote:
and perform some filtering of the contents we
know we will not be able to trust. So I mean to do that once at its
beginning of recovery, where we only compare the names of the 2PC file
names with the XID boundaries in the checkpoint record:

- Discard any files with an XID newer than the next XID. We know that
these would be in WAL anyway, if they replay from a timeline where it
matters.

v3-0002-Improve-handling-of-2PC-files-during-recovery.patch has this continue
to emit a WARNING. However, this can happen without any exceptional events in
the backup's history. The message should be LOG or DEBUG if done this early.
This is not new in the patch, and it would be okay to not change it as part of
$SUBJECT. I mention it because, if we scanned pg_twophase after reaching
consistency, an ERROR would be appropriate. (A too-new pg_twophase file then
signifies the backup creator having copied files after the backup stop, a
violation of the backup protocol.)

I need to ponder more about this point, I think.

- Discard any files that are older than the oldest XID. We know that
these files don't matter as 2PC transactions hold the XID horizon.

In contrast, a WARNING is fine here. A too-old file implies one of these
three, each of which counts as an exceptional event:

- an OS crash revived a file after unlink(), since RemoveTwoPhaseFile() does not fsync
- unlink() failed in RemoveTwoPhaseFile()
- backup protocol violation

I agree start-of-recovery can correctly do your list of two discard actions;
they do not require a consistent state. If the patch brings us to the point
that recovery does only those two things with pg_twophase before reaching
consistency, that sounds fine. Doing them that early doesn't sound optimal to
me, but I've not edited that area as much as you have. If it gets the
user-visible behaviors right and doesn't look fragile, I'll be fine with it.

Noted.

So there's no actual need to check the contents of the files, still
that implies trusting the names of the 2PC files in pg_twophase/.

Trusting file names is fine.

Noted, thanks.

or if we know that we're
starting from a clean state?

I don't know what "clean slate" means. If "clean state"=wasShutdown, that's
just a special case of "consistency is reached".

By "clean state", I meant wasShutdown = true, yes. Sorry for the
confusion.

Or are you seeing things differently? In which case, I am not sure
what's the line you think would be "correct" here (well, you did say
only WAL until consistency is reached), nor do I completely understand
how much 2PC transaction state we should have in shared memory until
consistency is reached.

Shared memory should (not "must") omit a GXACT whose prepare_start_lsn lies
beyond the point recovery has reached. In other words, recovery should keep
anachronistic information out of shared memory. For example, one could
imagine a straw man implementation in which, before reaching consistency, we
load each pg_twophase file that does pass its CRC check. I'd dislike that,
because it would facilitate anachronisms involving the
GlobalTransactionData.gid field. We could end up with two GXACT having the
same gid, one being the present (according to recovery progress) instance of
that gid and another being a future instance. The system might not
malfunction today, but I'd consider that fragile. Anachronistic entries might
cause recovery to need more shared memory than max_prepared_transactions has
allocated.

So this comes down to attempting to translate the information from the
2PC files to shared memory at a point earlier than what we could.
Thinking more about this point, there is something that I can see us
potentially do here: how about tracking the file names in shared
memory when we go through them in restoreTwoPhaseData()? That
addresses my point about doing only one scan of pg_twophase/ at the
beginning of recovery. I have not studied in details how doable it
is, but we should be able to delay any calls to
ProcessTwoPhaseBuffer() until we are sure that it is safe to do, which
would in charge of reading the files based on the names we've picked
at the early stages.

RecoverPreparedTransactions() is an easy one, we only call it at the
end of recovery. The sticky point to study is the call of
StandbyRecoverPreparedTransactions() when replaying a shutdown
checkpoint.. Ugh.

You might find some more-important goal preempts that no-anachronisms goal.

Right, thanks.

By the way, what do you think we should do with 0001 at this stage
(your refactoring work with some changes I've sprinkled)? I was
looking at it this morning with fresh eyes and my opinion about it is
that it improves the situation in all these 2PC callers on HEAD now
that we need to deal with fxids for the file names, so that's an
independent piece. Perhaps this should be applied once v19 opens for
business while we sort out the rest that 0002 was trying to cover?
It's not something that we can backpatch safely due to the ABI change
in 2PC callback, unfortunately, just a cleanup piece.

Note: this introduced two calls to FullTransactionIdFromAllowableAt()
in StandbyTransactionIdIsPrepared() and PrepareRedoAdd(), which worried
me a bit on second look and gave me a pause:
- The call in StandbyTransactionIdIsPrepared() should use
AdjustToFullTransactionId(), ReadTwoPhaseFile() calls
TwoPhaseFilePath(), which uses AdjustToFullTransactionId() on HEAD.
- The call in PrepareRedoAdd() slightly worried me, but I think that
we're OK with using FullTransactionIdFromAllowableAt(), as we are in
the case of a 2PC transaction recovered from WAL, with the XID coming
from the file's header. I think that we'd better put an
Assert(InRecovery) in this path, just for safety.

What do you think? 0002 needs more thoughts, so I'm attaching a
slightly-reviewed version of 0001 for now.
--
Michael

On Fri, Jun 20, 2025 at 09:02:00AM +0900, Michael Paquier wrote:

On Fri, Jun 06, 2025 at 03:34:13PM -0700, Noah Misch wrote:

Shared memory should (not "must") omit a GXACT whose prepare_start_lsn lies
beyond the point recovery has reached. In other words, recovery should keep
anachronistic information out of shared memory. For example, one could
imagine a straw man implementation in which, before reaching consistency, we
load each pg_twophase file that does pass its CRC check. I'd dislike that,
because it would facilitate anachronisms involving the
GlobalTransactionData.gid field. We could end up with two GXACT having the
same gid, one being the present (according to recovery progress) instance of
that gid and another being a future instance. The system might not
malfunction today, but I'd consider that fragile. Anachronistic entries might
cause recovery to need more shared memory than max_prepared_transactions has
allocated.

So this comes down to attempting to translate the information from the
2PC files to shared memory at a point earlier than what we could.
Thinking more about this point, there is something that I can see us
potentially do here: how about tracking the file names in shared
memory when we go through them in restoreTwoPhaseData()? That
addresses my point about doing only one scan of pg_twophase/ at the
beginning of recovery. I have not studied in details how doable it
is,

A challenge is that the number of files in pg_twophase can exceed
max_prepared_transactions if we've not yet reached consistency. That happens
with a sequence like this:

- exactly max_prepared_transactions files are in pg_twophase
- backup process copies pg_twophase/$file1, then pauses before the next readdir
- some backend deletes pg_twophase/$file1
- checkpointer creates pg_twophase/$fileN
- backup process wakes up and copies remaining pg_twophase files

That's the only problem coming to mind. If that problem doesn't cancel out
the benefits of scanning pg_twophase early, you may as well do it.

By the way, what do you think we should do with 0001 at this stage
(your refactoring work with some changes I've sprinkled)? I was
looking at it this morning with fresh eyes and my opinion about it is
that it improves the situation in all these 2PC callers on HEAD now
that we need to deal with fxids for the file names, so that's an
independent piece. Perhaps this should be applied once v19 opens for
business while we sort out the rest that 0002 was trying to cover?

That looks harmless.

Note: this introduced two calls to FullTransactionIdFromAllowableAt()
in StandbyTransactionIdIsPrepared() and PrepareRedoAdd(), which worried
me a bit on second look and gave me a pause:
- The call in StandbyTransactionIdIsPrepared() should use
AdjustToFullTransactionId(), ReadTwoPhaseFile() calls
TwoPhaseFilePath(), which uses AdjustToFullTransactionId() on HEAD.
- The call in PrepareRedoAdd() slightly worried me, but I think that
we're OK with using FullTransactionIdFromAllowableAt(), as we are in
the case of a 2PC transaction recovered from WAL, with the XID coming
from the file's header. I think that we'd better put an
Assert(InRecovery) in this path, just for safety.

What do you think?

It's an appropriate level of risk-taking.

On Sun, Jul 06, 2025 at 11:07:11AM -0700, Noah Misch wrote:

A challenge is that the number of files in pg_twophase can exceed
max_prepared_transactions if we've not yet reached consistency. That happens
with a sequence like this:

- exactly max_prepared_transactions files are in pg_twophase
- backup process copies pg_twophase/$file1, then pauses before the next readdir
- some backend deletes pg_twophase/$file1
- checkpointer creates pg_twophase/$fileN
- backup process wakes up and copies remaining pg_twophase files

That's the only problem coming to mind. If that problem doesn't cancel out
the benefits of scanning pg_twophase early, you may as well do it.

Yeah.. That's where a linked link could become useful, and where
we'll need something different for PreparedXactProcs and the fake proc
entries. Not sure about that yet.

By the way, what do you think we should do with 0001 at this stage
(your refactoring work with some changes I've sprinkled)? I was
looking at it this morning with fresh eyes and my opinion about it is
that it improves the situation in all these 2PC callers on HEAD now
that we need to deal with fxids for the file names, so that's an
independent piece. Perhaps this should be applied once v19 opens for
business while we sort out the rest that 0002 was trying to cover?

That looks harmless.

Thanks. I've applied the refactoring piece for now, and marked the CF
entry as returned with feedback. I'm hoping to get back to that for
the September commit fest.
--
Michael