Re: Fix crash during recovery when redo segment is missing - Mailing list pgsql-hackers

Thank you for the detailed feedback.

> > Your tests are using a busy loop with usleep which nowadays is
> > considered as bad practice.  There 3 of such places, and I think the
> > first two of them
> > can be replaced with injection point wait.
>
> (This fell off my radar, apologies about that.)
>
> The problem with this test is not related to its use of sleeps, which
> is perfectly fine to check the start or end timings of a checkpoint
> depending on the contents of the logs.  It has two issues.
>
> One first issue is that the test is unnecessary long, taking more than
> 30s to finish because it relies on checkpoint_timeout to kick a
> checkpoint.  This could use a background psql object to kick a
> checkpoint to accelerate the whole.  So the test is sitting idle for a
> long time, doing nothing.
>
> Your intuition about injection points is right, though, but it points
> to a second problem: the test is not reliable because we could finish
> the checkpoint *before* the WAL segment is switched, and we expect the
> WAL segment switch to happen while the checkpoint is processing.  If
> you want to make that deterministic, having a wait in the middle of
> checkpointing would make the test actually test what it should.  In
> this case the test would randomly die on its "redo record wal file is
> the same" message.  That's OK to prove the point of the initial patch,
> but it's not acceptable for a test that could be added to the tree.
>
> An update of src/test/recovery/meson.build to add the new test is
> also required.

I will work on improving the test accordingly and include the changes
in the next version.

> Anyway, I think that the patch is overdoing it in issuing a PANIC in
> this case, going backwards with the other thread from [0]: a FATAL
> would be perfectly OK, like in the backup_label path because your
> manipulations of WAL segment missing are indeed possible, as the test
> posted is proving.  And there have been many arguments in the past
> about performing recovery without a backup_label, as well.  And that
> would make the test something that we could use, because no backtrace
> on buildfarm hosts or disk bloat.

The main reason I chose PANIC is that when the ReadCheckpointRecord()
fails, we already use PANIC for the error message ‘could not locate a
valid checkpoint record…’ in the no_backup_label case, whereas the
similar flow with backup_label uses FATAL. I am not entirely sure why
this difference exists. I looked into it but couldn’t find much. If we
decide to change this patch to use FATAL for ‘could not find redo
location…’, should we also consider changing the existing PANIC to
FATAL for consistency?

> > 4) There are comments from Robert, which are not covered [1].
> >
> > [0] https://www.postgresql.org/message-id/20231023232145.cmqe73stvivsmlhs%40awork3.anarazel.de
> > [1] https://www.postgresql.org/message-id/CA%2BTgmob1x7HMcWAb%3D6ep2cBuWuwpT-p9E7EmQegWFu6E8nKHeg%40mail.gmail.com
>
> I get the argument about the spaghetti code in general, however the
> code in question here deals with the WAL replay initialization, for a
> backup_label vs a non-backup_label path.  Perhaps I'm more used to
> this area than others, but it's not that much pasta to me.

Yes. As noted in my initial email, this patch is focused solely on
fixing the crash issue. It does not address error handling in
StartupXLOG(), CreateCheckPoint(), or involve any broader code
cleanup.

> I don't see a point in moving the memcpy() and the wasShutdown parts
> as proposed in the patch, by the way.  The PANIC would block things
> in its else block.  Let's limit outselves to the extra ReadRecord()
> check for the redo record when we find a checkpoint record.

I agree and will take care in the next patch.

Best Regards,
Nitin Jadhav
Azure Database for PostgreSQL
Microsoft