Re: Possible race condition in pg_mkdir_p()?

On Tue, Jul 30, 2019 at 4:04 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Tue, Jul 23, 2019 at 02:54:20PM +0800, Ning Yu wrote:
> > MakePGDirectory() is also called in TablespaceCreateDbspace(), EEXIST is
> > considered as non-error for parent directories, however as it considers
> > EEXIST as a failure for the last level of the path so the logic is
> > still correct,
>
> So the complains here are about two things:
> - In some code paths calling mkdir, we don't care about the fact that
> EEXIST can happen for something else than a folder. This could be a
> problem if we have conflicts in the backend related to the naming of
> the files/folders created. I find a bit surprising to not perform
> the sanity checks in MakePGDirectory() in your patch. What of all the
> existing callers of this routine?

Thanks for the reply. There are several callers of MakePGDirectory(), most of
them already treats EEXIST as an error; TablespaceCreateDbspace() already has
a proper checking for the target dir, it has the chance to fail on a
concurrently created dir, but at least it will not be confused by a file with
the same name; PathNameCreateTemporaryDir() is fixed by our patch 0004, we
will call stat() after mkdir() to double check the result.

In fact personally I'm thinking that whether could we replace all uses of
MakePGDirectory() with pg_mkdir_p(), so we could simplify
TablespaceCreateDbspace() and PathNameCreateTemporaryDir() and other callers
significantly.

> - pg_mkdir_p is pretty bad at detecting problems with concurrent
> creation of parent directories, leading to random failures where these
> should not happen.
>
> I may be missing something, but your patch does not actually fix
> problem 2, no? Trying to do an initdb with a set of N folders using
> the same parent folders not created still results in random failures.

Well, we should have fixed problem 2, this is our major purpose of the patch
0001, it performs sanity check with stat() after mkdir() at each part of the
path.

The initdb test was just the one used by us to verify our fix, here is our
script:

n=4
testdir=testdir
datadir=$testdir/a/b/c/d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/w/x/y/z
logdir=$testdir/logs

rm -rf $testdir
mkdir -p $logdir

for i in `seq $n`; do
initdb -D $datadir/$i >$logdir/$i.log 2>&1 &
done

wait

# check for failures
grep 'could not create directory' $logdir/*

We have provided a test module in patch 0002 to perform a similar test, it
calls pg_mkdir_p() concurrently to trigger the issue, which has higher fail
rate than initdb. With the patch 0001 both the initdb test and the test
module will always succeed in our local environment.

Thanks
Ning

> --
> Michael