From 77c50877644b9bd7723e4f4032c608101fd872e6 Mon Sep 17 00:00:00 2001 From: Salil Chandra Date: Wed, 20 May 2026 14:23:18 -0400 Subject: [PATCH] {184305174} Fix delfiles to clear ufid-hash before deleting files This fixes a regression introduced in commit 5802570a9 where ufid clearing was moved from bdb_del_file() to schema changes only (bdb_close_only_sc). This caused the standalone 'delfiles' command to leave open file descriptors in ufid-hash, especially on replicants that opened handles during recovery. Root Cause: - When replicants apply log records during recovery, they open DB handles via ufid-hash and mark them with DB_AM_RECOVER flag - These handles are kept open for performance - When 'delfiles' command runs WITHOUT a schema change, bdb_del_file() directly calls dbremove() without clearing ufid-hash entries - The OS cannot release disk space because handles remain open - Files show as "deleted" in lsof output but space is not reclaimed Impact: - Production databases accumulate "deleted" files over time - Requires weekly database bounces to reclaim space The Fix: - Restore ufid clearing to bdb_del_file() before calling dbremove() - Respects gbl_clear_ufid_on_db_close tunable (default=1) - Opens the file briefly, calls clear_ufid_hash(), then closes it - This clears any recovery handles (DB_AM_RECOVER) from ufid-hash - Non-fatal if clearing fails - logs warning and continues Test: - New test delfiles_ufid_leak.test validates delfiles WITHOUT schema change - Unlike berkdb_file_leaks_rep.test which does TRUNCATE before delfiles, this test reproduces the production scenario - Verifies no "deleted" files remain open after delfiles Signed-off-by: Salil Chandra --- bdb/file.c | 27 ++++++- tests/delfiles_ufid_leak.test/Makefile | 5 ++ tests/delfiles_ufid_leak.test/lrl.options | 4 + tests/delfiles_ufid_leak.test/runit | 91 +++++++++++++++++++++++ 4 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 tests/delfiles_ufid_leak.test/Makefile create mode 100644 tests/delfiles_ufid_leak.test/lrl.options create mode 100755 tests/delfiles_ufid_leak.test/runit diff --git a/bdb/file.c b/bdb/file.c index 6b7da5f35c..bb8572cda3 100644 --- a/bdb/file.c +++ b/bdb/file.c @@ -6554,6 +6554,7 @@ static int bdb_del_file(bdb_state_type *bdb_state, DB_TXN *tid, char *filename, int *bdberr) { DB_ENV *dbenv; + DB *dbp = NULL; char transname[PATH_MAX]; char *pname = bdb_trans(filename, transname); int rc = 0; @@ -6564,7 +6565,31 @@ static int bdb_del_file(bdb_state_type *bdb_state, DB_TXN *tid, char *filename, dbenv = bdb_state->dbenv; if ((rc = access(pname, F_OK)) == 0) { - int rc = dbenv->dbremove(dbenv, tid, filename, NULL, 0); + extern int gbl_clear_ufid_on_db_close; + + /* Clear any ufid-hash entries before deleting the file to prevent + * "deleted" file descriptors from being held open, especially on + * replicants that opened handles during recovery. */ + if (gbl_clear_ufid_on_db_close) { + if ((rc = db_create(&dbp, dbenv, 0)) == 0 && + (rc = dbp->open(dbp, NULL, pname, NULL, DB_BTREE, 0, 0666)) == 0) { + /* Clear ufid hash entry for this file */ + int clear_rc = dbp->clear_ufid_hash(dbp, NULL, 0); + if (clear_rc != 0) { + logmsg(LOGMSG_WARN, "bdb_del_file: clear_ufid_hash for %s failed: %d %s\n", filename, clear_rc, + db_strerror(clear_rc)); + /* Continue anyway - not fatal */ + } + dbp->close(dbp, DB_NOSYNC); + dbp = NULL; + } else if (rc != ENOENT) { + /* File exists but we couldn't open it - log but continue */ + logmsg(LOGMSG_WARN, "bdb_del_file: failed to open %s for ufid clearing: %d %s\n", filename, rc, + db_strerror(rc)); + } + } + + rc = dbenv->dbremove(dbenv, tid, filename, NULL, 0); if (rc) { logmsg(LOGMSG_ERROR, "bdb_del_file: dbremove %s failed: %d %s\n", filename, rc, db_strerror(rc)); diff --git a/tests/delfiles_ufid_leak.test/Makefile b/tests/delfiles_ufid_leak.test/Makefile new file mode 100644 index 0000000000..b6cafefe6f --- /dev/null +++ b/tests/delfiles_ufid_leak.test/Makefile @@ -0,0 +1,5 @@ +ifeq ($(TESTSROOTDIR),) + include ../testcase.mk +else + include $(TESTSROOTDIR)/testcase.mk +endif diff --git a/tests/delfiles_ufid_leak.test/lrl.options b/tests/delfiles_ufid_leak.test/lrl.options new file mode 100644 index 0000000000..1f49260fc4 --- /dev/null +++ b/tests/delfiles_ufid_leak.test/lrl.options @@ -0,0 +1,4 @@ +setattr CHECKPOINTTIME 9999999 +setattr MEMPTRICKLEPERCENT 10 +clear_ufid_on_db_close 1 +dtastripe 1 diff --git a/tests/delfiles_ufid_leak.test/runit b/tests/delfiles_ufid_leak.test/runit new file mode 100755 index 0000000000..ed7a562540 --- /dev/null +++ b/tests/delfiles_ufid_leak.test/runit @@ -0,0 +1,91 @@ +#!/usr/bin/env bash +bash -n "$0" | exit 1 + +# This test validates that the delfiles command clears ufid-hash entries +# BEFORE deleting files, preventing "deleted" file descriptors from being +# held open on replicants. +# +# Unlike berkdb_file_leaks_rep.test which does TRUNCATE before delfiles, +# this test runs delfiles WITHOUT any preceding schema change to reproduce +# the issue where deleted files accumulate on replicants. + +source ${TESTSROOTDIR}/tools/cluster_utils.sh + +[ -z "${CLUSTER}" ] && { echo "Test requires a cluster"; exit 0; } + +dbnm=$1 + +master=`cdb2sql --tabs ${CDB2_OPTIONS} $dbnm default "select host from comdb2_cluster where is_master='Y'"` +replicant=`cdb2sql --tabs ${CDB2_OPTIONS} $dbnm default "select comdb2_host()"` + +echo "Creating table and inserting data..." +cdb2sql ${CDB2_OPTIONS} $dbnm default "CREATE TABLE t1 (i integer, b blob)" +sleep 2 + +cdb2sql $dbnm --host $master "EXEC PROCEDURE sys.cmd.send('bdb checkpoint')" +sleep 2 + +# Insert data to create files +cdb2sql ${CDB2_OPTIONS} $dbnm default "INSERT INTO t1 VALUES(1, x'DEADBEEF')" +cdb2sql ${CDB2_OPTIONS} $dbnm default "INSERT INTO t1 VALUES(2, x'CAFEBABE')" +sleep 2 + +cdb2sql $dbnm --host $master "EXEC PROCEDURE sys.cmd.send('bdb checkpoint')" +sleep 2 + +# TRUNCATE to create an old file version +echo "Truncating table to create old file version..." +cdb2sql ${CDB2_OPTIONS} $dbnm default "TRUNCATE TABLE t1" +sleep 2 + +cdb2sql $dbnm --host $master "EXEC PROCEDURE sys.cmd.send('bdb checkpoint')" +sleep 2 + +# Insert new data in the new file version +cdb2sql ${CDB2_OPTIONS} $dbnm default "INSERT INTO t1 VALUES(3, x'BAADF00D')" +sleep 2 + +cdb2sql $dbnm --host $master "EXEC PROCEDURE sys.cmd.send('bdb checkpoint')" +sleep 2 + +echo "Restarting replicant $replicant to force recovery..." +# This causes the replicant to open DB handles via ufid-hash during recovery +# These handles will be for BOTH the old and new file versions +kill_restart_node $replicant 10 1 +cdb2sql $dbnm --host $replicant "SELECT 1" +if [ $? -ne 0 ]; then + echo 'db not up?' >&2 + exit 1 +fi + +sleep 2 + +echo "Running delfiles WITHOUT any schema change..." +# KEY: No TRUNCATE/ALTER/DROP before delfiles! +# This is the scenario where delfiles is run standalone +# Without the fix, ufid-hash holds open handles to old files +cdb2sql -tabs $dbnm --host $master "EXEC PROCEDURE sys.cmd.send('delfiles t1')" + +sleep 2 + +# Check all nodes for "deleted" files +echo "Checking for deleted files held open..." +for host in $CLUSTER; do + dbpid=`ssh $host 'pgrep -a comdb2' | grep $dbnm | awk '{print $1}' | head -1` + echo "Checking host $host (pid $dbpid)..." + + deleted_count=`ssh $host "lsof -p $dbpid 2>/dev/null | grep -c deleted"` + rc=$? + [ $rc -ne 0 ] && deleted_count=0 + + if [ "$deleted_count" != "0" ]; then + echo "ERROR: Host $host has $deleted_count deleted files held open!" >&2 + echo "Deleted files:" >&2 + ssh $host "lsof -p $dbpid 2>/dev/null | grep deleted" >&2 + exit 1 + fi + echo " No deleted files on $host" +done + +echo "Success! delfiles properly cleared ufid-hash entries." +exit 0