Hi Tom,
> Yeah, I noticed that too, and it does offend my inner neatnik.
>
> Instead of what you did, I'd be inclined to add
>
> free(configdir);
>
> return true;
> +
> +fail:
> + free(configdir);
> +
> + return false;
> }
>
> and then s/return false/goto fail/ throughout, so as to avoid
> duplicating the free() calls. It's a minor point as things stand,
> but if more cleanup gets added to the function I think it'd be
> easier to maintain this way.
Makes sense. Here is the corrected patch v2.
> Huh ... don't quite see where in that recipe we'd reach a
> SelectConfigFiles error exit.
How exactly we reach this code patch is a good question. I tried to
understand the exact conditions by using my steps to reproduce and an
ancient debugging technique with sleep(), elog() and `watch` - see
trick.txt. Unfortunately I was not able to reproduce it again nor
under Valgrind nor without it. I guess it means that either I did
something differently before or the right conditions are met under
rare circumstances.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 46fdefebe35..c25dfa6ab47 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1769,7 +1769,8 @@ RemoveGUCFromLists(struct config_generic *gconf)
slist_delete(&guc_report_list, &gconf->report_link);
}
-
+#include <stdio.h>
+#include <unistd.h>
/*
* Select the configuration files and data directory to be used, and
* do the initial read of postgresql.conf.
@@ -1789,7 +1790,7 @@ SelectConfigFiles(const char *userDoption, const char
*progname)
bool fname_is_malloced;
struct stat stat_buf;
struct config_string *data_directory_rec;
-
+ int ret;
/* configdir is -D option, or $PGDATA if no -D */
if (userDoption)
configdir = make_absolute_path(userDoption);
@@ -1978,6 +1979,13 @@ SelectConfigFiles(const char *userDoption, const char
*progname)
return true;
fail:
+
+ elog(WARNING, "Attach with the debugger, pid = %d\n", getpid());
+ do {
+ ret = sleep(1);
+ } while(ret == 0);
+ elog(WARNING, "sleep() returned %d\n", ret);
+
free(configdir);
return false;
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index 7e1aa422332..d14be4fda2a 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -248,7 +248,7 @@
* You should normally use MEMORY_CONTEXT_CHECKING with USE_VALGRIND;
* instrumentation of repalloc() is inferior without it.
*/
-/* #define USE_VALGRIND */
+#define USE_VALGRIND
/*
* Define this to cause pfree()'d memory to be cleared immediately, to
From 553d45826e1ec71ffd2a1d4bb4104d948cf8844d Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksan...@timescale.com>
Date: Wed, 13 Aug 2025 22:26:13 +0300
Subject: [PATCH v2] Fix memory leaks in SelectConfigFiles()
Make sure the memory allocated by make_absolute_path() is freed when
SelectConfigFiles() fails. It's mainly to silence Valgrind. The callers
exit() rapidly in such cases and no memory actually leaks.
Author: Aleksander Alekseev <aleksan...@tigerdata.com>
Reviewed-by: Tom Lane <t...@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAJ7c6TMByXE8dc7zDvDWTQjk6o-XXAdRg_RAg5CBaUOgFPV3LQ%40mail.gmail.com
---
src/backend/utils/misc/guc.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e404c345e6e..46fdefebe35 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1803,7 +1803,7 @@ SelectConfigFiles(const char *userDoption, const char *progname)
configdir);
if (errno == ENOENT)
write_stderr("Run initdb or pg_basebackup to initialize a PostgreSQL data directory.\n");
- return false;
+ goto fail;
}
/*
@@ -1830,7 +1830,7 @@ SelectConfigFiles(const char *userDoption, const char *progname)
"You must specify the --config-file or -D invocation "
"option or set the PGDATA environment variable.\n",
progname);
- return false;
+ goto fail;
}
/*
@@ -1851,8 +1851,7 @@ SelectConfigFiles(const char *userDoption, const char *progname)
{
write_stderr("%s: could not access the server configuration file \"%s\": %m\n",
progname, ConfigFileName);
- free(configdir);
- return false;
+ goto fail;
}
/*
@@ -1882,7 +1881,7 @@ SelectConfigFiles(const char *userDoption, const char *progname)
"or by the -D invocation option, or by the "
"PGDATA environment variable.\n",
progname, ConfigFileName);
- return false;
+ goto fail;
}
/*
@@ -1934,7 +1933,7 @@ SelectConfigFiles(const char *userDoption, const char *progname)
"or by the -D invocation option, or by the "
"PGDATA environment variable.\n",
progname, ConfigFileName);
- return false;
+ goto fail;
}
SetConfigOption("hba_file", fname, PGC_POSTMASTER, PGC_S_OVERRIDE);
@@ -1965,7 +1964,7 @@ SelectConfigFiles(const char *userDoption, const char *progname)
"or by the -D invocation option, or by the "
"PGDATA environment variable.\n",
progname, ConfigFileName);
- return false;
+ goto fail;
}
SetConfigOption("ident_file", fname, PGC_POSTMASTER, PGC_S_OVERRIDE);
@@ -1977,6 +1976,11 @@ SelectConfigFiles(const char *userDoption, const char *progname)
free(configdir);
return true;
+
+fail:
+ free(configdir);
+
+ return false;
}
/*
--
2.43.0
