Subject: warn if GUC set to an invalid shared library

From: Justin Pryzby <[email-masked]>
Date: Tue, 28 Dec 2021 11:45:57 -0600

forking <[email-masked]>

On Mon, Dec 13, 2021 at 09:01:57AM -0500, Robert Haas wrote:
> On Thu, Dec 9, 2021 at 2:32 AM Maciek Sakrejda <[email-masked]> wrote:
> > > Considering the vanishingly small number of actual complaints we've
> > > seen about this, that sounds ridiculously over-engineered.
> > > A documentation example should be sufficient.
> >
> > I don't know if this will tip the scales, but I'd like to lodge a
> > belated complaint. I've gotten myself in this server-fails-to-start
> > situation several times (in development, for what it's worth). The
> > syntax (as Bharath pointed out in the original message) is pretty
> > picky, there are no guard rails, and if you got there through ALTER
> > SYSTEM, you can't fix it with ALTER SYSTEM (because the server isn't
> > up). If you go to fix it manually, you get a scary "Do not edit this
> > file manually!" warning that you have to know to ignore in this case
> > (that's if you find the file after you realize what the fairly generic
> > "FATAL: ... No such file or directory" error in the log is telling
> > you). Plus you have to get the (different!) quoting syntax right or
> > cut your losses and delete the change.
> 
> +1. I disagree that trying to detect this kind of problem would be
> "ridiculously over-engineered." I don't know whether it can be done
> elegantly enough that we'd be happy with it and I don't know whether
> it would end up just garden variety over-engineered. But there's
> nothing ridiculous about trying to prevent people from putting their
> system into a state where it won't start.
> 
> (To be clear, I also think updating the documentation is sensible,
> without taking a view on exactly what that update should look like.)

Yea, I think documentation won't help to avoid this issue:

If ALTER SYSTEM gives an ERROR, someone will likely to check the docs after a
few minutes if they know that they didn't get the correct syntax.
But if it gives no error nor warning, then most likely they won't know to check
the docs.

We should check session_preload_libraries too, right ?  It's PGC_SIGHUP, so if
someone sets the variable and sends sighup, clients will be rejected, and they
had no good opportunity to avoid that.

0001 adds WARNINGs when doing SET:

	postgres=# SET local_preload_libraries=xyz;
	WARNING:  could not load library: xyz: cannot open shared object file: No such file or directory
	SET

	postgres=# ALTER SYSTEM SET shared_preload_libraries =asdf;
	WARNING:  could not load library: $libdir/plugins/asdf: cannot open shared object file: No such file or directory
	ALTER SYSTEM

0002 adds context when failing to start.

	2021-12-27 17:01:12.996 CST postmaster[1403] WARNING:  could not load library: $libdir/plugins/asdf: cannot open shared object file: No such file or directory
	2021-12-27 17:01:14.938 CST postmaster[1403] FATAL:  could not access file "asdf": No such file or directory
	2021-12-27 17:01:14.938 CST postmaster[1403] CONTEXT:  guc "shared_preload_libraries"
	2021-12-27 17:01:14.939 CST postmaster[1403] LOG:  database system is shut down

But I wonder whether it'd be adequate context if dlopen were to fail rather
than stat() ?

Before 0003:
	2021-12-18 23:13:57.861 CST postmaster[11956] FATAL:  could not access file "asdf": No such file or directory
	2021-12-18 23:13:57.862 CST postmaster[11956] LOG:  database system is shut down

After 0003:
	2021-12-18 23:16:05.719 CST postmaster[13481] FATAL:  could not load library: asdf: cannot open shared object file: No such file or directory
	2021-12-18 23:16:05.720 CST postmaster[13481] LOG:  database system is shut down

-- 
Justin

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 30 Dec 2021 13:50:49 +0530

>         2021-12-18 23:16:05.720 CST postmaster[13481] LOG:  database system is shut down

Overall the idea looks good to me. A warning on ALTER SYSTEM SET seems
reasonable than nothing. ERROR isn't the way to go as it limits the
users of setting the extensions in shared_preload_libraries first,
installing them later. Is NOTICE here a better idea than WARNING?

