Failing assertion while taking a restartpoint during crash recovery

On Sat, Aug 15, 2026 at 4:57 PM Imran Zaheer <imran.zhir@gmail.com> wrote:

Hi

While doing some benchmarking related to my work with the recovery
pipelining [1], I found that the checkpointer fails when trying to
take a time-based restartpoint during the crash recovery.

Right now I am facing this assertion failure.

Thanks for the report! I was also able to reproduce the assertion failure.

It looks like the checkpointer is not aware of whether hot standby
initialization was done during startup, which happens only in the case
of ArchiveRecoveryRequested [2]. In this specific case, the
checkpointer assumes hot standby initialization would have been done,
so it tries to call TruncateSUBTRANS(). A simple fix is to add
ArchiveRecoveryRequested to recovery shared memory XLogRecoveryCtl;
then the checkpointer will simply skip TruncateSUBTRANS if archive
recovery was not requested. I have attached my patch below.

I think it would be better and more robust to check directly whether
StartupSUBTRANS() has already been called, rather than checking
ArchiveRecoveryRequested, as in the attached patch.

Thoughts?

Regards,

--
Fujii Masao

Hi,

I agree that tracking whether StartupSUBTRANS() has actually been
called is more direct and robust. I don't have any strong objection to
your approach; your fix looks reasonable to me.

My initial concern was more about having too many recovery state
management variables at this point, i.e., ArchiveRecoveryRequested,
InArchiveRecovery, EnableHotStandby, StandbyMode,
StandbyModeRequested, etc. I just wanted to keep the context close to
the existing states and did not want to create a new state for this
specific bug.

Thanks,
Imran Zaheer

On Sat, Aug 15, 2026 at 9:28 PM Imran Zaheer <imran.zhir@gmail.com> wrote:

I agree that tracking whether StartupSUBTRANS() has actually been
called is more direct and robust. I don't have any strong objection to
your approach; your fix looks reasonable to me.

Thanks for the review!

I've applied the cosmetic changes to the patch and created patches
for the older branches.

My initial concern was more about having too many recovery state
management variables at this point, i.e., ArchiveRecoveryRequested,
InArchiveRecovery, EnableHotStandby, StandbyMode,
StandbyModeRequested, etc. I just wanted to keep the context close to
the existing states and did not want to create a new state for this
specific bug.

I agree that adding such new recovery state variables basically would
not be a good idea. But, I don't think the flag introduced by this patch
falls into that category. It simply tracks whether pg_subtrans has been
started during recovery, so I don't have much concern about adding it.

Regards,

--
Fujii Masao

On Sun, Aug 16, 2026 at 02:14:32AM +0900, Fujii Masao wrote:

I agree that adding such new recovery state variables basically would
not be a good idea. But, I don't think the flag introduced by this patch
falls into that category. It simply tracks whether pg_subtrans has been
started during recovery, so I don't have much concern about adding it.

I am not concerned with the addition of this new flag. You are
outsourcing a check based on ArchiveRecoveryRequested *and*
EnableHotStandby to rely on a single state once we have called
StartupSUBTRANS(), so you are simplifying the recovery logic. This
argument counts as a +1 here.

What I do see an an issue is that the script used in the report is
mentioned as a link on github. If this content is lost, we would also
lose the history related to this thread. Attaching it here for the
sake of the archives. That's a better practice in general.

Fujii-san, how did you reproduce the problem? Just by reusing the
script? If we've failed detected this defect for so many years,
perhaps this warrants a test? If it proves to be expensive in terms
of cycles, I'd be a bit meh. Perhaps it can be made cheap, even if I
suspect that injection points cannot be used: we need something that
would be set up at very early startup.

One question that has not been raised is if exposing this new flag has
any value for pg_stat_recovery. This could not be a v19 item, perhaps
a v20 item. I don't see how useful it would be as it is no better
than connections getting accepted by the postmaster once a hot standby
is set up (flag would be always true at SQL level).
--
Michael

On Sun, Aug 16, 2026 at 10:21 AM Michael Paquier <michael@paquier.xyz> wrote:

I am not concerned with the addition of this new flag. You are
outsourcing a check based on ArchiveRecoveryRequested *and*
EnableHotStandby to rely on a single state once we have called
StartupSUBTRANS(), so you are simplifying the recovery logic. This
argument counts as a +1 here.

Thanks for the review!

Fujii-san, how did you reproduce the problem? Just by reusing the
script?

TBH, I didn't see the attached script. Based on Imran's explanation,
I came up with the following procedure to reproduce the issue.

------------------------------------------
initdb -D data
cat <<EOF >> data/postgresql.conf
checkpoint_timeout = 30s
wal_keep_size = 100GB
full_page_writes = off
EOF
pg_ctl -D data start
pg_basebackup -D test -c fast

psql -c "CREATE TABLE t (i uuid primary key, j uuid unique)"
for tmp in $(seq 1 4); do
psql -c "INSERT INTO t SELECT uuidv7(), uuidv4() FROM
generate_series(1, 5000000)" &
done

