Thread: Re: Issues with 2PC at recovery: CLOG lookups and GlobalTransactionData

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]: https://www.postgresql.org/message-id/20250117005221.05.nmisch@google.com
> [2]: https://www.postgresql.org/message-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 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.