Subject: [PATCH v4 1/3] warn when setting GUC to a nonextant library

From: Justin Pryzby <[email-masked]>
Date: Mon, 13 Dec 2021 08:42:38 -0600

---
 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

--1Ow488MNN9B9o/ov
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v4-0002-errcontext-if-server-fails-to-start-due-to-librar.patch"