I haven't looked at the patches though.

Regards,
Bharath Rupireddy.

---

From: Robert Haas <[email-masked]>
Date: Fri, 28 Jan 2022 09:42:17 -0500

>         2021-12-27 17:01:14.939 CST postmaster[1403] LOG:  database system is shut down

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

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.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Cary Huang <[email-masked]>
Date: Fri, 28 Jan 2022 23:36:20 +0000

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

Hello

I tested the patches on master branch on Ubuntu 18.04 and regression turns out fine. I did a manual test following the query examples in this email thread and I do see the warnings when I attempted: these queries:

postgres=# SET local_preload_libraries=xyz.so;
2022-01-28 15:11:00.592 PST [13622] WARNING:  could not access file "xyz.so"
WARNING:  could not access file "xyz.so"
SET
postgres=# ALTER SYSTEM SET shared_preload_libraries=abc.so;
2022-01-28 15:11:07.729 PST [13622] WARNING:  could not access file "$libdir/plugins/abc.so"
WARNING:  could not access file "$libdir/plugins/abc.so"
ALTER SYSTEM

This is fine as this is what these patches are aiming to provide. However, when I try to restart the server, it fails to start because abc.so and xyz.so do not exist. Setting the parameters "local_preload_libraries" and "local_preload_libraries" to something else in postgresql.conf does not seem to take effect either.
It still complains shared_preload_libraries abc.so does not exist even though I have already set shared_preload_libraries to something else in postgresql.conf. This seems a little strange to me 

thank you
Cary

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 28 Jan 2022 18:09:12 -0600

Thanks for loooking

> It still complains shared_preload_libraries abc.so does not exist even though I have already set shared_preload_libraries to something else in postgresql.conf. This seems a little strange to me 

Could you show exactly what you did and the output ?

The patches don't entirely prevent someone from putting the server config into
a bad state.  It only aims to tell them if they've done that, so they can fix
it, rather than letting someone (else) find the error at some later (probably
inconvenient) time.

ALTER SYSTEM adds config into postgresql.auto.conf.  If you stop the server
after adding bad config there (after getting a warning), the server won't
start.  Once the server is off, you have to remove it manually.

The goal of the patch is to 1) warn someone that they've put a bad config in
place, so they don't leave it there; and, 2) if the server fails to start for
such a reason, provide a CONTEXT line to help them resolve it quickly.

Maybe you know all that and I didn't understand what you're saying.

-- 
Justin

---

From: Maciek Sakrejda <[email-masked]>
Date: Wed, 02 Feb 2022 06:06:01 +0000

Spec compliant:           not tested

I tried the latest version of the patch, and it works as discussed. There is no documentation, but I think that's moot for this warning (we may want to note something in the setting docs, but even if so, I think we should figure out the message first and then decide if it merits additional explanation in the docs). I do not know whether it is spec-compliant, but I doubt the spec has much to say on something like this.

I tried running ALTER SYSTEM and got the warnings as expected:

postgres=# alter system set shared_preload_libraries = no_such_library,not_this_one_either;
WARNING:  could not access file "$libdir/plugins/no_such_library"
WARNING:  could not access file "$libdir/plugins/not_this_one_either"
ALTER SYSTEM

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. In my mind, that's a big part of the reason for having a warning here. Having made this mistake a couple of times, I would be able to read between the lines, as would many other users, but if you're not familiar with Postgres this might still be pretty opaque. I think if I'm reading the code correctly, this warning path is shared between ALTER SYSTEM and a SET of local_preload_libraries so it might be tricky to word this in a way that works in all situations, but it could make the precarious situation a lot clearer. 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."?

Also, there are two sides to this: one is actually applying the possibly-bogus setting, and the other is when that setting takes effect (e.g., attempting to start the server or to start a new session). I think Robert had good feedback regarding the latter:

> FATAL and CONTEXT lines. Ideally we'd tighten that somehow.

Maybe we don't even need the WARNING in this case? At this point, it's clear what the problem is. I think the CONTEXT line does actually help, because otherwise it's not clear why the server failed to start, but the warning does seem superfluous here. 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"

I think that would be pretty clear. In the ALTER SYSTEM case, you still need to know to edit the file in spite of the warning telling you not to edit it, but I think that's still better. Based on Cary's feedback, maybe that could be clearer, too (like you, I'm not sure if I understood what he did correctly), but I think that could certainly be future work.

Thanks,
Maciek

---

From: "David G. Johnston" <[email-masked]>
Date: Wed, 2 Feb 2022 08:39:03 -0700

On Tue, Feb 1, 2022 at 11:06 PM Maciek Sakrejda <[email-masked]>
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. In
> my mind, that's a big part of the reason for having a warning here. Having
> made this mistake a couple of times, I would be able to read between the
> lines, as would many other users, but if you're not familiar with Postgres
> this might still be pretty opaque.

+1

I would at least consider having the UX go something like:

postgres=# ALTER SYSTEM SET shared_preload_libraries = not_such_library;
ERROR: <paraphrase: your system will not reboot in its current state as
that library is not present>.
HINT: to bypass the error please add FORCE before SET
postgres=# ALTER SYSTEM FORCE SET shared_preload_libraries =
no_such_library;
NOTICE: Error suppressed while setting shared_preload_libraries.

That is, have the user express their desire to leave the system in a
precarious state explicitly before actually doing so.

Upon startup, if the system already can track each separate location that
shared_preload_libraries is set, printing out those locations and current
values would be useful context.  Seeing ALTER SYSTEM in that listing would
be helpful.

David J.

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 9 Feb 2022 19:58:54 -0600

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:

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

> 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 <[email-masked]>
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 <[email-masked]>
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 <[email-masked]>
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

---

From: Maciek Sakrejda <[email-masked]>
Date: Mon, 14 Feb 2022 10:12:22 -0800

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

The v3-0001 attached above had "while... for GUC..."--sorry I wasn't clear.

In v4, the message looks fine to me for shared_preload_libraries
(except there is a doubled "is"). However, I also get the message for
a simple SET with local_preload_libraries:

postgres=# set local_preload_libraries=xyz;
WARNING:  could not access file "xyz"
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.
SET

I'm not familiar with that setting (reading the docs, it's like a
non-superuser session_preload_libraries for compatible modules?), but
given nothing is being persisted here with ALTER SYSTEM, this seems
incorrect.

Changing session_preload_libraries emits a similar warning:

postgres=# set session_preload_libraries = foo;
WARNING:  could not access file "$libdir/plugins/foo"
HINT:  New sessions will fail with the existing configuration.
SET

