Date: Sat, 8 Jan 2022 16:07:02 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Maciek Sakrejda <m.sakrejda@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: warn if GUC set to an invalid shared library
Message-ID: <20220108220702.GK14051@telsasoft.com>
References: <20211228174557.GI24477@telsasoft.com>
 <CAOtHd0BtAEQukewfXyWBtyDeiM5BvHPVCHu3dKaBmtrU8Z0=Hg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="IpgPcFyQO6wM49Um"
Content-Disposition: inline
In-Reply-To: 
 <CAOtHd0BtAEQukewfXyWBtyDeiM5BvHPVCHu3dKaBmtrU8Z0=Hg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220108220702.GK14051%40telsasoft.com>
Precedence: bulk


--IpgPcFyQO6wM49Um
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sat, Jan 08, 2022 at 01:29:24PM -0800, Maciek Sakrejda wrote:
> Thanks for working on this! I tried it out and it worked for me. I
> reviewed the patch and didn't see any problems, but I'm not much of a
> C programmer.

Thanks for looking at it.  I was just hacking on it myself.

Unfortunately, the output for dlopen() is not portable, which (I think) means
most of what I wrote can't be made to work..  Since it doesn't work to call
dlopen() when using SET, I tried using just stat().  But that also fails on
windows, since one of the regression tests has an invalid filename involving
unbalanced quotes, which cause it to return EINVAL rather than ENOENT.  So SET
cannot warn portably, unless it includes no details at all (or we specially
handle the windows case), or change the pre-existing regression test.  But
there's a 2nd instability, too, apparently having to do with timing.  So I'm
planning to drop the 0001 patch.

> On Tue, Dec 28, 2021 at 9:45 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > 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
> 
> For whatever reason, I get slightly different (and somewhat redundant)
> output on failing to start:
> 
> 2022-01-08 12:59:36.784 PST [324482] WARNING:  could not load library: $libdir/plugins/totally bogus: cannot open shared object file: No such file or directory
> 2022-01-08 12:59:36.787 PST [324482] FATAL:  could not load library: totally bogus: cannot open shared object file: No such file or directory
> 2022-01-08 12:59:36.787 PST [324482] LOG:  database system is shut down

I think the first WARNING is from the GUC mechanism "setting" the library.
And then the FATAL is from trying to apply the GUC.
It looks like you didn't apply the 0002 patch for that test so got no CONTEXT ?

$ ./tmp_install/usr/local/pgsql/bin/postgres -D src/test/regress/tmp_check/data -c shared_preload_libraries=asdf
2022-01-08 16:05:00.050 CST postmaster[2588] FATAL:  could not access file "asdf": No such file or directory
2022-01-08 16:05:00.050 CST postmaster[2588] CONTEXT:  while loading shared libraries for GUC "shared_preload_libraries"
2022-01-08 16:05:00.050 CST postmaster[2588] LOG:  database system is shut down

-- 
Justin

--IpgPcFyQO6wM49Um
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v2-0001-errcontext-if-server-fails-to-start-due-to-librar.patch"

From 91fab1e4bf6806c1f4b571ac8abde505446b8621 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 18 Dec 2021 22:51:01 -0600
Subject: [PATCH v2] 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..40eed5b5530 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 GUC \"%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 0f2570d6264..5c295e44ff8 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1658,7 +1658,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


--IpgPcFyQO6wM49Um--