From 1a2507066d9da2babcf2069a4bcd9d58193ec454 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Mon, 14 Sep 2026 13:03:40 +0000 Subject: [PATCH v10 3/3] Route filesystem mutations through FILEOPS Route the server's transactional DDL/storage callers that mutate the filesystem outside the buffer manager through the FILEOPS resource manager added in the previous commit, so an abort or crash recovery recursively reverts a partial filesystem change instead of leaving orphan trees behind. - copydir() gains a register_for_abort_cleanup argument: forward-path callers register the destination tree with FILEOPS (FileOpsRmtree at abort), while WAL-replay callers pass false because the redo handler is itself the recovery action. - CREATE DATABASE (STRATEGY=FILE_COPY) and ALTER DATABASE SET TABLESPACE copy their directory trees with abort cleanup enabled, closing the long-standing crash-asymmetry where a rolled-back copy left an orphan tree. - CREATE/DROP TABLESPACE route their directory create/remove through FileOps* so tablespace symlink and directory changes are transactional. Author: Greg Burd Changes since v9: - CREATE DATABASE (both strategies) and the copydir() destination create their directory with FileOpsMkdirTree(), so an abort or a crash part way through the copy removes the whole tree. v9 only rmdir()ed it, which failed on the copied files, and FILE_COPY's cleanup was kept in memory only (Shihao; recovery/066, with injection points createdb-wal-log-after-copy-relation and createdb-file-copy-after-copydir) - test_fileops/010 checks that abort redo replays every kind of inverse, including a CREATE DATABASE that fails part way --- src/backend/commands/dbcommands.c | 152 +++++++++---- src/backend/commands/tablespace.c | 175 +++++++++++---- src/backend/storage/file/copydir.c | 23 +- src/include/storage/copydir.h | 14 +- src/test/modules/test_fileops/meson.build | 1 + .../t/007_fileops_commit_crash.pl | 18 ++ .../test_fileops/t/010_fileops_abort_redo.pl | 202 ++++++++++++++++++ src/test/recovery/meson.build | 1 + .../recovery/t/066_fileops_createdb_crash.pl | 113 ++++++++++ 9 files changed, 602 insertions(+), 97 deletions(-) create mode 100644 src/test/modules/test_fileops/t/010_fileops_abort_redo.pl create mode 100644 src/test/recovery/t/066_fileops_createdb_crash.pl diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index 7e3fc59eafd..ca16ce8bd73 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -56,6 +56,7 @@ #include "replication/slot.h" #include "storage/copydir.h" #include "storage/fd.h" +#include "storage/fileops.h" #include "storage/ipc.h" #include "storage/lmgr.h" #include "storage/md.h" @@ -219,6 +220,8 @@ CreateDatabaseUsingWalLog(Oid src_dboid, Oid dst_dboid, /* Copy relation storage from source to the destination. */ CreateAndCopyRelationData(srcrlocator, dstrlocator, relinfo->permanent); + INJECTION_POINT("createdb-wal-log-after-copy-relation", NULL); + /* Release the relation locks. */ UnlockRelationId(&srcrelid, AccessShareLock); UnlockRelationId(&dstrelid, AccessShareLock); @@ -472,56 +475,95 @@ CreateDirAndVersionFile(char *dbpath, Oid dbid, Oid tsid, bool isRedo) sprintf(buf, "%s\n", PG_MAJORVERSION); nbytes = strlen(PG_MAJORVERSION) + 1; - /* Create database directory. */ - if (MakePGDirectory(dbpath) < 0) + /* + * Create database directory. The copy fills it through smgr, so an abort + * or crash before commit must remove the whole tree. + */ + if (!isRedo) { - /* Failure other than already exists or not in WAL replay? */ - if (errno != EEXIST || !isRedo) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not create directory \"%s\": %m", dbpath))); + FileOpsMkdirTree(dbpath, pg_dir_create_mode, dbid); + } + else + { + if (MakePGDirectory(dbpath) < 0) + { + /* Failure other than already exists or not in WAL replay? */ + if (errno != EEXIST) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not create directory \"%s\": %m", dbpath))); + } } /* - * Create PG_VERSION file in the database path. If the file already - * exists and we are in WAL replay then try again to open it in write - * mode. + * Create PG_VERSION file in the database path. */ snprintf(versionfile, sizeof(versionfile), "%s/%s", dbpath, "PG_VERSION"); - fd = OpenTransientFile(versionfile, O_WRONLY | O_CREAT | O_EXCL | PG_BINARY); - if (fd < 0 && errno == EEXIST && isRedo) - fd = OpenTransientFile(versionfile, O_WRONLY | O_TRUNC | PG_BINARY); + if (!isRedo) + { + /* + * Use FileOpsCreate + FileOpsWrite for transactional version file + * creation with crash-safe rollback support. + */ + fd = FileOpsCreate(versionfile, O_WRONLY | PG_BINARY, + pg_file_create_mode, true); + pgstat_report_wait_start(WAIT_EVENT_VERSION_FILE_WRITE); + errno = 0; + if ((int) write(fd, buf, nbytes) != nbytes) + { + if (errno == 0) + errno = ENOSPC; + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not write to file \"%s\": %m", versionfile))); + } + pgstat_report_wait_end(); - if (fd < 0) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not create file \"%s\": %m", versionfile))); + pgstat_report_wait_start(WAIT_EVENT_VERSION_FILE_SYNC); + if (pg_fsync(fd) != 0) + ereport(data_sync_elevel(ERROR), + (errcode_for_file_access(), + errmsg("could not fsync file \"%s\": %m", versionfile))); + fsync_fname(dbpath, true); + pgstat_report_wait_end(); - /* Write PG_MAJORVERSION in the PG_VERSION file. */ - pgstat_report_wait_start(WAIT_EVENT_VERSION_FILE_WRITE); - errno = 0; - if (write(fd, buf, nbytes) != nbytes) - { - /* If write didn't set errno, assume problem is no disk space. */ - if (errno == 0) - errno = ENOSPC; - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not write to file \"%s\": %m", versionfile))); + CloseTransientFile(fd); } - pgstat_report_wait_end(); + else + { + /* WAL replay: use raw file operations */ + fd = OpenTransientFile(versionfile, O_WRONLY | O_CREAT | O_EXCL | PG_BINARY); + if (fd < 0 && errno == EEXIST) + fd = OpenTransientFile(versionfile, O_WRONLY | O_TRUNC | PG_BINARY); - pgstat_report_wait_start(WAIT_EVENT_VERSION_FILE_SYNC); - if (pg_fsync(fd) != 0) - ereport(data_sync_elevel(ERROR), - (errcode_for_file_access(), - errmsg("could not fsync file \"%s\": %m", versionfile))); - fsync_fname(dbpath, true); - pgstat_report_wait_end(); + if (fd < 0) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not create file \"%s\": %m", versionfile))); - /* Close the version file. */ - CloseTransientFile(fd); + pgstat_report_wait_start(WAIT_EVENT_VERSION_FILE_WRITE); + errno = 0; + if ((int) write(fd, buf, nbytes) != nbytes) + { + if (errno == 0) + errno = ENOSPC; + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not write to file \"%s\": %m", versionfile))); + } + pgstat_report_wait_end(); + + pgstat_report_wait_start(WAIT_EVENT_VERSION_FILE_SYNC); + if (pg_fsync(fd) != 0) + ereport(data_sync_elevel(ERROR), + (errcode_for_file_access(), + errmsg("could not fsync file \"%s\": %m", versionfile))); + fsync_fname(dbpath, true); + pgstat_report_wait_end(); + + CloseTransientFile(fd); + } /* If we are not in WAL replay then write the WAL. */ if (!isRedo) @@ -633,9 +675,13 @@ CreateDatabaseUsingFileCopy(Oid src_dboid, Oid dst_dboid, Oid src_tsid, /* * Copy this subdirectory to the new location * - * We don't need to copy subdirectories + * We don't need to copy subdirectories. Register the destination for + * delete-on-abort so a transaction abort recursively removes the + * partial copy. */ - copydir(srcpath, dstpath, false); + copydir(srcpath, dstpath, false, true); + + INJECTION_POINT("createdb-file-copy-after-copydir", NULL); /* Record the filesystem change in XLOG */ { @@ -2221,8 +2267,17 @@ movedb(const char *dbname, const char *tblspcname) FreeDir(dstdir); /* - * The directory exists but is empty. We must remove it before using + * The directory exists but is empty. We must remove it before using * the copydir function. + * + * NB: this rmdir is deliberately NOT routed through FILEOPS. The + * destination tablespace path is not yet referenced by any catalog + * row (the pg_database.dattablespace update happens later, and is + * what binds dst_dbpath to the database on commit), so an abort that + * leaves dst_dbpath missing does not orphan any catalog state -- the + * database remains in src_tblspcoid. copydir() below recreates + * dst_dbpath with FileOpsMkdirTree(), so an abort or a crash before + * commit removes the partial copy. */ if (rmdir(dst_dbpath) != 0) elog(ERROR, "could not remove directory \"%s\": %m", @@ -2245,9 +2300,13 @@ movedb(const char *dbname, const char *tblspcname) bool new_record_repl[Natts_pg_database] = {0}; /* - * Copy files from the old tablespace to the new one + * Copy files from the old tablespace to the new one. Register the + * destination for delete-on-abort so a transaction abort recursively + * removes the partial copy (closing the long-standing crash-asymmetry + * with the source tree, which is preserved until commit by movedb's + * post-commit cleanup). */ - copydir(src_dbpath, dst_dbpath, false); + copydir(src_dbpath, dst_dbpath, false, true); /* * Record the filesystem change in XLOG @@ -3407,9 +3466,12 @@ dbase_redo(XLogReaderState *record) /* * Copy this subdirectory to the new location * - * We don't need to copy subdirectories + * We don't need to copy subdirectories. Pass false for + * register_for_abort_cleanup: this is a WAL replay path with no + * surrounding transaction; the redo handler is itself the recovery + * action. */ - copydir(src_path, dst_path, false); + copydir(src_path, dst_path, false, false); pfree(src_path); pfree(dst_path); diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c index 7c40176b7b9..0ba99f635af 100644 --- a/src/backend/commands/tablespace.c +++ b/src/backend/commands/tablespace.c @@ -70,6 +70,7 @@ #include "miscadmin.h" #include "postmaster/bgwriter.h" #include "storage/fd.h" +#include "storage/fileops.h" #include "storage/lmgr.h" #include "storage/lwlock.h" #include "storage/procsignal.h" @@ -600,11 +601,18 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid) if (in_place) { - if (MakePGDirectory(linkloc) < 0 && errno != EEXIST) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not create directory \"%s\": %m", - linkloc))); + if (!InRecovery) + { + FileOpsMkdir(linkloc, pg_dir_create_mode); + } + else + { + if (MakePGDirectory(linkloc) < 0 && errno != EEXIST) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not create directory \"%s\": %m", + linkloc))); + } } location_with_version_dir = psprintf("%s/%s", in_place ? linkloc : location, @@ -615,20 +623,36 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid) * it doesn't exist or has the wrong owner. Not needed for in-place mode, * because in that case we created the directory with the desired * permissions. + * + * Outside recovery, route through FileOpsChmod so a transaction abort + * restores the prior mode via the FILEOPS UNDO record. In recovery + * (where there is no surrounding transaction) issue chmod directly. On + * Windows, chmod is partial (only the read-only bit is meaningful); + * FileOpsChmod handles platform differences. */ - if (!in_place && chmod(location, pg_dir_create_mode) != 0) + if (!in_place) { - if (errno == ENOENT) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_FILE), - errmsg("directory \"%s\" does not exist", location), - InRecovery ? errhint("Create this directory for the tablespace before " - "restarting the server.") : 0)); + int rc; + + if (InRecovery) + rc = chmod(location, pg_dir_create_mode); else - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not set permissions on directory \"%s\": %m", - location))); + rc = FileOpsChmod(location, pg_dir_create_mode); + + if (rc != 0) + { + if (errno == ENOENT) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_FILE), + errmsg("directory \"%s\" does not exist", location), + InRecovery ? errhint("Create this directory for the tablespace before " + "restarting the server.") : 0)); + else + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not set permissions on directory \"%s\": %m", + location))); + } } /* @@ -645,6 +669,8 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid) (errcode_for_file_access(), errmsg("could not stat directory \"%s\": %m", location_with_version_dir))); + else if (!InRecovery) + FileOpsMkdir(location_with_version_dir, pg_dir_create_mode); else if (MakePGDirectory(location_with_version_dir) < 0) ereport(ERROR, (errcode_for_file_access(), @@ -671,11 +697,16 @@ create_tablespace_directories(const char *location, const Oid tablespaceoid) /* * Create the symlink under PGDATA */ - if (!in_place && symlink(location, linkloc) < 0) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not create symbolic link \"%s\": %m", - linkloc))); + if (!in_place) + { + if (!InRecovery) + FileOpsSymlink(location, linkloc); + else if (symlink(location, linkloc) < 0) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not create symbolic link \"%s\": %m", + linkloc))); + } pfree(linkloc); pfree(location_with_version_dir); @@ -773,27 +804,50 @@ destroy_tablespace_directories(Oid tablespaceoid, bool redo) return false; } - /* remove empty directory */ - if (rmdir(subfile) < 0) - ereport(redo ? LOG : ERROR, - (errcode_for_file_access(), - errmsg("could not remove directory \"%s\": %m", - subfile))); + /* + * Remove empty directory. Outside recovery, defer to commit time so + * a transaction abort leaves the per-DB tablespace subdirectory + * intact. In recovery, issue rmdir directly because the redo handler + * is itself the recovery action. + */ + if (redo) + { + if (rmdir(subfile) < 0) + ereport(LOG, + (errcode_for_file_access(), + errmsg("could not remove directory \"%s\": %m", + subfile))); + } + else + { + FileOpsRmdir(subfile, true /* at_commit */ ); + } pfree(subfile); } FreeDir(dirdesc); - /* remove version directory */ - if (rmdir(linkloc_with_version_dir) < 0) + /* + * Remove version directory. Same forward / redo split as above: defer to + * commit outside recovery so abort restores the version dir; issue rmdir + * directly during redo. + */ + if (redo) { - ereport(redo ? LOG : ERROR, - (errcode_for_file_access(), - errmsg("could not remove directory \"%s\": %m", - linkloc_with_version_dir))); - pfree(linkloc_with_version_dir); - return false; + if (rmdir(linkloc_with_version_dir) < 0) + { + ereport(LOG, + (errcode_for_file_access(), + errmsg("could not remove directory \"%s\": %m", + linkloc_with_version_dir))); + pfree(linkloc_with_version_dir); + return false; + } + } + else + { + FileOpsRmdir(linkloc_with_version_dir, true /* at_commit */ ); } /* @@ -819,26 +873,53 @@ remove_symlink: } else if (S_ISDIR(st.st_mode)) { - if (rmdir(linkloc) < 0) + /* + * Junction-style entry (Windows) or stale directory (any platform). + * In redo, remove now; outside redo, defer to commit so abort + * restores the entry. + */ + if (redo) { - int saved_errno = errno; + if (rmdir(linkloc) < 0) + { + int saved_errno = errno; - ereport(redo ? LOG : (saved_errno == ENOENT ? WARNING : ERROR), - (errcode_for_file_access(), - errmsg("could not remove directory \"%s\": %m", - linkloc))); + ereport(saved_errno == ENOENT ? LOG : LOG, + (errcode_for_file_access(), + errmsg("could not remove directory \"%s\": %m", + linkloc))); + } + } + else + { + FileOpsRmdir(linkloc, true /* at_commit */ ); } } else if (S_ISLNK(st.st_mode)) { - if (unlink(linkloc) < 0) + /* + * Tablespace symlink under PGDATA/pg_tblspc/. Symmetric with + * FileOpsSymlink on the create path: defer the unlink to commit so + * abort restores the symlink, closing the long-standing + * crash-asymmetry between CREATE TABLESPACE and DROP TABLESPACE. In + * redo, issue unlink directly because the redo handler is itself the + * recovery action. + */ + if (redo) { - int saved_errno = errno; + if (unlink(linkloc) < 0) + { + int saved_errno = errno; - ereport(redo ? LOG : (saved_errno == ENOENT ? WARNING : ERROR), - (errcode_for_file_access(), - errmsg("could not remove symbolic link \"%s\": %m", - linkloc))); + ereport(saved_errno == ENOENT ? LOG : LOG, + (errcode_for_file_access(), + errmsg("could not remove symbolic link \"%s\": %m", + linkloc))); + } + } + else + { + FileOpsDelete(linkloc, true /* at_commit */ ); } } else diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c index ee42c796f77..4eb46d3498a 100644 --- a/src/backend/storage/file/copydir.c +++ b/src/backend/storage/file/copydir.c @@ -24,11 +24,14 @@ #include #include +#include "access/xlog.h" +#include "common/file_perm.h" #include "common/file_utils.h" #include "miscadmin.h" #include "pgstat.h" #include "storage/copydir.h" #include "storage/fd.h" +#include "storage/fileops.h" #include "utils/wait_event.h" /* GUCs */ @@ -44,16 +47,24 @@ static void clone_file(const char *fromfile, const char *tofile); * * This function uses the file_copy_method GUC. New uses of this function must * be documented in doc/src/sgml/config.sgml. + * + * If register_for_abort_cleanup is true and we are not in WAL replay, the + * destination is created with FileOpsMkdirTree(), so that a transaction + * abort, or a crash before commit, removes the partial copy. Recovery + * callers pass false because they are themselves the recovery action. */ void -copydir(const char *fromdir, const char *todir, bool recurse) +copydir(const char *fromdir, const char *todir, bool recurse, + bool register_for_abort_cleanup) { DIR *xldir; struct dirent *xlde; char fromfile[MAXPGPATH * 2]; char tofile[MAXPGPATH * 2]; - if (MakePGDirectory(todir) != 0) + if (register_for_abort_cleanup && !RecoveryInProgress()) + FileOpsMkdirTree(todir, pg_dir_create_mode, InvalidOid); + else if (MakePGDirectory(todir) != 0) ereport(ERROR, (errcode_for_file_access(), errmsg("could not create directory \"%s\": %m", todir))); @@ -78,9 +89,13 @@ copydir(const char *fromdir, const char *todir, bool recurse) if (xlde_type == PGFILETYPE_DIR) { - /* recurse to handle subdirectories */ + /* + * Recurse to handle subdirectories. Pass false for + * register_for_abort_cleanup: removing the top-level destination + * tree removes them too. + */ if (recurse) - copydir(fromfile, tofile, true); + copydir(fromfile, tofile, true, false); } else if (xlde_type == PGFILETYPE_REG) { diff --git a/src/include/storage/copydir.h b/src/include/storage/copydir.h index 40849c2ef99..b979ee4258b 100644 --- a/src/include/storage/copydir.h +++ b/src/include/storage/copydir.h @@ -22,7 +22,19 @@ typedef enum FileCopyMethod /* GUC parameters */ extern PGDLLIMPORT int file_copy_method; -extern void copydir(const char *fromdir, const char *todir, bool recurse); +/* + * copydir(): copy a directory tree. + * + * If register_for_abort_cleanup is true, the destination is created with + * FileOpsMkdirTree(). Forward-path callers in transactional commands + * (CREATE DATABASE STRATEGY=FILE_COPY, ALTER DATABASE SET TABLESPACE) should + * pass true so that a transaction abort, or a crash before commit, removes + * the partial copy. Recovery callers (XLOG_DBASE_CREATE_FILE_COPY redo) must + * pass false because there is no surrounding transaction context and the + * redo handler is itself the recovery action. + */ +extern void copydir(const char *fromdir, const char *todir, bool recurse, + bool register_for_abort_cleanup); extern void copy_file(const char *fromfile, const char *tofile); #endif /* COPYDIR_H */ diff --git a/src/test/modules/test_fileops/meson.build b/src/test/modules/test_fileops/meson.build index 5d886f13739..e14676912a4 100644 --- a/src/test/modules/test_fileops/meson.build +++ b/src/test/modules/test_fileops/meson.build @@ -40,6 +40,7 @@ tests += { 't/005_fileops_checkpoint_loser.pl', 't/007_fileops_commit_crash.pl', 't/008_fileops_standby_abort.pl', + 't/010_fileops_abort_redo.pl', 't/018_fileops_rename.pl', 't/023_fileops_inline_abort_ctx.pl', 't/024_fileops_recovery_apply_failure.pl', diff --git a/src/test/modules/test_fileops/t/007_fileops_commit_crash.pl b/src/test/modules/test_fileops/t/007_fileops_commit_crash.pl index 0ff3e80890f..f40dcd271b0 100644 --- a/src/test/modules/test_fileops/t/007_fileops_commit_crash.pl +++ b/src/test/modules/test_fileops/t/007_fileops_commit_crash.pl @@ -73,5 +73,23 @@ TODO: 'committed FileOpsRmdir happened despite the crash'); } +# DROP TABLESPACE is safe: its XLOG_TBLSPC_DROP precedes the commit record +# and its redo removes the directories. (In-place, so they are under +# pg_tblspc.) +$node->safe_psql('postgres', + 'SET allow_in_place_tablespaces = on; CREATE TABLESPACE ts LOCATION \'\'' +); +my $tsoid = $node->safe_psql('postgres', + "SELECT oid FROM pg_tablespace WHERE spcname = 'ts'"); +ok(-d "$datadir/pg_tblspc/$tsoid", 'tablespace directory exists'); +commit_and_crash('DROP TABLESPACE ts;'); +is( $node->safe_psql( + 'postgres', "SELECT count(*) FROM pg_tablespace WHERE spcname = 'ts'" + ), + '0', + 'DROP TABLESPACE committed'); +ok(!-e "$datadir/pg_tblspc/$tsoid", + 'committed DROP TABLESPACE removed its directory despite the crash'); + $node->stop; done_testing(); diff --git a/src/test/modules/test_fileops/t/010_fileops_abort_redo.pl b/src/test/modules/test_fileops/t/010_fileops_abort_redo.pl new file mode 100644 index 00000000000..11a33c94c58 --- /dev/null +++ b/src/test/modules/test_fileops/t/010_fileops_abort_redo.pl @@ -0,0 +1,202 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('abort_redo'); +$node->init; +$node->append_conf('postgresql.conf', + "wal_level = replica\nautovacuum = off\ncheckpoint_timeout = '1h'\nmax_prepared_transactions = 5\n" +); +$node->start; +$node->safe_psql('postgres', 'CREATE EXTENSION test_fileops'); +my $dir = $node->data_dir; +$node->safe_psql( + 'postgres', q{ +BEGIN; +SELECT test_fileops_create('base.dat', 384); +SELECT test_fileops_write('base.dat', 0, 'committed-base'); +COMMIT; +CHECKPOINT; +BEGIN; +SELECT test_fileops_create('abort.dat', 384); +SELECT test_fileops_write('abort.dat', 0, 'aborted'); +ROLLBACK; +BEGIN; SELECT test_fileops_mkdir('abortdir', 448); ROLLBACK; +BEGIN; SELECT test_fileops_chmod('base.dat', 448); ROLLBACK; +BEGIN; SELECT test_fileops_write('base.dat', 0, 'OVERWRITTEN!!!'); ROLLBACK; +BEGIN; SELECT test_fileops_truncate('base.dat', 3); ROLLBACK; +BEGIN; +SAVEPOINT s; +SELECT test_fileops_create('sub.dat', 384); +ROLLBACK TO s; +SELECT test_fileops_write('base.dat', 0, 'parent'); +ROLLBACK; +BEGIN; SELECT test_fileops_symlink('base.dat', 'abort.sym'); ROLLBACK; +BEGIN; SELECT test_fileops_link('base.dat', 'abort.link'); ROLLBACK; + +}); +my $xattrs = $node->safe_psql('postgres', + "SELECT test_fileops_setxattr('base.dat', 'user.abort_test', 'original')") + eq 't'; +if ($xattrs) +{ + $node->safe_psql( + 'postgres', q{ +BEGIN; +SELECT test_fileops_setxattr('base.dat', 'user.abort_test', 'changed'); +ROLLBACK; +BEGIN; +SELECT test_fileops_removexattr('base.dat', 'user.abort_test'); +ROLLBACK; +}); +} +my ($uid, $gid) = (stat("$dir/base.dat"))[ 4, 5 ]; +# An unprivileged test can chown to its own identity; a different uid needs +# a privileged harness. This still exercises CHOWN inverse serialization. +$node->safe_psql('postgres', + "BEGIN; SELECT test_fileops_chown('base.dat', $uid, $gid); ROLLBACK;"); +for my $restart (0 .. 2) +{ + if ($restart) + { + $node->stop('immediate'); + $node->start; + } + ok(!-e "$dir/sub.dat", "$restart: subabort then parent abort"); + ok(!-l "$dir/abort.sym", "$restart: symlink inverse"); + ok(!-e "$dir/abort.link", "$restart: link inverse"); + ok(!-e "$dir/abort.dat", "$restart: aborted create absent"); + ok(!-e "$dir/abortdir", "$restart: aborted mkdir absent"); + SKIP: + { + skip 'POSIX permission semantics required', 1 if $windows_os; + is((stat("$dir/base.dat"))[2] & 07777, + 0600, "$restart: original mode"); + } + is((stat("$dir/base.dat"))[4], + $uid, "$restart: owner after chown inverse"); + is((stat("$dir/base.dat"))[5], + $gid, "$restart: group after chown inverse"); + SKIP: + { + skip 'extended attributes not supported', 1 unless $xattrs; + is( $node->safe_psql( + 'postgres', + "SELECT test_fileops_getxattr('base.dat', 'user.abort_test')" + ), + 'original', + "$restart: xattr inverses restore value"); + } + is(-s "$dir/base.dat", 14, "$restart: original size"); + is(slurp_file("$dir/base.dat"), + 'committed-base', "$restart: original bytes"); +} + +# Stop with a durable abort decision but no inverses applied yet. A +# concurrently requested checkpoint must wait, not omit the abort from redo. +if ($ENV{enable_injection_points} eq 'yes') +{ + $node->safe_psql('postgres', 'CREATE EXTENSION injection_points'); + for my $checkpoint (0, 1, 2) + { + my $finish = + $checkpoint == 2 + ? "PREPARE TRANSACTION 'window'; ROLLBACK PREPARED 'window';" + : 'ROLLBACK;'; + $node->safe_psql('postgres', 'CHECKPOINT'); + $node->safe_psql('postgres', + "SELECT injection_points_attach('fileops-abort-after-flush', 'wait')" + ); + my $bg = $node->background_psql('postgres', on_error_stop => 0); + $bg->query_until( + qr/start/, qq(\\echo start +BEGIN; +SELECT test_fileops_create('mid.dat', 384); +SELECT test_fileops_write('mid.dat', 0, 'uncommitted'); +SELECT test_fileops_truncate('base.dat', 3); +$finish +)); + $node->wait_for_event('client backend', 'fileops-abort-after-flush'); + my $ckpt; + if ($checkpoint == 1) + { + $ckpt = $node->background_psql('postgres', on_error_stop => 0); + $ckpt->query_until(qr/checkpoint/, + "\\echo checkpoint\nCHECKPOINT;\n"); + $node->wait_for_event('checkpointer', 'CheckpointDelayStart'); + } + $node->stop('immediate'); + $bg->{run}->finish; + $ckpt->{run}->finish if $ckpt; + $node->start; + ok(!-e "$dir/mid.dat", + "$checkpoint: interrupted abort removed create"); + is(slurp_file("$dir/base.dat"), + 'committed-base', + "$checkpoint: interrupted abort restored shrink bytes"); + $node->stop('immediate'); + $node->start; + ok(!-e "$dir/mid.dat", + "$checkpoint: repeated recovery keeps create absent"); + } +} + +if ($ENV{enable_injection_points} eq 'yes') +{ + $node->safe_psql('postgres', 'CHECKPOINT'); + $node->safe_psql( + 'postgres', q{ +SELECT injection_points_attach('createdb-wal-log-after-copy-relation', 'error'); +SELECT injection_points_attach('fileops-abort-after-flush', 'wait'); +}); + my $db = $node->background_psql('postgres', on_error_stop => 0); + $db->query_until(qr/start/, + "\\echo start\nCREATE DATABASE aborted_tree STRATEGY WAL_LOG OID 90000;\n" + ); + $node->wait_for_event('client backend', 'fileops-abort-after-flush'); + # createdb's error cleanup removes the online directory already. Redo + # recreates it from forward WAL, so the embedded tree inverse must remove + # it again even though the original inode no longer exists. + ok(!-e "$dir/base/90000", 'createdb error cleanup removed online tree'); + $node->stop('immediate'); + $db->{run}->finish; + $node->start; + ok(!-e "$dir/base/90000", 'abort redo removes database tree'); +} + +# A WARNING is not a durable retry obligation: fixing permissions does not +# retry this inverse online. A crash before the next checkpoint replays it. +$node->safe_psql('postgres', 'CHECKPOINT'); +my $failed = $node->background_psql('postgres'); +$failed->query_safe('BEGIN'); +$failed->query_safe("SELECT test_fileops_truncate('base.dat', 3)"); +chmod(0400, "$dir/base.dat") or die "chmod: $!"; +my $log_offset = -s $node->logfile; +$failed->query('ROLLBACK'); +$failed->quit; +like( + slurp_file($node->logfile, $log_offset), + qr/WARNING:.*FILEOPS UNDO TRUNCATE: could not open/, + 'failed inverse reports a warning'); +chmod(0600, "$dir/base.dat") or die "chmod: $!"; +is(slurp_file("$dir/base.dat"), + 'com', 'failed inverse remains unapplied online'); +$node->stop('immediate'); +$node->start; +is(slurp_file("$dir/base.dat"), + 'committed-base', 'abort redo retries failed inverse after crash'); + +# Refuse excessive before-images before touching the file. +my ($ret, $out, $err) = $node->psql('postgres', + "SELECT test_fileops_write('base.dat', 0, repeat('x', 1048577))"); +like( + $err, + qr/ERROR:.*excessive transactional file write range/, + 'oversized write is an ordinary error'); +is(slurp_file("$dir/base.dat"), + 'committed-base', 'refused write changes nothing'); +$node->stop; +done_testing(); diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index daa07c38560..430d199455f 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -74,6 +74,7 @@ tests += { 't/062_fileops_recovery.pl', 't/063_fileops_2pc.pl', 't/065_fileops_crash_undo.pl', + 't/066_fileops_createdb_crash.pl', ], }, } diff --git a/src/test/recovery/t/066_fileops_createdb_crash.pl b/src/test/recovery/t/066_fileops_createdb_crash.pl new file mode 100644 index 00000000000..108a932fc02 --- /dev/null +++ b/src/test/recovery/t/066_fileops_createdb_crash.pl @@ -0,0 +1,113 @@ +# Copyright (c) 2024-2026, PostgreSQL Global Development Group + +# A CREATE DATABASE that crashes, or fails, part way through its copy must +# leave no database directory behind, for either strategy. The directory +# is created by FILEOPS but filled by smgr (WAL_LOG) or copydir (FILE_COPY), +# so its UNDO must remove the whole tree, not just rmdir it. + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $node = PostgreSQL::Test::Cluster->new('createdb_crash'); +$node->init; +$node->append_conf( + 'postgresql.conf', q{ +shared_preload_libraries = 'injection_points' +wal_level = replica +autovacuum = off +}); +$node->start; +if (!$node->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points'); +$node->safe_psql('postgres', 'CREATE DATABASE tmpl'); +$node->safe_psql('tmpl', + "CREATE TABLE t AS SELECT g, repeat('x', 200) v FROM generate_series(1, 20000) g" +); + +my $base = $node->data_dir . '/base'; + +# Directories under base/ that belong to no database. +sub orphans +{ + my %known = map { $_ => 1 } split /\n/, + $node->safe_psql('postgres', 'SELECT oid FROM pg_database'); + opendir(my $dh, $base) or die "opendir $base: $!"; + my @o = grep { /^\d+$/ && !$known{$_} } readdir($dh); + closedir($dh); + return join(' ', sort @o); +} + +my %point = ( + wal_log => 'createdb-wal-log-after-copy-relation', + file_copy => 'createdb-file-copy-after-copydir'); + +foreach my $strategy ('wal_log', 'file_copy') +{ + my $ip = $point{$strategy}; + my %before = map { $_ => 1 } split / /, orphans(); + + # Crash while the new directory holds copied files. + $node->safe_psql('postgres', + "SELECT injection_points_attach('$ip', 'wait')"); + my $bg = $node->background_psql('postgres', on_error_stop => 0); + $bg->query_until( + qr/start/, qq(\\echo start +CREATE DATABASE c_$strategy TEMPLATE tmpl STRATEGY $strategy; +)); + $node->wait_for_event('client backend', $ip); + + my ($new) = grep { !$before{$_} } split / /, orphans(); + ok(defined $new, "$strategy: the new database directory exists"); + $new //= 'none'; + my @files = glob("$base/$new/*"); + cmp_ok(scalar(@files), '>', 1, + "$strategy: it holds copied files at the crash"); + + my $log_offset = -s $node->logfile; + $node->stop('immediate'); + $bg->{run}->finish; + $node->start; + + is(orphans(), '', "$strategy: no database directory left after a crash"); + my $log = slurp_file($node->logfile, $log_offset); + unlike( + $log, + qr/WARNING: FILEOPS UNDO/, + "$strategy: crash recovery applied the UNDO cleanly"); + + # The same, with the copy failing instead. + $node->safe_psql('postgres', + "SELECT injection_points_attach('$ip', 'error')"); + my ($ret, $out, $err) = $node->psql('postgres', + "CREATE DATABASE e_$strategy TEMPLATE tmpl STRATEGY $strategy"); + like($err, qr/ERROR/, "$strategy: CREATE DATABASE failed mid-copy"); + $node->safe_psql('postgres', "SELECT injection_points_detach('$ip')"); + is(orphans(), '', "$strategy: no database directory left after an error"); + + # A later CREATE DATABASE still works. + $node->safe_psql('postgres', + "CREATE DATABASE ok_$strategy TEMPLATE tmpl STRATEGY $strategy"); + is($node->safe_psql("ok_$strategy", 'SELECT count(*) FROM t'), + '20000', "$strategy: a new CREATE DATABASE succeeds"); +} + +# No failed rmdir of a non-empty directory anywhere in the log. +sleep(1); +unlike( + slurp_file($node->logfile), + qr/could not rmdir .*Directory not empty/, + 'no failed rmdir of a non-empty directory'); + +$node->stop; +done_testing(); -- 2.54.0