Re: thinko in basic_archive.c

On Tue, Nov 8, 2022 at 3:18 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> The call to snprintf() should take care of adding a terminating null byte
> in the right place.

Ah, my bad. MemSet is avoided in v5 patch setting only the first byte.

> > So, IIUC, your point here is what if the copy_file fails to create the
> > temp file when it already exists. With the name collision being a rare
> > scenario, given the pid and timestamp variables, I'm not sure if
> > copy_file can ever fail because the temp file already exists (with
> > errno EEXIST). However, if we want to be extra-cautious, checking if
> > temp file exists with file_exists() before calling copy_file() might
> > help avoid such cases. If we don't want to have extra system call (via
> > file_exists()) to check the temp file existence, we can think of
> > sending a flag to copy_file(src, dst, &is_dst_file_created) and use
> > is_dst_file_created in the shutdown callback. Thoughts?
>
> Presently, if copy_file() encounters a pre-existing file, it should ERROR,
> which will be caught in the sigsetjmp() block in basic_archive_file(). The
> shutdown callback shouldn't run in this scenario.

Determining the "file already exists" error/EEXIST case from a bunch
of other errors in copy_file() is tricky. However, I quickly hacked up
copy_file() by adding elevel parameter, please see the attached
v5-0001.

> I think this cleanup logic should run in both the shutdown callback and the
> sigsetjmp() block, but it should only take action (i.e., deleting the
> leftover temporary file) if the ERROR or shutdown occurs after creating the
> file in copy_file() and before renaming the temporary file to its final
> destination.

Please see attached v5 patch set.

If the direction seems okay, I'll add a CF entry.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com