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

Hi Bharath,
I reviewed your patch. Minor comments.

1. Why are we not using durable_unlink instead of unlink to remove the
partial tmp files?

2. Below could be a simple if(shouldcreatetempfile){} else{} as in error
case we need to return NULL.
+ if (errno != ENOENT || !shouldcreatetempfile)
+ {
+ dir_data->lasterrno = errno;
+ return NULL;
+ }
+ else if (shouldcreatetempfile)
+ {
+ /*
+ * Actual file doesn't exist. Now, create a temporary file pad it
+ * and rename to the target file. The temporary file may exist from
+ * the last failed attempt (failed during partial padding or
+ * renaming or some other). If it exists, let's play safe and
+ * delete it before creating a new one.
+ */
+ snprintf(tmpsuffixpath, MAXPGPATH, "%s.tmp", targetpath);
+
+ if (dir_existsfile(tmpsuffixpath))
+ {
+ if (unlink(tmpsuffixpath) != 0)
+ {
+ dir_data->lasterrno = errno;
+ return NULL;
+ }
+ }
+
+ fd = open(tmpsuffixpath, flags | O_CREAT, pg_file_create_mode);
+ if (fd < 0)
+ {
+ dir_data->lasterrno = errno;
+ return NULL;
+ }
+ }

On Mon, 8 Aug 2022 at 11:59, Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:

> On Thu, Aug 4, 2022 at 11:59 AM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > On Sun, Jul 31, 2022 at 8:36 PM mahendrakar s
> > <mahendrakarforpg(at)gmail(dot)com> wrote:
> > >
> > >> On Mon, 25 Jul 2022 at 16:42, Bharath Rupireddy <
> bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > >> Here's the v3 patch after rebasing.
> > >>
> > >> I just would like to reiterate the issue the patch is trying to solve:
> > >> At times (when no space is left on the device or when the process is
> > >> taken down forcibly (VM/container crash)), there can be leftover
> > >> uninitialized .partial files (note that padding i.e. filling 16MB WAL
> > >> files with all zeros is done in non-compression cases) due to which
> > >> pg_receivewal fails to come up after the crash. To address this, the
> > >> proposed patch makes the padding 16MB WAL files atomic (first write a
> > >> .temp file, pad it and durably rename it to the original .partial
> > >> file, ready to be filled with received WAL records). This approach is
> > >> similar to what core postgres achieves atomicity while creating new
> > >> WAL file (see how XLogFileInitInternal() creates xlogtemp.%d file
> > >> first and then how InstallXLogFileSegment() durably renames it to
> > >> original WAL file).
> > >>
> > >> Thoughts?
> > >
> > > Hi Bharath,
> > >
> > > Idea to atomically allocate WAL file by creating tmp file and renaming
> it is nice.
> >
> > Thanks for reviewing it.
> >
> > > I have one question though:
> > > How is partially temp file created will be cleaned if the VM crashes
> or out of disk space cases? Does it endup creating multiple files for
> every VM crash/disk space during process of pg_receivewal?
> > >
> > > Thoughts?
> >
> > It is handled in the patch, see [1].
> >
> > Attaching v4 patch herewith which now uses the temporary file suffix
> > '.tmp' as opposed to v3 patch '.temp'. This is just to be in sync with
> > other atomic file write codes in the core - autoprewarm,
> > pg_stat_statement, slot, basebacup, replorigin, snapbuild, receivelog
> > and so on.
> >
> > Please review the v4 patch.
>
> I've done some more testing today (hacked the code a bit by adding
> pg_usleep(10000L); in pre-padding loop and crashing the pg_receivewal
> process to produce the warning [1]) and found that the better place to
> remove ".partial.tmp" leftover files is in FindStreamingStart()
> because there we do a traversal of all the files in target directory
> along the way to remove if ".partial.tmp" file(s) is/are found.
>
> Please review the v5 patch further.
>
> [1] pg_receivewal: warning: segment file
> "0000000100000006000000B9.partial" has incorrect size 15884288,
> skipping
>
> --
> Bharath Rupireddy
> RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/
>