Re: thinko in basic_archive.c

On Sun, Nov 6, 2022 at 3:17 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> On Fri, Oct 21, 2022 at 09:30:16PM +0530, Bharath Rupireddy wrote:
> > On Fri, Oct 21, 2022 at 10:43 AM Kyotaro Horiguchi
> > <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> >> Thanks, but we don't need to wipe out the all bytes. Just putting \0
> >> at the beginning of the buffer is sufficient.
> >
> > Nah, that's not a clean way IMO.
>
> Why not? This is a commonly-used technique. I see over 80 existing useѕ
> in PostgreSQL. Plus, your shutdown callback only checks for the first
> byte, anyway.
>
> + if (tempfilepath[0] == '\0')
> + return;

The tempfile name can vary in size for the simple reason that a pid
can be of varying digits - for instance, tempfile name is 'foo1234'
(pid being 1234) and it becomes '\'\0\'oo1234' if we just reset the
first char to '\0' and say pid wraparound occurred, now the it becomes
'bar5674' (pid being 567).

BTW, I couldn't find the 80 existing instances, can you please let me
know your search keyword?

> As noted upthread [0], I think we should be cautious to only remove the
> temporary file if we know we created it. I still feel that trying to add
> this cleanup logic to basic_archive is probably more trouble than it's
> worth, but if there is a safe and effective way to do so, I won't object.
>
> [0] https://postgr.es/m/20221015211026.GA1821022%40nathanxps13

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?

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