On Tue, Jan 4, 2022 at 1:40 AM SATYANARAYANA NARLAPURAM <satyanarlapu...@gmail.com> wrote: > > On Sun, Jan 2, 2022 at 11:56 PM Michael Paquier <mich...@paquier.xyz> 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.
v2-0001-Avoid-partially-padded-files-under-pg_receivewal-.patch
Description: Binary data