This is also not persisted, so I think this is also incorrect, right?
(I'm not sure what setting session_preload_libraries without an ALTER
ROLE or ALTER DATABASE accomplishes, given a new session is required
for the change to take effect, but I thought I'd point this out.) I'm
guessing this may be due to trying to have the warning for ALTER ROLE?

postgres=# alter role bob set session_preload_libraries = foo;
WARNING:  could not access file "$libdir/plugins/foo"
HINT:  New sessions will fail with the existing configuration.
ALTER ROLE

This is great. Ideally, we'd qualify this with "New sessions for
user..." or "New sessions for database..." but given you get the
warning right after running the relevant command, maybe that's clear
enough.

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

This is great. I think the file:line output is helpful here.

Thanks,
Maciek

---

From: Tom Lane <[email-masked]>
Date: Wed, 23 Mar 2022 15:02:23 -0400

> postgresql.auto.conf file to allow it to start.

I agree with Maciek's concerns about these HINTs being emitted
inappropriately, but I also have a stylistic gripe: they're only
halfway hints.  Given that we fix things so they only print when they
should, the complaint about the server not starting is not a hint,
it's a fact, which per style guidelines means it should be errdetail.
So I think this ought to look more like

WARNING:  could not access file "xyz"
DETAIL:  The server will fail to start with this setting.
HINT:  If the server is shut down, it will be necessary to manually edit the
postgresql.auto.conf file to allow it to start again.

I adjusted the wording a bit too --- YMMV, but I think my text is clearer.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Thu, 16 Jun 2022 12:14:46 +0900

> I adjusted the wording a bit too --- YMMV, but I think my text is clearer.

It seems to me that there is no objection to the proposed patch, but
an update is required.  Justin?
--
Michael

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 15 Jun 2022 22:50:14 -0500

I've started to think that we should really WARN whenever a (set of) GUC is set
in a manner that the server will fail to start - not just for shared libraries.

In particular, I think the server should also warn if it's going to fail to
start like this:

2022-06-15 22:48:34.279 CDT postmaster[20782] FATAL:  WAL streaming (max_wal_senders > 0) requires wal_level "replica" or "logical"

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 21 Jul 2022 20:54:12 -0500

Finally returning to this .. rebased and updated per feedback.

I'm not sure of a good place to put test cases for this..

---

From: Maciek Sakrejda <[email-masked]>
Date: Fri, 22 Jul 2022 10:42:11 -0700

Thanks for picking this back up, Justin.

>in a manner that the server will fail to start - not just for shared libraries.

+0.5. I think it's a reasonable change, but I've never broken my
server with anything other than shared_preload_libraries, so I'd
rather see an improvement here first rather than expanding scope. I
think shared_preload_libraries (and friends) is especially tricky due
to the syntax, and more likely to lead to problems.

On the update patch itself, I have some minor feedback about message wording

Great, it's nice that this no longer gives a warning.

postgres=# alter role bob set local_preload_libraries = xyz;
WARNING:  could not access file "xyz"
DETAIL:  New sessions will currently fail to connect with the new setting.
ALTER ROLE

The warning makes sense, but the detail feels a little awkward. I
think "currently" is sort of redundant with "new setting". And it
could be clearer that the setting did in fact take effect (I know the
ALTER ROLE command tag echo tells you that, but we could reinforce
that in the warning).

Also, I know I said last time that the scope of the warning is clear
from the setting, but looking at it again, I think we could do better.
I guess because when we're generating the error, we don't know whether
the source was ALTER DATABASE or ALTER ROLE, we can't give a more
specific message? Ideally, I think the DETAIL would be something like
"New sessions for this role will fail to connect due to this setting".
Maybe even with a HINT of "Run ALTER ROLE again with a valid value to
fix this"? If that's not feasible, maybe  "New sessions for this role
or database will fail to connect due to this setting"? That message is
not as clear about the impact of the change as it could be, but
hopefully you know what command you just ran, so that should make it
unambiguous. I do think without qualifying that, it suggests that all
new sessions are affected.

Hmm, or maybe just "New sessions affected by this setting will fail to
connect"? That also makes the scope clear without the warning having
to be aware of the scope: if you just ran ALTER DATABASE it's pretty
clear that what is affected by the setting is the database. I think
this is probably the way to go, but leaving my thought process above
for context.

postgres=# alter system set shared_preload_libraries = lol;
WARNING:  could not access file "$libdir/plugins/lol"
DETAIL:  The server will currently fail to start with this setting.
HINT:  If the server is shut down, it will be necessary to manually
edit the postgresql.auto.conf file to allow it to start again.
ALTER SYSTEM

I think this works. Tom's copy edit above omitted "currently" from the
DETAIL and did not include the "$libdir/plugins/" prefix. I don't feel
strongly about these either way.

2022-07-22 10:37:50.217 PDT [1131187] LOG:  database system is shut down
2022-07-22 10:37:50.306 PDT [1134058] WARNING:  could not access file
"$libdir/plugins/lol"
2022-07-22 10:37:50.306 PDT [1134058] DETAIL:  The server will
currently fail to start with this setting.
2022-07-22 10:37:50.306 PDT [1134058] HINT:  If the server is shut
down, it will be necessary to manually edit the postgresql.auto.conf
file to allow it to start again.
2022-07-22 10:37:50.312 PDT [1134058] FATAL:  could not access file
"lol": No such file or directory
2022-07-22 10:37:50.312 PDT [1134058] CONTEXT:  while loading shared
libraries for setting "shared_preload_libraries"
from /home/maciek/code/aux/postgres/tmpdb/postgresql.auto.conf:3
2022-07-22 10:37:50.312 PDT [1134058] LOG:  database system is shut down

Hmm, I guess this is a side effect of where these messages are
emitted, but at this point, lines 4-6 here are a little confusing, no?
The server was already shut down, and we're trying to start it back
up. If there's no reasonable way to avoid that output, I think it's
okay, but it'd be better to skip it (or adjust it to the new context).

Thanks,
Maciek

---

From: Tom Lane <[email-masked]>
Date: Fri, 22 Jul 2022 13:53:21 -0400

> rather see an improvement here first rather than expanding scope.

Generally speaking, anything that tries to check a combination of
GUC settings is going to be so fragile as to be worthless.  We've
learned that lesson the hard way in the past.

> emitted, but at this point, lines 4-6 here are a little confusing, no?

This indicates that the warning is being issued in the wrong place.
It's okay if it comes out during ALTER SYSTEM.  It's not okay if it
comes out during server start; then it's just an annoyance.

			regards, tom lane

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 22 Jul 2022 13:35:57 -0500

On Fri, Jul 22, 2022 at 01:53:21PM -0400, Tom Lane wrote:
> > 2022-07-22 10:37:50.306 PDT [1134058] WARNING:  could not access file "$libdir/plugins/lol"
> > 2022-07-22 10:37:50.306 PDT [1134058] DETAIL:  The server will currently fail to start with this setting.
> > 2022-07-22 10:37:50.306 PDT [1134058] HINT:  If the server is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start again.
> > 2022-07-22 10:37:50.312 PDT [1134058] FATAL:  could not access file "lol": No such file or directory
> > 2022-07-22 10:37:50.312 PDT [1134058] CONTEXT:  while loading shared libraries for setting "shared_preload_libraries" from /home/maciek/code/aux/postgres/tmpdb/postgresql.auto.conf:3

This was a regression from the previous patch version, and I even noticed the
problem, but then forgot when returning to the patch :(

The previous patch version checked if (!IsUnderPostmaster()) before warning.
Is there a better way ?

ALTER SYSTEM uses PGC_S_FILE, the same as during startup..

-- 
Justin

---

From: Tom Lane <[email-masked]>
Date: Fri, 22 Jul 2022 15:00:23 -0400

> ALTER SYSTEM uses PGC_S_FILE, the same as during startup..

Shouldn't you be doing this when the source is PGC_S_TEST, instead?
That's pretty much what it's for.  See check_default_table_access_method
and other examples.

			regards, tom lane

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 22 Jul 2022 14:14:43 -0500

> That's pretty much what it's for.  See check_default_table_access_method

That makes sense, but it doesn't work for ALTER SYSTEM, which uses PGC_S_FILE.

postgres=# ALTER SYSTEM SET shared_preload_libraries =a;
2022-07-22 14:07:25.489 CDT client backend[23623] psql WARNING:  source 3
WARNING:  source 3
2022-07-22 14:07:25.489 CDT client backend[23623] psql WARNING:  could not access file "$libdir/plugins/a"
2022-07-22 14:07:25.489 CDT client backend[23623] psql DETAIL:  The server will currently fail to start with this setting.
2022-07-22 14:07:25.489 CDT client backend[23623] psql HINT:  If the server is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start again.

postgres=# ALTER SYSTEM SET default_table_access_method=abc;
Breakpoint 1, check_default_table_access_method (newval=0x7ffe4c6fe820, extra=0x7ffe4c6fe828, source=PGC_S_FILE) at tableamapi.c:112

-- 
Justin

---

From: Tom Lane <[email-masked]>
Date: Fri, 22 Jul 2022 15:26:47 -0400

> That makes sense, but it doesn't work for ALTER SYSTEM, which uses PGC_S_FILE.

Hmph.  I wonder if we shouldn't change that, because it's a lie.
The value isn't actually coming from the config file, at least
not yet.

We might need to invent a separate PGC_S_TEST_FILE value; or maybe it'd
be better to pass the "this is a test" flag separately?  But that'd
require changing the signature of all GUC check hooks, so probably
it's unduly invasive.  I'm not sure whether any users of the TEST
capability need to distinguish values proposed for postgresql.auto.conf
from those proposed for pg_db_role_setting ... but I guess it's
plausible that somebody might.

			regards, tom lane

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 2 Sep 2022 17:24:58 -0500

> Hmph.  I wonder if we shouldn't change that, because it's a lie.

I think so, and I was going to raise this question some months ago when
I first picked up the patch.

The question is, which behavior do we want ?

postgres=# ALTER SYSTEM SET default_table_access_method=abc;
2022-07-22 15:24:55.445 CDT client backend[27938] psql ERROR:  invalid value for parameter "default_table_access_method": "abc"
2022-07-22 15:24:55.445 CDT client backend[27938] psql DETAIL:  Table access method "abc" does not exist.
2022-07-22 15:24:55.445 CDT client backend[27938] psql STATEMENT:  ALTER SYSTEM SET default_table_access_method=abc;

That behavior differs from ALTER SYSTEM SET shared_preload_libraries,
which supports first seting the GUC and then installing the library.  If
that wasn't supported, I think we'd just throw an error and avoid the
possibility that the server can't start.

It caused no issue when I changed:

                        /* Check that it's acceptable for the indicated parameter */
                        if (!parse_and_validate_value(record, name, value,
-                                                     PGC_S_FILE, ERROR,
+                                                     PGC_S_TEST, ERROR,
                                                      &newval, &newextra))

I'm not sure where to go from here.

-- 
Justin

---

From: Michael Paquier <[email-masked]>
Date: Wed, 12 Oct 2022 14:34:09 +0900

> I'm not sure where to go from here.

Not sure either, but the thread has no activity for a bit more than 1
month, so marked as RwF for now.
--
Michael

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 29 Oct 2022 12:40:53 -0500

> I'm not sure where to go from here.

I'm hoping for some guidance ; this simple change may be naive, but I'm not
sure what a wider change would look like.

-- 
Justin

---

From: Maciek Sakrejda <[email-masked]>
Date: Sun, 30 Oct 2022 16:12:33 -0700

> sure what a wider change would look like.

I assume you mean guidance on implementation details here, and not on
design. I still think this is a useful patch and I'd be happy to
review and try out future iterations, but I don't have any useful
input here.

Also, for what it's worth, I think requiring the libraries to be in
place before running ALTER SYSTEM does not really seem that onerous. I
can't really think of use cases it precludes.

Thanks,
Maciek

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 31 Oct 2022 08:31:20 -0500

> I assume you mean guidance on implementation details here, and not on

ALTER SYSTEM tests the new/proposed setting using PGC_S_FILE ("which is
a lie").

It seems better to address that lie before attempting to change the
behavior of *_preload_libraries.

PGC_S_TEST is a better fit, so my question is whether it's really that
simple ?  

> can't really think of use cases it precludes.

Right now, it's allowed to set the GUC before installing the shlib.
That's a supported case (see the 11 month old messages toward the
beginning of this thread).

-- 
Justin

---

From: Tom Lane <[email-masked]>
Date: Mon, 31 Oct 2022 09:43:05 -0400

> beginning of this thread).

Yeah, I am afraid that you will break assorted dump/restore and
pg_upgrade scenarios if you insist on that.

			regards, tom lane

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 1 Nov 2022 17:26:34 -0500

> PGC_S_TEST is a better fit, so my question is whether it's really that

I've added the trivial change as 0001 and re-opened the patch (which ended
up in January's CF)

If for some reason it's not really as simple as that, then 001 will
serve as a "straw-man patch" hoping to elicit discussion on that point.

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 7 Jan 2024 07:27:00 -0600

On Thu, Jul 06, 2023 at 03:15:20PM -0500, Justin Pryzby wrote:
> I'm still hoping.

@cfbot: rebased