From: Nathan Bossart <nathandbossart@gmail.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: robertmhaas@gmail.com
Cc: pgsql-hackers@postgresql.org
Subject: Re: thinko in basic_archive.c
Date: Mon, 7 Nov 2022 13:48:09 -0800
Message-ID: <20221107214809.GB632101@nathanxps13> (raw)
In-Reply-To: <CALj2ACVgsH_HwgpWp1ULHMVudr8jiy2FFXyHA+QxO-Vt3WkzMg@mail.gmail.com>
References: <CALj2ACXJjR+4qmEunj4RtBE4rGKkW8S9BLp7qfx+62sbBYkW-g@mail.gmail.com>
<20221020.102647.746666963360821887.horikyota.ntt@gmail.com>
<CALj2ACUi_bzt=U7Nx5A2H0i-iK3G3EeJoe7MMKiNERmj6O3A=w@mail.gmail.com>
<20221021.141346.1491472262189507263.horikyota.ntt@gmail.com>
<CALj2ACULgfF_W4MitGPG7iw7KQyFstVbpQiB_B3x=fJZ4jWHvQ@mail.gmail.com>
<20221105214651.GC137360@nathanxps13>
<CALj2ACVgsH_HwgpWp1ULHMVudr8jiy2FFXyHA+QxO-Vt3WkzMg@mail.gmail.com>
On Mon, Nov 07, 2022 at 04:53:35PM +0530, Bharath Rupireddy wrote:
> 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).
The call to snprintf() should take care of adding a terminating null byte
in the right place.
> BTW, I couldn't find the 80 existing instances, can you please let me
> know your search keyword?
grep "\[0\] = '\\\0'" src -r
> 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.
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.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, bharath.rupireddyforpostgres@gmail.com, horikyota.ntt@gmail.com, robertmhaas@gmail.com
Subject: Re: thinko in basic_archive.c
In-Reply-To: <20221107214809.GB632101@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox