On Fri, Jan 28, 2022 at 09:42:17AM -0500, Robert Haas wrote:
> -1 from me on using "guc" in any user-facing error message. And even
> guc -> setting isn't a big improvement. If we're going to structure
> the reporting this way there, we should try to use a meaningful phrase
> there, probably beginning with the word "while"; see "git grep
> errcontext.*while" for interesting precedents.

On Wed, Feb 02, 2022 at 06:06:01AM +0000, Maciek Sakrejda wrote:
> I do agree that GUC is awkward here, and I like the "while..." wording 
> suggested both for consistency with other messages and how it could work here:
> CONTEXT: while loading "shared_preload_libraries"

FYI, it has said "while..." and hasn't said "guc" since the 2nd revision of the
patch.

> That said, that series of messages seems a bit suspect to me, because
> the WARNING seems to be stating the same problem as the subsequent
> FATAL and CONTEXT lines. Ideally we'd tighten that somehow.

I avoided the warning by checking IsUnderPostmaster, though I'm not sure if
that's the right condition..

On Wed, Feb 02, 2022 at 06:06:01AM +0000, Maciek Sakrejda wrote:
> I think this is great, but it would be really helpful to also indicate that 
> at this point the server will fail to come back up after a restart.

> I don't really know a good wording here, but maybe a hint like "The server or 
> session will not be able to start if it has been configured to use libraries 
> that cannot be loaded."?

postgres=# ALTER SYSTEM SET shared_preload_libraries =a,b;
WARNING:  could not access file "$libdir/plugins/a"
HINT:  The server will fail to start with the existing configuration.  If it is 
is shut down, it will be necessary to manually edit the postgresql.auto.conf 
file to allow it to start.
WARNING:  could not access file "$libdir/plugins/b"
HINT:  The server will fail to start with the existing configuration.  If it is 
is shut down, it will be necessary to manually edit the postgresql.auto.conf 
file to allow it to start.
ALTER SYSTEM
postgres=# ALTER SYSTEM SET session_preload_libraries =c,d;
WARNING:  could not access file "$libdir/plugins/c"
HINT:  New sessions will fail with the existing configuration.
WARNING:  could not access file "$libdir/plugins/d"
HINT:  New sessions will fail with the existing configuration.
ALTER SYSTEM

$ ./tmp_install/usr/local/pgsql/bin/postgres -D src/test/regress/tmp_check/data 
-clogging_collector=on
2022-02-09 19:53:48.034 CST postmaster[30979] FATAL:  could not access file 
"a": No such file or directory
2022-02-09 19:53:48.034 CST postmaster[30979] CONTEXT:  while loading shared 
libraries for setting "shared_preload_libraries"
        from 
/home/pryzbyj/src/postgres/src/test/regress/tmp_check/data/postgresql.auto.conf:3
2022-02-09 19:53:48.034 CST postmaster[30979] LOG:  database system is shut down

Maybe it's enough to show the GucSource rather than file:line...

-- 
Justin
>From 69560e81bfbb43a67269f54b564fe8ac5ae1b5c6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH v4 1/3] warn when setting GUC to a nonextant library

---
 src/backend/utils/misc/guc.c                  | 103 +++++++++++++++++-
 .../unsafe_tests/expected/rolenames.out       |   1 +
 .../worker_spi/expected/worker_spi.out        |   2 +
 .../modules/worker_spi/sql/worker_spi.sql     |   2 +
 src/test/regress/expected/rules.out           |   9 ++
 src/test/regress/sql/rules.sql                |   1 +
 6 files changed, 115 insertions(+), 3 deletions(-)

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f505413a7f9..c44b8ebbfd6 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -170,6 +170,12 @@ static bool call_enum_check_hook(struct config_enum *conf, int *newval,
 								 void **extra, GucSource source, int elevel);
 
 static bool check_log_destination(char **newval, void **extra, GucSource source);
+static bool check_local_preload_libraries(char **newval, void **extra,
+		GucSource source);
+static bool check_session_preload_libraries(char **newval, void **extra,
+		GucSource source);
+static bool check_shared_preload_libraries(char **newval, void **extra,
+		GucSource source);
 static void assign_log_destination(const char *newval, void *extra);
 
 static bool check_wal_consistency_checking(char **newval, void **extra,
@@ -4198,7 +4204,7 @@ static struct config_string ConfigureNamesString[] =
 		},
 		&session_preload_libraries_string,
 		"",