(*) At this point, I usually wait until a sufficient amount of WAL
has been generated, rather than waiting for all four INSERTs to
complete, since they take quite a while.

pg_ctl -D data -m i stop
mv data/pg_wal/0000000100000000000000* test/pg_wal/
pg_ctl -D test start
------------------------------------------

If we've failed detected this defect for so many years,
perhaps this warrants a test? If it proves to be expensive in terms
of cycles, I'd be a bit meh.

The test would need to trigger a restartpoint during crash recovery,
which I'm afraid could be expensive.

One question that has not been raised is if exposing this new flag has
any value for pg_stat_recovery.

I'm not sure how useful it would be to expose this flag to end users...

Regards,

--
Fujii Masao

On Sun, Aug 16, 2026 at 2:14 AM Fujii Masao <masao.fujii@gmail.com> wrote:

I've applied the cosmetic changes to the patch and created patches
for the older branches.

Regarding the patch for v14, since the checkpointer is not started
during crash recovery in v14, it never runs a restartpoint during
crash recovery, so the reported issue cannot occur in the usual way.

But, a fast shutdown during crash recovery does run a restartpoint,
which can trigger the same assertion failure in v14. So the bug fix
should be backpatched to v14 as well, I think.

Regards,

--
Fujii Masao

Hi

I was looking into the patch, and I was wondering if we could make use
of the following small changes.

* Instead of setting the shared state in StartupXLOG, why not set it
in the StartupSUBTRANS itself?
```
@@ -6234,7 +6234,6 @@ StartupXLOG(void)
                         * during recovery and need not be started yet.
                         */
                        StartupSUBTRANS(oldestActiveXID);
-                       SetRecoverySubtransInitialized();
```

* This way we can also add an assertion that subtrans shouldn't
already be initialized.
```
@@ -307,6 +307,8 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
LWLock *prevlock = NULL;
LWLock *lock;

+       Assert(!RecoverySubtransInitialized());
+
        /*
         * Since we don't expect pg_subtrans to be valid across crashes, we
         * initialize the currently-active page(s) to zeroes during startup.
@@ -339,6 +341,8 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
        }

        LWLockRelease(lock);
+
+       SetRecoverySubtransInitialized();
 }
 ```

* Also we can add an assertion while truncating subtrans for safety.

```
@@ -405,6 +409,8 @@ TruncateSUBTRANS(TransactionId oldestXact)
{
int64 cutoffPage;

+       Assert(RecoverySubtransInitialized());
+
```

* There is is another subtrans call under StartupXLOG that can also be improved
```
@@ -6518,7 +6517,7 @@ StartupXLOG(void)
         * Start up subtrans, if not already done for hot standby.  (commit
         * timestamps are started below, if necessary.)
         */
-       if (standbyState == STANDBY_DISABLED)
+       if (standbyState == STANDBY_DISABLED && !RecoverySubtransInitialized())
                StartupSUBTRANS(oldestActiveXID);
```

* Other than that we also call TruncateSUBTRANS() while creating a
checkpoint; maybe we can also improve the guard here, although the
assertion under TruncateSUBTRANS could be enough?

@@ -7879,7 +7878,7 @@ CreateCheckPoint(int flags)
         * in subtrans.c).  During recovery, though, we mustn't do this because
         * StartupSUBTRANS hasn't been called yet.
         */
-       if (!RecoveryInProgress())
+       if (!RecoveryInProgress() && RecoverySubtransInitialized())
                TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning());

Thoughts?

Thanks,
Imran Zaheer

On Tue, Aug 18, 2026 at 1:37 PM Imran Zaheer <imran.zhir@gmail.com> wrote:

I was looking into the patch

Thanks!

* Instead of setting the shared state in StartupXLOG, why not set it
in the StartupSUBTRANS itself?

*If* we take this approach, I think the flag should no longer mean that hot
standby initialization has started pg_subtrans, but rather that
StartupSUBTRANS() has completed. We would therefore also need to rename
the flag and update the comments accordingly.

For a bug fix in stable branches, I would prefer to keep the change as
small as possible. Also, subtrans.c says that there are no XLOG
interactions, so having it update shared recovery state in
xlogrecovery.c feels a bit odd to me. I therefore prefer the current
patch and approach.

* Other than that we also call TruncateSUBTRANS() while creating a
checkpoint; maybe we can also improve the guard here, although the
assertion under TruncateSUBTRANS could be enough?

@@ -7879,7 +7878,7 @@ CreateCheckPoint(int flags)
* in subtrans.c).  During recovery, though, we mustn't do this because
* StartupSUBTRANS hasn't been called yet.
*/
-       if (!RecoveryInProgress())
+       if (!RecoveryInProgress() && RecoverySubtransInitialized())
TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning());

I don't think this extra check is necessary. If !RecoveryInProgress(),
StartupSUBTRANS() should already have been called, so the existing
guard should be sufficient.

Regards,

--
Fujii Masao