Re: Failing assertion while taking a restartpoint during crash recovery

[Imran Zaheer]

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