From 3f0252b89bb62c2f40b414d4051e869f25f635e1 Mon Sep 17 00:00:00 2001 From: Ning Yu Date: Tue, 23 Jul 2019 11:29:04 +0800 Subject: [PATCH v1 2/4] Test concurrent call to pg_mkdir_p() --- src/test/modules/Makefile | 1 + src/test/modules/test_pg_mkdir_p/.gitignore | 2 + src/test/modules/test_pg_mkdir_p/Makefile | 23 ++++ src/test/modules/test_pg_mkdir_p/README | 44 ++++++++ .../expected/test_pg_mkdir_p.out | 67 +++++++++++ .../test_pg_mkdir_p/sql/test_pg_mkdir_p.sql | 18 +++ .../test_pg_mkdir_p/test_pg_mkdir_p--1.0.sql | 9 ++ .../modules/test_pg_mkdir_p/test_pg_mkdir_p.c | 106 ++++++++++++++++++ .../test_pg_mkdir_p/test_pg_mkdir_p.control | 5 + 9 files changed, 275 insertions(+) create mode 100644 src/test/modules/test_pg_mkdir_p/.gitignore create mode 100644 src/test/modules/test_pg_mkdir_p/Makefile create mode 100644 src/test/modules/test_pg_mkdir_p/README create mode 100644 src/test/modules/test_pg_mkdir_p/expected/test_pg_mkdir_p.out create mode 100644 src/test/modules/test_pg_mkdir_p/sql/test_pg_mkdir_p.sql create mode 100644 src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p--1.0.sql create mode 100644 src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p.c create mode 100644 src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p.control diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 60d6d7be1b..ff7847747d 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -15,6 +15,7 @@ SUBDIRS = \ test_integerset \ test_parser \ test_pg_dump \ + test_pg_mkdir_p \ test_predtest \ test_rbtree \ test_rls_hooks \ diff --git a/src/test/modules/test_pg_mkdir_p/.gitignore b/src/test/modules/test_pg_mkdir_p/.gitignore new file mode 100644 index 0000000000..19b6c5ba42 --- /dev/null +++ b/src/test/modules/test_pg_mkdir_p/.gitignore @@ -0,0 +1,2 @@ +# Generated subdirectories +/results/ diff --git a/src/test/modules/test_pg_mkdir_p/Makefile b/src/test/modules/test_pg_mkdir_p/Makefile new file mode 100644 index 0000000000..0005653d6f --- /dev/null +++ b/src/test/modules/test_pg_mkdir_p/Makefile @@ -0,0 +1,23 @@ +# src/test/modules/test_pg_mkdir_p/Makefile + +MODULE_big = test_pg_mkdir_p +OBJS = test_pg_mkdir_p.o $(WIN32RES) +PGFILEDESC = "test_pg_mkdir_p - helper function to test concurrent call to pg_mkdir_p" + +EXTENSION = test_pg_mkdir_p +DATA = test_pg_mkdir_p--1.0.sql + +REGRESS = test_pg_mkdir_p + +SHLIB_LINK += -L$(top_builddir)/src/port -lpgport + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_pg_mkdir_p +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_pg_mkdir_p/README b/src/test/modules/test_pg_mkdir_p/README new file mode 100644 index 0000000000..0ef705f4f0 --- /dev/null +++ b/src/test/modules/test_pg_mkdir_p/README @@ -0,0 +1,44 @@ +test_pg_mkdir_p is a test module to validate a race condition issue in +pg_mkdir_p(). + +pg_mkdir_p() is used by initdb and pg_basebackup to create a directory as well +as its parent directories. The logic used to be like below: + + if (stat(path) < 0) + { + if (mkdir(path) < 0) + retval = -1; + } + +It first checks for the existence of the path, if path does not pre-exist then +it creates the directory. However if two processes try to create path +concurrently, then one possible execution order is as below: + + A: stat(path) returns -1, so decide to create it; + B: stat(path) returns -1, so decide to create it; + B: mkdir(path) returns 0, successfully created path; + A: mkdir(path) returns -1, fail to create path as it already exist; + +It could be triggered easily with initdb: + + testdir=/tmp/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 + + rm -rf $testdir + mkdir $testdir + + # init two databases with common parent directories + initdb -D $datadir/db1 >$testdir/db1.log 2>&1 & + initdb -D $datadir/db2 >$testdir/db2.log 2>&1 & + + # wait for them to finish and check for the error + wait + grep 'could not create directory' $testdir/*.log + +The fail rate is not 100% but should be large enough to happen in 5 tries. + +This race condition could be fixed by swapping the order of stat() and +mkdir(), this is also what the "mkdir -p" command does. + +In this test module we test concurrent calls to pg_mkdir_p() to ensure the +race condition does not happen. diff --git a/src/test/modules/test_pg_mkdir_p/expected/test_pg_mkdir_p.out b/src/test/modules/test_pg_mkdir_p/expected/test_pg_mkdir_p.out new file mode 100644 index 0000000000..701c316926 --- /dev/null +++ b/src/test/modules/test_pg_mkdir_p/expected/test_pg_mkdir_p.out @@ -0,0 +1,67 @@ +CREATE EXTENSION test_pg_mkdir_p; +select * from test_pg_mkdir_p(1); + test_pg_mkdir_p +----------------- + t +(1 row) + +select * from test_pg_mkdir_p(2); + test_pg_mkdir_p +----------------- + t +(1 row) + +select * from test_pg_mkdir_p(2); + test_pg_mkdir_p +----------------- + t +(1 row) + +select * from test_pg_mkdir_p(4); + test_pg_mkdir_p +----------------- + t +(1 row) + +select * from test_pg_mkdir_p(4); + test_pg_mkdir_p +----------------- + t +(1 row) + +select * from test_pg_mkdir_p(8); + test_pg_mkdir_p +----------------- + t +(1 row) + +select * from test_pg_mkdir_p(8); + test_pg_mkdir_p +----------------- + t +(1 row) + +select * from test_pg_mkdir_p(16); + test_pg_mkdir_p +----------------- + t +(1 row) + +select * from test_pg_mkdir_p(16); + test_pg_mkdir_p +----------------- + t +(1 row) + +select * from test_pg_mkdir_p(32); + test_pg_mkdir_p +----------------- + t +(1 row) + +select * from test_pg_mkdir_p(32); + test_pg_mkdir_p +----------------- + t +(1 row) + diff --git a/src/test/modules/test_pg_mkdir_p/sql/test_pg_mkdir_p.sql b/src/test/modules/test_pg_mkdir_p/sql/test_pg_mkdir_p.sql new file mode 100644 index 0000000000..a637d7d75f --- /dev/null +++ b/src/test/modules/test_pg_mkdir_p/sql/test_pg_mkdir_p.sql @@ -0,0 +1,18 @@ +CREATE EXTENSION test_pg_mkdir_p; + +select * from test_pg_mkdir_p(1); + +select * from test_pg_mkdir_p(2); +select * from test_pg_mkdir_p(2); + +select * from test_pg_mkdir_p(4); +select * from test_pg_mkdir_p(4); + +select * from test_pg_mkdir_p(8); +select * from test_pg_mkdir_p(8); + +select * from test_pg_mkdir_p(16); +select * from test_pg_mkdir_p(16); + +select * from test_pg_mkdir_p(32); +select * from test_pg_mkdir_p(32); diff --git a/src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p--1.0.sql b/src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p--1.0.sql new file mode 100644 index 0000000000..044c4e8f38 --- /dev/null +++ b/src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p--1.0.sql @@ -0,0 +1,9 @@ +/* src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_pg_mkdir_p" to load this file. \quit + +CREATE FUNCTION test_pg_mkdir_p(integer) +RETURNS bool +AS 'MODULE_PATHNAME' +LANGUAGE C STRICT; diff --git a/src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p.c b/src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p.c new file mode 100644 index 0000000000..59cc4efbaf --- /dev/null +++ b/src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p.c @@ -0,0 +1,106 @@ +/*------------------------------------------------------------------------- + * + * test_pg_mkdir_p.c + * Helper function to test concurrent call to pg_mkdir_p() + * + * Copyright (c) 2007-2019, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p.c + * + *------------------------------------------------------------------------- + */ +#include "postgres.h" + +#include +#include + +#include "common/file_perm.h" +#include "fmgr.h" +#include "port.h" +#include "utils/elog.h" + +PG_MODULE_MAGIC; + +#define TESTDIR "/tmp/testdir_pg_mkdir_p" +#define 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" + +/* + * A struct to pass arguments to the thread and return the results. + */ +typedef struct +{ + pthread_t tid; /* thread id */ + char path[MAXPGPATH]; /* the path to create */ + int retcode; /* return code of pg_mkdir_p() */ + int error; /* errno */ +} Job; + +PG_FUNCTION_INFO_V1(test_pg_mkdir_p); + +static void * +job_thread(void *arg) +{ + Job *job = (Job *) arg; + + errno = 0; + + job->retcode = pg_mkdir_p(job->path, pg_dir_create_mode); + job->error = errno; + + return NULL; +} + +/* + * This function accepts one int32 argument n, it will launch n concurrent + * threads to call pg_mkdir_p() to create the same dir and check for errors + * from them. + * + * Return true if all the calls to pg_mkdir_p() succeed, otherwise false is + * returned. + */ +Datum +test_pg_mkdir_p(PG_FUNCTION_ARGS) +{ + int n = PG_GETARG_INT32(0); + int failed = 0; + int i; + Job *jobs; + + if (n <= 0) + elog(ERROR, "invalid argument: %d", n); + + jobs = palloc(sizeof(Job) * n); + + rmtree(TESTDIR, true); + + /* Create concurrent threads to execute pg_mkdir_p() */ + for (i = 0; i < n; i++) + { + Job *job = &jobs[i]; + + strncpy(job->path, DATADIR, sizeof(job->path)); + pthread_create(&job->tid, NULL, job_thread, job); + } + + /* Check for the results */ + for (i = 0; i < n; i++) + { + Job *job = &jobs[i]; + + pthread_join(job->tid, NULL); + + if (job->retcode < 0) + { + elog(NOTICE, + "job %d: could not create directory \"%s\": %s", + i, job->path, strerror(job->error)); + + failed++; + } + } + + pfree(jobs); + + PG_RETURN_BOOL(failed == 0); +} diff --git a/src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p.control b/src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p.control new file mode 100644 index 0000000000..2b8b738b6e --- /dev/null +++ b/src/test/modules/test_pg_mkdir_p/test_pg_mkdir_p.control @@ -0,0 +1,5 @@ +# test_pg_mkdir_p extension +comment = 'Test concurrent call to pg_mkdir_p()' +default_version = '1.0' +module_pathname = '$libdir/test_pg_mkdir_p' +relocatable = true -- 2.20.1