Subject: Re: pg_receivewal fail to streams when the partial file to write is not fully initialized present in the wal receiver directory

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 9 Apr 2022 18:03:01 +0530

On Tue, Jan 4, 2022 at 1:40 AM SATYANARAYANA NARLAPURAM
<[email-masked]> wrote:
>
> On Sun, Jan 2, 2022 at 11:56 PM Michael Paquier <[email-masked]> wrote:
>>
>> On Sun, Jan 02, 2022 at 09:27:43PM -0800, SATYANARAYANA NARLAPURAM wrote:
>> > I noticed that pg_receivewal fails to stream when the partial file to write
>> > is not fully initialized and fails with the error message something like
>> > below. This requires an extra step of deleting the partial file that is not
>> > fully initialized before starting the pg_receivewal. Attaching a simple
>> > patch that creates a temp file, fully initialize it and rename the file to
>> > the desired wal segment name.
>>
>> Are you referring to the pre-padding when creating a new partial
>> segment, aka when we write chunks of XLOG_BLCKSZ full of zeros until
>> the file is fully created?  What kind of error did you see?  I guess
>> that a write() with ENOSPC would be more likely, but you got a
>> different problem?
>
> I see two cases, 1/ when no space  is left on the device and 2/ when the process is taken down forcibly (a VM/container crash)

Yeah, these cases can occur leaving uninitialized .partial files which
can be a problem for both pg_receivewal and pg_basebackup that uses
dir_open_for_write (CreateWalDirectoryMethod).

>>   I don't disagree with improving such cases, but we
>> should not do things so as there is a risk of leaving behind an
>> infinite set of segments in case of repeated errors
>
> Do you see a problem with the proposed patch that leaves the files behind, at least in my testing I don't see any files left behind?

With the proposed patch, it doesn't leave the unpadded .partial files.
Also, the v2 patch always removes a leftover .partial.temp file before
it creates a new one.

>> , and partial
>> segments are already a kind of temporary file.
>
>
> if the .partial file exists with not zero-padded up to the wal segment size (WalSegSz), then open_walfile fails with the below error. I have two options here, 1/ to continue padding the existing partial file and let it zero up to WalSegSz , 2/create a temp file as I did in the patch. I thought the latter is safe because it can handle corrupt cases as described below. Thoughts?

The temp file approach looks clean.

>> -       if (dir_data->sync)
>> +       if (shouldcreatetempfile)
>> +       {
>> +               if (durable_rename(tmpsuffixpath, targetpath) != 0)
>> +               {
>> +                       close(fd);
>> +                       unlink(tmpsuffixpath);
>> +                       return NULL;
>> +               }
>> +       }
>>
>> durable_rename() does a set of fsync()'s, but --no-sync should not
>> flush any data.

Fixed this in v2.

Another thing I found while working on this is the way the
dir_open_for_write does padding - it doesn't retry in case of partial
writes of blocks of size XLOG_BLCKSZ, unlike what core postgres does
with pg_pwritev_with_retry in XLogFileInitInternal. Maybe
dir_open_for_write should use the same approach. Thoughts?

I fixed couple of issues with v1 (which was using static local
variables in dir_open_for_write, not using durable_rename/rename for
dir_data->sync true/false cases, not considering compression method
none while setting shouldcreatetempfile true), improved comments and
added commit message.

Please review the v2 further.

Regards,
Bharath Rupireddy.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Mon, 11 Apr 2022 15:13:14 +0900 (JST)

At Sat, 9 Apr 2022 18:03:01 +0530, Bharath Rupireddy <[email-masked]> wrote in 
> > Do you see a problem with the proposed patch that leaves the files behind, at least in my testing I don't see any files left behind?

I guess that Michael took this patch as creating a temp file name such
like "tmp.n" very time finding an incomplete file.

> >> segments are already a kind of temporary file.

I'm not sure this is true for pg_receivewal case.  The .partial file
is not a temporary file but the current working file for the tool.

> > if the .partial file exists with not zero-padded up to the wal segment size (WalSegSz), then open_walfile fails with the below error. I have two options here, 1/ to continue padding the existing…

I think this patch shouldn't involve pg_basebackup.  I agree to Cary
that deleting the erroring file should be fine.

We already "skipping" (complete = non-.partial) WAL files with a wrong
size in FindStreamingStart so we can error-out with suggesting a hint.

$ pg_receivewal -D xlog -p 5432 -h /tmp
pg_receivewal: error: segment file "0000000100000022000000F5.partial" has incorrect size 8404992
hint: You can continue after removing the file.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Mon, 11 Apr 2022 15:16:25 +0900 (JST)

Sorry for the terrible typos..

> > Do you see a problem with the proposed patch that leaves the files behind, at least in my testing I don't see any files left behind?

I guess that Michael took this patch as creating a temp file with a
name such like "tmp.n" every time finding an incomplete file.

regards.