Message-ID: <89f477dc-c3e5-412e-9765-61c214aa69ab@pgmasters.net>
Date: Fri, 15 Mar 2024 18:23:15 +1300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add basic tests for the low-level backup method.
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@lists.postgresql.org
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>
Content-Language: en-US
From: David Steele <david@pgmasters.net>
In-Reply-To: <ZfOKh0U51IhnIdkp@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/89f477dc-c3e5-412e-9765-61c214aa69ab%40pgmasters.net>
Precedence: bulk

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