Re: Add basic tests for the low-level backup method.

On 3/15/24 12:38, Michael Paquier wrote:
> On Fri, Mar 15, 2024 at 09:40:38AM +1300, David Steele wrote:
>> Is the missing test in meson the reason we did not see test failures for
>> Windows in CI?
>
> The test has to be listed in src/test/recovery/meson.build or the CI
> would ignore it.

Right -- I will keep this in mind for the future.

>>> The second LOG is something that can be acted on. I've added some
>>> debugging to the parsing of the backup_label file in the backend, and
>>> noticed that the first fscanf() for START WAL LOCATION is failing
>>> because the last %c is detected as \r rather than \n. Tweaking the
>>> contents stored from pg_backend_stop() with a sed won't help, because
>>> the issue is that we write the CRLFs with append_to_file, and the
>>> startup process cannot cope with that. The simplest method I can
>>> think of is to use binmode, as of the attached.
>>
>> Yeah, that makes sense.
>
> I am wondering if there is a better trick here that would not require
> changes in the backend to make the backup_label parsing more flexible,
> though.

Well, this is what we recommend in the docs, i.e. using bin mode to save
backup_label, so it seems OK to me.

>>> I am attaching an updated patch with all that fixed, which is stable
>>> in the CI and any tests I've run. Do you have any comments about
>>
>> These changes look good to me. Sure wish we had an easier to way to test
>> commits in the build farm.
>
> That's why these tests are not that easy, they can be racy. I've run
> the test 5~10 times in the CI this time to gain more confidence, and
> saw zero failures with the stability fixes in place including Windows.
> I've applied it now, as I can still monitor the buildfarm for a few
> more days. Let's see what happens, but that should be better.

At least sidewinder is happy now -- and the build farm in general as far
as I can see.

Thank you for your help on this!
-David