-		NULL, NULL, NULL
+		check_session_preload_libraries, NULL, NULL
 	},
 
 	{
@@ -4209,7 +4215,7 @@ static struct config_string ConfigureNamesString[] =
 		},
 		&shared_preload_libraries_string,
 		"",
-		NULL, NULL, NULL
+		check_shared_preload_libraries, NULL, NULL
 	},
 
 	{
@@ -4220,7 +4226,7 @@ static struct config_string ConfigureNamesString[] =
 		},
 		&local_preload_libraries_string,
 		"",
-		NULL, NULL, NULL
+		check_local_preload_libraries, NULL, NULL
 	},
 
 	{
@@ -12149,6 +12155,97 @@ check_max_worker_processes(int *newval, void **extra, GucSource source)
 	return true;
 }
 
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+	bool restricted, bool is_session)
+{
+	char		*rawstring;
+	List		*elemlist;
+	ListCell	*l;
+
+	/* nothing to do if empty */
+	if (newval == NULL || *newval[0] == '\0')
+		return true;
+
+	/*
+	 * Do not issue warnings while applying GUCs during startup.  That would
+	 * issue a FATAL error message, and the warnings would be redundant.
+	 */
+	if (!IsUnderPostmaster)
+		return true;
+
+	/* Need a modifiable copy of string */
+	rawstring = pstrdup(*newval);
+
+	/* Parse string into list of filename paths */
+	if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+	{
+		/* Should not happen ? */
+		return false;
+	}
+
+	foreach(l, elemlist)
+	{
+		/* Note that filename was already canonicalized */
+		char       *filename = (char *) lfirst(l);
+		char       *expanded = NULL;
+
+		/* If restricting, insert $libdir/plugins if not mentioned already */
+		if (restricted && first_dir_separator(filename) == NULL)
+		{
+			expanded = psprintf("$libdir/plugins/%s", filename);
+			filename = expanded;
+		}
+
+		/*
+		 * stat()/access() only check that the library exists, not that it has
+		 * the correct magic number or even a library.  But error messages from
+		 * dlopen() are not portable, so it'd be hard to report any problem
+		 * other than "does not exist".
+		 */
+		if (access(filename, R_OK) == 0) // F_OK
+			continue;
+
+		if (is_session)
+			ereport(WARNING,
+					errcode_for_file_access(),
+					errmsg("could not access file \"%s\"", filename),
+					errhint("New sessions will fail with the existing configuration."));
+		else
+			ereport(WARNING,
+					errcode_for_file_access(),
+					errmsg("could not access file \"%s\"", filename),
+					errhint("The server will fail to start with the existing configuration."
+						"  If it is is shut down, it will be necessary to manually edit the %s file to allow it to start.",
+						"postgresql.auto.conf"));
+	}
+
+	list_free_deep(elemlist);
+	pfree(rawstring);
+	return true;
+}
+
+static bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+	return check_preload_libraries(newval, extra, source, true, false);
+}
+
+static bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+	return check_preload_libraries(newval, extra, source, false, false);
+}
+
+static bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+	return check_preload_libraries(newval, extra, source, true, true);
+}
+
 static bool
 check_effective_io_concurrency(int *newval, void **extra, GucSource source)
 {
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index eb608fdc2ea..368b10558d7 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1066,6 +1066,7 @@ GRANT pg_read_all_settings TO regress_role_haspriv;
 BEGIN;
 -- A GUC using GUC_SUPERUSER_ONLY is useful for negative tests.
 SET LOCAL session_preload_libraries TO 'path-to-preload-libraries';
+WARNING:  could not access file "$libdir/plugins/path-to-preload-libraries"
 SET SESSION AUTHORIZATION regress_role_haspriv;
 -- passes with role member of pg_read_all_settings
 SHOW session_preload_libraries;
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
index dc0a79bf759..5b275669bcf 100644
--- a/src/test/modules/worker_spi/expected/worker_spi.out
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -28,6 +28,8 @@ SELECT pg_reload_conf();
  t
 (1 row)
 
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
 -- wait until the worker has processed the tuple we just inserted
 DO $$
 DECLARE
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
index 4683523b29d..4ed5370d456 100644
--- a/src/test/modules/worker_spi/sql/worker_spi.sql
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -18,6 +18,8 @@ END
 $$;
 INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
 SELECT pg_reload_conf();
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
 -- wait until the worker has processed the tuple we just inserted
 DO $$
 DECLARE
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 1420288d67b..f3d33dda95e 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3362,6 +3362,7 @@ drop table hats;
 drop table hat_data;
 -- test for pg_get_functiondef properly regurgitating SET parameters
 -- Note that the function is kept around to stress pg_dump.
+SET check_function_bodies=no;
 CREATE FUNCTION func_with_set_params() RETURNS integer
     AS 'select 1;'
     LANGUAGE SQL
@@ -3371,6 +3372,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
     SET datestyle to iso, mdy
     SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
     IMMUTABLE STRICT;
+WARNING:  could not access file "Mixed/Case"
+HINT:  The server will fail to start with the existing configuration.  If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
+WARNING:  could not access file "c:/'a"/path"
+HINT:  The server will fail to start with the existing configuration.  If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
+WARNING:  could not access file ""
+HINT:  The server will fail to start with the existing configuration.  If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
+WARNING:  could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+HINT:  The server will fail to start with the existing configuration.  If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
 SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
                                                                             pg_get_functiondef                                                                            
 --------------------------------------------------------------------------------------------------------------------------------------------------------------------------
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index 8bdab6dec30..6308e5d27fd 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1198,6 +1198,7 @@ drop table hat_data;
 
 -- test for pg_get_functiondef properly regurgitating SET parameters
 -- Note that the function is kept around to stress pg_dump.
+SET check_function_bodies=no;
 CREATE FUNCTION func_with_set_params() RETURNS integer
     AS 'select 1;'
     LANGUAGE SQL
-- 
2.17.1

>From 8280f86eb5ebc2f4785f3df215ad71e690647c7f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sat, 18 Dec 2021 22:51:01 -0600
Subject: [PATCH v4 2/3] errcontext if server fails to start due to library
 GUCs

---
 src/backend/utils/fmgr/dfmgr.c    | 20 +++++++++++++++-----
 src/backend/utils/init/miscinit.c |  2 +-
 src/include/fmgr.h                |  1 +
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 050da780804..fcda19b7973 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -76,7 +76,7 @@ static DynamicFileList *file_tail = NULL;
 
 char	   *Dynamic_library_path;
 
-static void *internal_load_library(const char *libname);
+static void *internal_load_library(const char *libname, const char *gucname);
 static void incompatible_module_error(const char *libname,
 									  const Pg_magic_struct *module_magic_data) pg_attribute_noreturn();
 static void internal_unload_library(const char *libname);
@@ -115,7 +115,7 @@ load_external_function(const char *filename, const char *funcname,
 	fullname = expand_dynamic_library_name(filename);
 
 	/* Load the shared library, unless we already did */
-	lib_handle = internal_load_library(fullname);
+	lib_handle = internal_load_library(fullname, NULL);
 
 	/* Return handle if caller wants it */
 	if (filehandle)
@@ -144,6 +144,14 @@ load_external_function(const char *filename, const char *funcname,
  */
 void
 load_file(const char *filename, bool restricted)
+{
+	load_file_guc(filename, restricted, NULL);
+}
+
+/*
+ * Also accepts a GUC arg, for error reports
+ */
+void load_file_guc(const char *filename, bool restricted, const char *gucname)
 {
 	char	   *fullname;
 
@@ -158,7 +166,7 @@ load_file(const char *filename, bool restricted)
 	internal_unload_library(fullname);
 
 	/* Load the shared library */
-	(void) internal_load_library(fullname);
+	(void) internal_load_library(fullname, gucname);
 
 	pfree(fullname);
 }
@@ -179,9 +187,10 @@ lookup_external_function(void *filehandle, const char *funcname)
  * loaded.  Return the pg_dl* handle for the file.
  *
  * Note: libname is expected to be an exact name for the library file.
+ * gucname may be passed for error reports.
  */
 static void *
-internal_load_library(const char *libname)
+internal_load_library(const char *libname, const char *gucname)
 {
 	DynamicFileList *file_scanner;
 	PGModuleMagicFunction magic_func;
@@ -206,6 +215,7 @@ internal_load_library(const char *libname)
 		if (stat(libname, &stat_buf) == -1)
 			ereport(ERROR,
 					(errcode_for_file_access(),
+					 gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
 					 errmsg("could not access file \"%s\": %m",
 							libname)));
 
@@ -764,7 +774,7 @@ RestoreLibraryState(char *start_address)
 {
 	while (*start_address != '\0')
 	{
-		internal_load_library(start_address);
+		internal_load_library(start_address, NULL);
 		start_address += strlen(start_address) + 1;
 	}
 }
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 0868e5a24f6..17d13e6c6bf 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1659,7 +1659,7 @@ load_libraries(const char *libraries, const char *gucname, bool restricted)
 			expanded = psprintf("$libdir/plugins/%s", filename);
 			filename = expanded;
 		}
-		load_file(filename, restricted);
+		load_file_guc(filename, restricted, gucname);
 		ereport(DEBUG1,
 				(errmsg_internal("loaded library \"%s\"", filename)));
 		if (expanded)
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index 6560e462d66..9dccd0fc047 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -727,6 +727,7 @@ extern void *load_external_function(const char *filename, const char *funcname,
 									bool signalNotFound, void **filehandle);
 extern void *lookup_external_function(void *filehandle, const char *funcname);
 extern void load_file(const char *filename, bool restricted);
+extern void load_file_guc(const char *filename, bool restricted, const char *gucname);
 extern void **find_rendezvous_variable(const char *varName);
 extern Size EstimateLibraryStateSpace(void);
 extern void SerializeLibraryState(Size maxsize, char *start_address);
-- 
2.17.1

>From 2da099b709b84bb446c3555d111f2f9a6084dace Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Wed, 2 Feb 2022 20:54:49 -0600
Subject: [PATCH v4 3/3] show the GUC source in errcontext

---
 src/backend/utils/fmgr/dfmgr.c | 13 ++++++++++---
 src/backend/utils/misc/guc.c   | 21 +++++++++++++++++++++
 src/include/utils/guc.h        |  1 +
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index fcda19b7973..b679de0455e 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -34,6 +34,7 @@
 #include "lib/stringinfo.h"
 #include "miscadmin.h"
 #include "storage/shmem.h"
+#include "utils/guc_tables.h"
 #include "utils/hsearch.h"
 
 
@@ -213,11 +214,17 @@ internal_load_library(const char *libname, const char *gucname)
 		 * Check for same files - different paths (ie, symlink or link)
 		 */
 		if (stat(libname, &stat_buf) == -1)
+		{
+			char *errstr = strerror(errno);
+			int linenum;
+			char *sourcefile = gucname ? GetConfigSourceFile(gucname, &linenum) : NULL;
 			ereport(ERROR,
 					(errcode_for_file_access(),
 					 gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
-					 errmsg("could not access file \"%s\": %m",
-							libname)));
+					 sourcefile ? errcontext("from %s:%d", sourcefile, linenum) : 0,
+					 errmsg("could not access file \"%s\": %s",
+							libname, errstr)));
+		}
 
 		for (file_scanner = file_list;
 			 file_scanner != NULL &&
@@ -281,7 +288,7 @@ internal_load_library(const char *libname, const char *gucname)
 		}
 		else
 		{
-			/* try to close library */
+			/* try to close library */ // Not needed due to ERROR ? //
 			dlclose(file_scanner->handle);
 			free((char *) file_scanner);
 			/* complain */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index c44b8ebbfd6..ee9e375039f 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8283,6 +8283,27 @@ GetConfigOptionFlags(const char *name, bool missing_ok)
 	return record->flags;
 }
 
+/*
+ * Get the source file and line associated with the given option.
+ *
+ * If the option doesn't exist, return 0 if missing_ok is true,
+ * otherwise throw an ereport and don't return.
+ */
+char *
+GetConfigSourceFile(const char *name, int *linenum)
+{
+	struct config_generic *record;
+
+	record = find_option(name, false, false, ERROR);
+
+	/* Should not happen */
+	if (record == NULL)
+		return 0;
+
+	*linenum = record->sourceline;
+	return record->sourcefile;
+}
+
 
 /*
  * flatten_set_variable_args
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 6bb81707b09..7255246f339 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -360,6 +360,7 @@ extern const char *GetConfigOption(const char *name, bool missing_ok,
 								   bool restrict_privileged);
 extern const char *GetConfigOptionResetString(const char *name);
 extern int	GetConfigOptionFlags(const char *name, bool missing_ok);
+extern char	*GetConfigSourceFile(const char *name, int *linenum);
 extern void ProcessConfigFile(GucContext context);
 extern void InitializeGUCOptions(void);
 extern bool SelectConfigFiles(const char *userDoption, const char *progname);
-- 
2.17.1

Reply via email to