Date: Mon, 7 Nov 2022 13:48:09 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, robertmhaas@gmail.com,
	pgsql-hackers@postgresql.org
Subject: Re: thinko in basic_archive.c
Message-ID: <20221107214809.GB632101@nathanxps13>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CALj2ACVgsH_HwgpWp1ULHMVudr8jiy2FFXyHA+QxO-Vt3WkzMg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20221107214809.GB632101%40nathanxps13>
Precedence: bulk

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