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.rupireddyforpostg...@gmail.com> wrote: > On Thu, Aug 4, 2022 at 11:59 AM Bharath Rupireddy > <bharath.rupireddyforpostg...@gmail.com> wrote: > > > > On Sun, Jul 31, 2022 at 8:36 PM mahendrakar s > > <mahendrakarfo...@gmail.com> wrote: > > > > > >> On Mon, 25 Jul 2022 at 16:42, Bharath Rupireddy < > bharath.rupireddyforpostg...@gmail.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/ >