From: David Steele <david@pgmasters.net>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Add basic tests for the low-level backup method.
Date: Fri, 15 Mar 2024 18:23:15 +1300
Message-ID: <89f477dc-c3e5-412e-9765-61c214aa69ab@pgmasters.net> (raw)
In-Reply-To: <ZfOKh0U51IhnIdkp@paquier.xyz>
References: <03e4cf19-fb76-4891-accc-edd429a25b21@pgmasters.net>
<431ecc5d-8c07-4ee1-b05e-3ca536e606bb@pgmasters.net>
<ZeAAPW8h1uxvt4Bj@paquier.xyz>
<4980aba2-9cd4-4bd6-b2e1-e873639ab7ee@pgmasters.net>
<ZfFEfImMw1gtA9zJ@paquier.xyz>
<f20fcc82-dadb-478d-beb4-1e2ffb0ace76@pgmasters.net>
<ZfKgnG09TRKAnDb6@paquier.xyz>
<1c85f906-85d4-4479-9791-062989fec352@pgmasters.net>
<ZfOKh0U51IhnIdkp@paquier.xyz>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: david@pgmasters.net, michael@paquier.xyz, pgsql-hackers@lists.postgresql.org
Subject: Re: Add basic tests for the low-level backup method.
In-Reply-To: <89f477dc-c3e5-412e-9765-61c214aa69ab@pgmasters.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox