Subject: [PATCH v6 2/4] warn when setting GUC to a nonextant library

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

TODO: test that warnings aren't issued during startup (only fatals)
---
 src/backend/commands/variable.c               | 96 +++++++++++++++++++
 src/backend/utils/misc/guc.c                  |  2 +-
 src/backend/utils/misc/guc_tables.c           |  6 +-
 src/include/utils/guc_hooks.h                 |  7 ++
 .../unsafe_tests/expected/rolenames.out       | 19 ++++
 .../modules/unsafe_tests/sql/rolenames.sql    | 13 +++
 .../worker_spi/expected/worker_spi.out        |  2 +
 .../modules/worker_spi/sql/worker_spi.sql     |  2 +
 src/test/regress/expected/rules.out           |  8 ++
 9 files changed, 151 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 791bac6715c..85642ab8453 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -40,6 +40,7 @@
 #include "utils/timestamp.h"
 #include "utils/tzparser.h"
 #include "utils/varlena.h"
+#include <unistd.h>
 
 /*
  * DATESTYLE
@@ -1210,3 +1211,98 @@ check_ssl(bool *newval, void **extra, GucSource source)
 #endif
 	return true;
 }
+
+
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+	bool restricted)
+{
+	char		*rawstring;
+	List		*elemlist;
+	ListCell	*l;
+
+	/* nothing to do if empty */
+	if (newval == NULL || *newval[0] == '\0')
+		return true;
+
+	/*
+	 * issue warnings only during an interactive SET, from ALTER
+	 * ROLE/DATABASE/SYSTEM.
+	 */
+	if (source != PGC_S_TEST)
+		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)
+			continue;
+
+
+		if (source == PGC_S_FILE)
+			/* ALTER SYSTEM */
+			ereport(WARNING,
+					errcode_for_file_access(),
+					errmsg("could not access file \"%s\"", filename),
+					errdetail("The server will currently fail to start with this setting."),
+					errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.",
+						"postgresql.auto.conf"));
+		else
+			/* ALTER ROLE/DATABASE */
+			ereport(WARNING,
+					errcode_for_file_access(),
+					errmsg("could not access file \"%s\"", filename),
+					errdetail("New sessions will currently fail to connect with the new setting."));
+	}
+
+	list_free_deep(elemlist);
+	pfree(rawstring);
+	return true;
+}
+
+bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+	return check_preload_libraries(newval, extra, source, true);
+}
+
+bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+	return check_preload_libraries(newval, extra, source, false);
+}
+
+bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+	return check_preload_libraries(newval, extra, source, true);
+}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ae8810591d6..9c284906d6e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -24,7 +24,7 @@
  */
 #include "postgres.h"
 
-#include <limits.h>
+// #include <limits.h>
 #include <sys/stat.h>
 #include <unistd.h>
 
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 05ab087934c..b7c8d8dcdfb 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4015,7 +4015,7 @@ struct config_string ConfigureNamesString[] =
 		},
 		&session_preload_libraries_string,
 		"",
-		NULL, NULL, NULL
+		check_session_preload_libraries, NULL, NULL
 	},
 
 	{
@@ -4026,7 +4026,7 @@ struct config_string ConfigureNamesString[] =
 		},
 		&shared_preload_libraries_string,
 		"",
-		NULL, NULL, NULL
+		check_shared_preload_libraries, NULL, NULL
 	},
 
 	{
@@ -4037,7 +4037,7 @@ struct config_string ConfigureNamesString[] =
 		},
 		&local_preload_libraries_string,
 		"",
-		NULL, NULL, NULL
+		check_local_preload_libraries, NULL, NULL
 	},
 
 	{
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index f1a9a183b49..3f51436a6f5 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -155,4 +155,11 @@ extern bool check_wal_consistency_checking(char **newval, void **extra,
 extern void assign_wal_consistency_checking(const char *newval, void *extra);
 extern void assign_xlog_sync_method(int new_sync_method, void *extra);
 
+extern bool check_local_preload_libraries(char **newval, void **extra,
+		GucSource source);
+extern bool check_session_preload_libraries(char **newval, void **extra,
+		GucSource source);
+extern bool check_shared_preload_libraries(char **newval, void **extra,
+		GucSource source);
+
 #endif							/* GUC_HOOKS_H */
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 88b1ff843be..ac161ce7c33 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1082,6 +1082,25 @@ RESET SESSION AUTHORIZATION;
 ERROR:  current transaction is aborted, commands ignored until end of transaction block
 ROLLBACK;
 REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+WARNING:  could not access file "$libdir/plugins/DoesNotExist"
+DETAIL:  New sessions will currently fail to connect with the new setting.
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+WARNING:  could not access file "DoesNotExist"
+DETAIL:  New sessions will currently fail to connect with the new setting.
+CREATE DATABASE regress_nosuch_db;
+ALTER DATABASE regress_nosuch_db SET session_preload_libraries="DoesNotExist";
+WARNING:  could not access file "$libdir/plugins/DoesNotExist"
+DETAIL:  New sessions will currently fail to connect with the new setting.
+ALTER DATABASE regress_nosuch_db SET local_preload_libraries="DoesNotExist";
+WARNING:  could not access file "DoesNotExist"
+DETAIL:  New sessions will currently fail to connect with the new setting.
+DROP DATABASE regress_nosuch_db;
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
 -- clean up
 \c
 DROP SCHEMA test_roles_schema;
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index adac36536db..cf605b08d30 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -494,6 +494,19 @@ RESET SESSION AUTHORIZATION;
 ROLLBACK;
 REVOKE pg_read_all_settings FROM regress_role_haspriv;
 
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+CREATE DATABASE regress_nosuch_db;
+ALTER DATABASE regress_nosuch_db SET session_preload_libraries="DoesNotExist";
+ALTER DATABASE regress_nosuch_db SET local_preload_libraries="DoesNotExist";
+DROP DATABASE regress_nosuch_db;
+
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
+
 -- clean up
 \c
 
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 624d0e5aae1..3a36898b8d3 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3398,6 +3398,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"
+DETAIL:  New sessions will currently fail to connect with the new setting.
+WARNING:  could not access file "c:/'a"/path"
+DETAIL:  New sessions will currently fail to connect with the new setting.
+WARNING:  could not access file ""
+DETAIL:  New sessions will currently fail to connect with the new setting.
+WARNING:  could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+DETAIL:  New sessions will currently fail to connect with the new setting.
 SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
                                                                             pg_get_functiondef                                                                            
 --------------------------------------------------------------------------------------------------------------------------------------------------------------------------
-- 
2.25.1

--vDrIcvfimaPJgjx+
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v6-0003-errcontext-if-server-fails-to-start-due-to-librar.patch"