Re: pgsql: Fix unlink() for STATUS_DELETE_PENDING on Windows. - Mailing list pgsql-committers

On Wed, Oct 26, 2022 at 12:57 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Wed, Oct 26, 2022 at 11:15:16AM +1300, Thomas Munro wrote:
> > On Wed, Oct 26, 2022 at 10:31 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > > If I'm not wrong, this didn't fix the issue you said it fixed.
> > >
> > > s/said/hoped/sorry
> >
> > Drat.  More theories needed then.  Perhaps it has nothing to do with
> > my recent changes.  I am starting to wonder if we should have an
> > rmdir() wrapper that waits for zombie files to go away, and spits out
> > the name of the file that's in the way, and also the name/pid of the
> > process that has it open[1] if it times out, so we have a fighting
> > chance of debugging this type of stuff.
> >
> > [1] https://devblogs.microsoft.com/oldnewthing/20120217-00/?p=8283
>
> FWIW, your description of the problem sounded a bit off to me.
>
> You seemed to say that the problem is with rmdir() on a directory, which
> contains a file which was "recently removed", but still opened by
> something.

Right, that's what I was guessing because that is a known phenomenon,
and we can see that the error is "Directory not empty".  If, on the
other hand, that's caused by a file that's never been unlinked, we
might expect to see occasional failures on non-Windows systems too.

Wait a minute.  Even if that hunch were correct, what I changed would
not be enough to fix it.  Looking at the code that presumably performs
the recursive unlink and emits the warning, namely
src/common/rmtree.c, I see that it calls lstat() itself before calling
unlink().  So a zombie pending-deleted file would be skipped (because
ENOENT) before even reaching the code in question, and that was
already the case before anything I changed in the past few months in
this area AFAIK.

Which is a clue that we've been looking in the wrong place.  Perhaps
it has to do with file handles opened by pg_upgrade itself, which did
indeed recently change to a new path, though you'd think that'd be
more deterministic.  Now I'm tempted to write the patch that would
tell us the names of the files that are in the way to get more
visibility here.

> But as I recall, at the point that pg_upgrade is running rmdir(), the
> contained files have been unlinked, and the postgres process has ended:

Oh.  Hmm.  I wondered if it might be a logger process (they shut down
slightly after the postmaster IIRC), but it doesn't look like we start
one.

> My thinking has always been that this is essentially a bug/deficiency in
> the windows FS.  It seems absurd that unlink/rmdir would be
> asynchronous.

It certainly doesn't make life easy for open source projects from
planet Unix.  I wonder if those semantics came from the VMS orbit.

> It'd be swell if we could use a separate device in CI, to be used for
> running tests.  Bonus points if it supports COW.

I think it might be possible to create a ReFS filesystem inside a
loopback file.  On that filesystem, the tests I propose in CF #3951
take the !have_posix_unlink_semantics path, even if we commit the
patch to turn on POSIX semantics (which has useful effect only on
NTFS, according to experiments so far).  If we decided to commit that
without also setting up coverage for non-POSIX-semantics filesystems,
I predict that our support for non-POSIX unlink semantics would very
soon decay and become unsalvageable, which is why it amounts to a
policy decision about future support...