Re: stat() vs ERROR_DELETE_PENDING, round N + 1

On Thu, Sep 23, 2021 at 9:13 PM Juan José Santamaría Flecha
<juanjo(dot)santamaria(at)gmail(dot)com> wrote:
> On Thu, Sep 23, 2021 at 4:58 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>> One small detail I'd like to draw attention to is this bit, which
>> differs slightly from the [non-working] previous attempts by mapping
>> to two different errors:
>>
>> + * If there's no O_CREAT flag, then we'll pretend the file is
>> + * invisible. With O_CREAT, we have no choice but to report that
>> + * there's a file in the way (which wouldn't happen on Unix).
>>
>> ...
>>
>> + if (fileFlags & O_CREAT)
>> + err = ERROR_FILE_EXISTS;
>> + else
>> + err = ERROR_FILE_NOT_FOUND;
>
>
> When GetTempFileName() finds a duplicated file name and the file is pending for deletion, it fails with an "ERROR_ACCESS_DENIED" error code. That may describe the situation better than "ERROR_FILE_EXISTS".

Thanks for looking. Why do you think that's better? I assume that's
just the usual NT->Win32 error conversion at work.

The only case I can think of so far in our tree where you'd notice
this change of errno for the O_CREAT case is relfilenode creation[1],
and there it's just a case of printing a different message. Trying to
create a relfilenode that exists already in delete-pending state will
fail, but with this change we'll log the %m string for EEXIST instead
of EACCES (what you see today) or ENOENT (which seems nonsensical, "I
can't create your file because it doesn't exist", and what you'd get
with this patch if I didn't have the special case for O_CREAT). So I
think that's pretty arguably an improvement.

As for how likely you are to reach that case... hmm, I don't know what
access() returns for a file in delete-pending state. The docs say
"The function returns -1 if the named file does not exist or does not
have the given mode", so perhaps it returns 0 for such a case, in
which case we'll consider it a collision and keep searching for
another free relfilenode. If that's the case, it's probably really
really unlikely you'll reach the case described in the previous
paragraph, so it probably doesn't matter much.

Do we have any other code paths where this finer point could cause
problems? Looking around at code that handles EEXIST, most of it is
directory creation (unaffected by this patch), and then
src/port/mkdtemp.c for which this change is appropriate (it implements
POSIX mkdtemp(), which shouldn't report EACCES to its caller if
something it tries bumps into a delete-pending file, it should see
EEXIST and try a new name, which I think it will do with this patch,
through its call to open(O_CREAT | O_EXCL)).

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJz_pZTF9mckn6XgSv69%2BjGwdgLkxZ6b3NWGLBCVjqUZA%40mail.gmail.com