Обсуждение: pg_rewind with cascade standby doesn't work well

Hi,

> The attached patch updates minRecoveryPoint and minRecoveryPointTLI at this point by mimicking
CreateEndOfRecoveryRecord().
> With this patch, you can run pg_rewind with cascade standby immediately. (without waiting for checkpointing)

Many thanks for submitting the patch. I added it to the nearest open
commitfest [1].

IMO a test is needed that makes sure no one is going to break this in
the future.

[1]: https://commitfest.postgresql.org/45/4559/

-- 
Best regards,
Aleksander Alekseev

On Mon, Sep 11, 2023 at 07:04:30PM +0300, Aleksander Alekseev wrote:
> Many thanks for submitting the patch. I added it to the nearest open
> commitfest [1].
>
> IMO a test is needed that makes sure no one is going to break this in
> the future.

You definitely need more complex test scenarios for that.  If you can
come up with new ways to make the TAP tests of pg_rewind mode modular
in handling more complicated node setups, that would be a nice
addition, for example.

> [1]: https://commitfest.postgresql.org/45/4559/

@@ -7951,6 +7951,15 @@ xlog_redo(XLogReaderState *record)
             ereport(PANIC,
                     (errmsg("unexpected timeline ID %u (should be %u) in end-of-recovery record",
                             xlrec.ThisTimeLineID, replayTLI)));
+        /*
+         * Update the control file so that crash recovery can follow the timeline
+         * changes to this point.
+         */
+        LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
+        ControlFile->minRecoveryPoint = lsn;
+        ControlFile->minRecoveryPointTLI = xlrec.ThisTimeLineID;

This patch is at least incorrect in its handling of crash recovery,
because these two should *never* be set in this case as we want to
replay up to the end of WAL.  For example, see xlog.c or the top of
xlogrecovery.c about the assumptions behind these variables:
    /* crash recovery should always recover to the end of WAL */
    ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
    ControlFile->minRecoveryPointTLI = 0;

If an end-of-recovery record is replayed during crash recovery, these
assumptions are plain broken.

One thing that we could consider is to be more aggressive with
restartpoints when replaying this record for a standby, see a few
lines above the lines added by your patch, for example.  And we could
potentially emulate a post-promotion restart point to get a refresh of
the control file as it should, with the correct code paths involved in
the updates of minRecoveryPoint when the checkpointer does the job.
--
Michael

On Wed, Sep 20, 2023 at 11:46:45AM +0900, Kuwamura Masaki wrote:
> I also found a bug (maybe). If we call `CreateRestartPoint()` during
> crash-recovery, the assertion fails at ComputeXidHorizon() in procarray.c.
> It's inherently orthogonal to the problem I already reported. So you can
> reproduce this at HEAD with this procedure.
>
> 1. Start primary and standby server
> 2. Modify checkpoint_timeout to 1h on standby
> 3. Insert 10^10 records and concurrently run CHECKPOINT every second on
> primary
> 4. Do an immediate stop on both standby and primary at the end of the insert
> 5. Modify checkpoint_timeout to 30 on standby
> 6. Remove standby.signal on standby
> 7. Restart standby (it will start crash-recovery)
> 8. Assertion failure is raised shortly
>
> I think this is because `TruncateSUBTRANS();`  in `CreateRestartPoint()` is
> called but `StartupSUBTRANS()` isn't called yet. In `StartupXLOG()`, we
> call
> `StartupSUBTRANS()` if `(ArchiveRecoveryRequested && EnableHotStandby)`.
> However, in `CreateRestartPoint()`, we call `TruncateSUBTRANS()` if
> `(EnableHotStandby)`. I guess the difference causes this bug. The latter
> possibly be called even crash-recovery while former isn't.
> The attached patch 0002 fixes it. I think we could discuss about this bug
> in
> another thread if needed.

This is a known issue.  I guess that the same as this thread and this
CF entry:
https://commitfest.postgresql.org/44/4244/
https://www.postgresql.org/message-id/flat/ZArVOMifjzE7f8W7@paquier.xyz
--
Michael

On 2023/09/20 12:04, Michael Paquier wrote:
> This is a known issue.  I guess that the same as this thread and this
> CF entry:
> https://commitfest.postgresql.org/44/4244/
> https://www.postgresql.org/message-id/flat/ZArVOMifjzE7f8W7@paquier.xyz

I think this is a separate issue, and we should still use Kuwamura-san's patch
even after the one you posted on the thread gets accepted. BTW, I was able to
reproduce the assertion failure Kuwamura-san reported, even after applying
your latest patch from the thread.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Hi,

> >> IMO a test is needed that makes sure no one is going to break this in
> >> the future.
> >
> > You definitely need more complex test scenarios for that.  If you can
> > come up with new ways to make the TAP tests of pg_rewind mode modular
> > in handling more complicated node setups, that would be a nice
> > addition, for example.
>
> I'm sorry for lacking tests. For now, I started off with a simple test
> that cause the problem I mentioned. The updated WIP patch 0001 includes
> the new test for pg_rewind.

Many thanks for a quick update.

> And also, I'm afraid that I'm not sure what kind of tests I have to make
> for fix this behavior. Would you mind giving me some advice?

Personally I would prefer not to increase the scope of work. Your TAP
test added in 0001 seems to be adequate.

> BTW, I was able to
> reproduce the assertion failure Kuwamura-san reported, even after applying
> your latest patch from the thread.

Do you mean that the test fails or it doesn't but there are other
steps to reproduce the issue?

-- 
Best regards,
Aleksander Alekseev

On Tue, Sep 26, 2023 at 06:44:50PM +0300, Aleksander Alekseev wrote:
>> And also, I'm afraid that I'm not sure what kind of tests I have to make
>> for fix this behavior. Would you mind giving me some advice?
>
> Personally I would prefer not to increase the scope of work. Your TAP
> test added in 0001 seems to be adequate.

Yeah, agreed.  I'm OK with what you are proposing, basically (the
test could be made a bit cheaper actually).

         /*
-         * For Hot Standby, we could treat this like a Shutdown Checkpoint,
-         * but this case is rarer and harder to test, so the benefit doesn't
-         * outweigh the potential extra cost of maintenance.
+         * For Hot Standby, we could treat this like an end-of-recovery checkpoint
          */
+        RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT);

I don't understand what you want to change here.  Archive recovery and
crash recovery are two different things, still this code would be
triggered even if there is no standby.signal, aka the node is not a
standby.  Why isn't this stuff conditional?

>> BTW, I was able to
>> reproduce the assertion failure Kuwamura-san reported, even after applying
>> your latest patch from the thread.
>
> Do you mean that the test fails or it doesn't but there are other
> steps to reproduce the issue?

I get it as Fujii-san testing the patch from [1], still failing the
test from [2]:
[1]: https://www.postgresql.org/message-id/ZArVOMifjzE7f8W7@paquier.xyz
[2]: https://www.postgresql.org/message-id/CAMyC8qryE7mKyvPvGHCt5GpANAmp8sS_tRbraqXcPBx14viy6g@mail.gmail.com

I would be surprised, actually, because the patch from [1] would cause
step 7 of the test to fail: the patch causes standby.signal or
recovery.signal to be required.  Anyway, this specific issue, if any,
had better be discussed on the other thread.  I need to address a few
comments there as well and was planning to get back to it.  It is
possible that I've missed something on the other thread with the
restrictions I was proposing in the latest version of the patch.

For this thread, let's focus on the pg_rewind case and how we want to
treat these records to improve the cascading case.
--
Michael