warn if GUC set to an invalid shared library
forking <CA+TgmoawONZqEwe-GqmKERNY1ug0z1QhBzkHdA158xfToHKN9w@mail.gmail.com>
On Mon, Dec 13, 2021 at 09:01:57AM -0500, Robert Haas wrote:
On Thu, Dec 9, 2021 at 2:32 AM Maciek Sakrejda <m.sakrejda@gmail.com> 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
Attachments:
0001-warn-if-shared_preload_libraries-refers-to-an-nonext.patchtext/x-diff; charset=us-asciiDownload
From 0a47d84cc3536138188984d8f2b46ce5a71a8896 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH 1/3] warn if shared_preload_libraries refers to an nonextant
library
---
src/backend/utils/fmgr/dfmgr.c | 100 ++++++++++--------
src/backend/utils/misc/guc.c | 81 +++++++++++++-
src/include/fmgr.h | 1 +
.../unsafe_tests/expected/rolenames.out | 1 +
src/test/regress/expected/rules.out | 6 ++
src/test/regress/sql/rules.sql | 1 +
6 files changed, 145 insertions(+), 45 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index cc1a5200705..474de52d349 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -173,6 +173,62 @@ lookup_external_function(void *filehandle, const char *funcname)
return dlsym(filehandle, funcname);
}
+/*
+ * Attempt to load the library with the given filename.
+ * If it fails, issue an error as specified and return NULL.
+ */
+void *
+try_open_library(const char *libname, int errlevel)
+{
+ void *dlh;
+ PGModuleMagicFunction magic_func;
+ const Pg_magic_struct *magic_data_ptr;
+
+ dlh = dlopen(libname, RTLD_NOW | RTLD_GLOBAL);
+ if (dlh == NULL)
+ {
+ char *load_error = dlerror();
+ /* errcode_for_file_access might not be appropriate here? */
+ ereport(errlevel,
+ (errcode_for_file_access(),
+ errmsg("could not load library: %s",
+ load_error)));
+ return NULL;
+ }
+
+ /* Check the magic function to determine compatibility */
+ magic_func = (PGModuleMagicFunction) dlsym(dlh,
+ PG_MAGIC_FUNCTION_NAME_STRING);
+ if (magic_func == NULL)
+ {
+ /* try to close library */
+ dlclose(dlh);
+ /* complain */
+ ereport(errlevel,
+ (errmsg("incompatible library \"%s\": missing magic block",
+ libname),
+ errhint("Extension libraries are required to use the PG_MODULE_MAGIC macro.")));
+ return NULL;
+ }
+
+ magic_data_ptr = (*magic_func) ();
+
+ if (magic_data_ptr->len != magic_data.len ||
+ memcmp(magic_data_ptr, &magic_data, magic_data.len) != 0)
+ {
+ /* copy data block before unlinking library */
+ Pg_magic_struct module_magic_data = *magic_data_ptr;
+
+ /* try to close library */
+ dlclose(dlh);
+
+ /* issue suitable complaint */
+ incompatible_module_error(libname, &module_magic_data);
+ return NULL;
+ }
+
+ return dlh;
+}
/*
* Load the specified dynamic-link library file, unless it already is
@@ -184,8 +240,6 @@ static void *
internal_load_library(const char *libname)
{
DynamicFileList *file_scanner;
- PGModuleMagicFunction magic_func;
- char *load_error;
struct stat stat_buf;
PG_init_t PG_init;
@@ -236,49 +290,11 @@ internal_load_library(const char *libname)
#endif
file_scanner->next = NULL;
- file_scanner->handle = dlopen(file_scanner->filename, RTLD_NOW | RTLD_GLOBAL);
+ file_scanner->handle = try_open_library(libname, ERROR);
if (file_scanner->handle == NULL)
{
- load_error = dlerror();
+ /* Not needed since we would've issued an ERROR ? */
free((char *) file_scanner);
- /* errcode_for_file_access might not be appropriate here? */
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not load library \"%s\": %s",
- libname, load_error)));
- }
-
- /* Check the magic function to determine compatibility */
- magic_func = (PGModuleMagicFunction)
- dlsym(file_scanner->handle, PG_MAGIC_FUNCTION_NAME_STRING);
- if (magic_func)
- {
- const Pg_magic_struct *magic_data_ptr = (*magic_func) ();
-
- if (magic_data_ptr->len != magic_data.len ||
- memcmp(magic_data_ptr, &magic_data, magic_data.len) != 0)
- {
- /* copy data block before unlinking library */
- Pg_magic_struct module_magic_data = *magic_data_ptr;
-
- /* try to close library */
- dlclose(file_scanner->handle);
- free((char *) file_scanner);
-
- /* issue suitable complaint */
- incompatible_module_error(libname, &module_magic_data);
- }
- }
- else
- {
- /* try to close library */
- dlclose(file_scanner->handle);
- free((char *) file_scanner);
- /* complain */
- ereport(ERROR,
- (errmsg("incompatible library \"%s\": missing magic block",
- libname),
- errhint("Extension libraries are required to use the PG_MODULE_MAGIC macro.")));
}
/*
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index bff949a40bc..17cfda9d200 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -169,6 +169,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,
@@ -4178,7 +4184,7 @@ static struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4189,7 +4195,7 @@ static struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4200,7 +4206,7 @@ static struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
@@ -12133,6 +12139,75 @@ 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)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ 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;
+ void *dlh;
+
+ /* 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() or access() ?
+
+ dlh = try_open_library(filename, WARNING);
+ if (dlh && false)
+ ; /* Do nothing? dlclose(dlh); */
+ }
+
+ 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);
+}
+
+static bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
+
+static bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
static bool
check_effective_io_concurrency(int *newval, void **extra, GucSource source)
{
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index cec663bdff0..0c27b617487 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 *try_open_library(const char *libname, int errlevel);
extern void **find_rendezvous_variable(const char *varName);
extern Size EstimateLibraryStateSpace(void);
extern void SerializeLibraryState(Size maxsize, char *start_address);
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index eb608fdc2ea..bd45cd26c43 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 load library: $libdir/plugins/path-to-preload-libraries: cannot open shared object file: No such file or directory
SET SESSION AUTHORIZATION regress_role_haspriv;
-- passes with role member of pg_read_all_settings
SHOW session_preload_libraries;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index b58b062b10d..d8bf63886c5 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3572,6 +3572,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
@@ -3581,6 +3582,11 @@ 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 load library: Mixed/Case: cannot open shared object file: No such file or directory
+WARNING: could not load library: c:/'a"/path: cannot open shared object file: No such file or directory
+WARNING: incompatible library "": missing magic block
+HINT: Extension libraries are required to use the PG_MODULE_MAGIC macro.
+WARNING: could not load library: 0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789: cannot open shared object file: No such file or directory
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 b732833e63c..1e42d092862 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
0002-errcontext-if-server-fails-to-start-due-to-library-G.patchtext/x-diff; charset=us-asciiDownload
From aea559e490f5b249e8b2ca02d59634e4a0de2e28 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 18 Dec 2021 22:51:01 -0600
Subject: [PATCH 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 474de52d349..a9e2ec5aafa 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);
}
@@ -235,9 +243,10 @@ try_open_library(const char *libname, int errlevel)
* 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;
struct stat stat_buf;
@@ -260,6 +269,7 @@ internal_load_library(const char *libname)
if (stat(libname, &stat_buf) == -1)
ereport(ERROR,
(errcode_for_file_access(),
+ gucname ? errcontext("guc \"%s\"", gucname) : 0,
errmsg("could not access file \"%s\": %m",
libname)));
@@ -780,7 +790,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 88801374b57..8218ca339a4 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 0c27b617487..5ad6d6f282b 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 *try_open_library(const char *libname, int errlevel);
extern void **find_rendezvous_variable(const char *varName);
extern Size EstimateLibraryStateSpace(void);
--
2.17.1
0003-wip-allow-dlopen-to-error-rather-than-stat.patchtext/x-diff; charset=us-asciiDownload
From 2688096e81d415261cb65429550f80c201642acb Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 22 Dec 2021 19:24:43 -0600
Subject: [PATCH 3/3] wip: allow dlopen to error() rather than stat()
TODO: test warning message:
./
/dev/null
postmaster.pid
/bin/false
non library
---
contrib/test_decoding/expected/slot.out | 2 +-
src/backend/utils/fmgr/dfmgr.c | 20 ++++++++-----------
.../regress/expected/create_function_0.out | 2 +-
3 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/contrib/test_decoding/expected/slot.out b/contrib/test_decoding/expected/slot.out
index 63a9940f73a..48085ad4ef5 100644
--- a/contrib/test_decoding/expected/slot.out
+++ b/contrib/test_decoding/expected/slot.out
@@ -31,7 +31,7 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot_t2', 'tes
(1 row)
SELECT pg_create_logical_replication_slot('foo', 'nonexistent');
-ERROR: could not access file "nonexistent": No such file or directory
+ERROR: could not load library: nonexistent: cannot open shared object file: No such file or directory
-- here we want to start a new session and wait till old one is gone
select pg_backend_pid() as oldpid \gset
\c -
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index a9e2ec5aafa..c68037a3a8f 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -266,18 +266,14 @@ 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)
- ereport(ERROR,
- (errcode_for_file_access(),
- gucname ? errcontext("guc \"%s\"", gucname) : 0,
- errmsg("could not access file \"%s\": %m",
- libname)));
-
- for (file_scanner = file_list;
- file_scanner != NULL &&
- !SAME_INODE(stat_buf, *file_scanner);
- file_scanner = file_scanner->next)
- ;
+ if (stat(libname, &stat_buf) != -1)
+ {
+ for (file_scanner = file_list;
+ file_scanner != NULL &&
+ !SAME_INODE(stat_buf, *file_scanner);
+ file_scanner = file_scanner->next)
+ ;
+ }
}
if (file_scanner == NULL)
diff --git a/src/test/regress/expected/create_function_0.out b/src/test/regress/expected/create_function_0.out
index 6e96d6c5d66..476be3d0544 100644
--- a/src/test/regress/expected/create_function_0.out
+++ b/src/test/regress/expected/create_function_0.out
@@ -85,7 +85,7 @@ CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL
ERROR: only one AS item needed for language "sql"
CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C
AS 'nosuchfile';
-ERROR: could not access file "nosuchfile": No such file or directory
+ERROR: could not load library: nosuchfile: cannot open shared object file: No such file or directory
-- To produce stable regression test output, we have to filter the name
-- of the regresslib file out of the error message in this test.
\set VERBOSITY sqlstate
--
2.17.1
On Tue, Dec 28, 2021 at 11:15 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
forking <CA+TgmoawONZqEwe-GqmKERNY1ug0z1QhBzkHdA158xfToHKN9w@mail.gmail.com>
On Mon, Dec 13, 2021 at 09:01:57AM -0500, Robert Haas wrote:
On Thu, Dec 9, 2021 at 2:32 AM Maciek Sakrejda <m.sakrejda@gmail.com> 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
SETpostgres=# 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 SYSTEM0002 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 downBut 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 downAfter 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
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.
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.
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'm on a pretty standard Ubuntu 20.04 (with PGDG packages installed
for 11, 12, and 13). I did configure to a --prefix and set
LD_LIBRARY_PATH and PATH. Not sure if this is an issue in my
environment or a platform difference?
Also, regarding the original warning:
2022-01-08 12:57:24.953 PST [324338] WARNING: could not load library:
$libdir/plugins/totally bogus: cannot open shared object file: No such
file or directory
I think this is pretty clear to users familiar with
shared_preload_libraries. However, for someone less experienced with
Postgres and just following instructions on how to set up, e.g.,
auto_explain (and making a typo), it's not clear from this message
that your server will fail to start again after this if you shut it
down (or it crashes!), and how to get out of this situation. Should we
add a HINT to that effect?
Similarly, for
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
This works for me, but should this explain the impact (especially if
used with something like ALTER ROLE)? I guess that's probably because
the context may not be easily accessible. I think
shared_preload_libraries is much more common, though, so I'm more
interested in a warning there.
Thanks,
Maciek
On Thu, Dec 30, 2021 at 12:21 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
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 don't think so--I'm skeptical that "updated shared_preload_libraries
first, then install them" is much more than a theoretical use case. We
may not want to block that off completely, but I think a warning is
reasonable here, because you're *probably* doing something wrong if
you get to this message at all (and if you're not, you're probably
familiar enough with Postgres to know to ignore the warning).
Thanks,
Maciek
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 downFor 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
Attachments:
v2-0001-errcontext-if-server-fails-to-start-due-to-librar.patchtext/x-diff; charset=us-asciiDownload
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
On Sat, Jan 8, 2022 at 2:07 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
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.
Hmm. I think 001 is a big part of the usability improvement here.
Could we not at least warn generically, without relaying the
underlying error? The notable thing in this situation is that the
specified library could not be loaded (and that it will almost
certainly cause problems on restart). The specific error would be nice
to have, but it's less important. What is the timing instability?
On Tue, Dec 28, 2021 at 9:45 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
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 downI 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 ?
I still had the terminal open where I tested this, and the scrollback
did show me applying the patch (and building after). I tried a make
clean and applying the patch again, and I do see the CONTEXT line now.
I'm not sure what the problem was but seems like PEBKAC--sorry about
that.
Thanks,
Maciek
On Sun, Jan 09, 2022 at 11:58:18AM -0800, Maciek Sakrejda wrote:
On Sat, Jan 8, 2022 at 2:07 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
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.Hmm. I think 001 is a big part of the usability improvement here.
I agree - it helps people avoid causing a disruption, rather than just helping
them to fix it faster.
Could we not at least warn generically, without relaying the
underlying error? The notable thing in this situation is that the
specified library could not be loaded (and that it will almost
certainly cause problems on restart). The specific error would be nice
to have, but it's less important. What is the timing instability?
I saw regression diffs like this, showing that the warning could be displayed
before or after the SELECT was echoed.
https://cirrus-ci.com/task/6301672321318912
-SELECT * FROM schema4.counted;
WARNING: could not load library: $libdir/plugins/worker_spi: cannot open shared object file: No such file or directory
+SELECT * FROM schema4.counted;
It's certainly possible to show a static message without additional text from
errno.
On Tue, Dec 28, 2021 at 9:45 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
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 downI 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 ?I still had the terminal open where I tested this, and the scrollback
did show me applying the patch (and building after). I tried a make
clean and applying the patch again, and I do see the CONTEXT line now.
I'm not sure what the problem was but seems like PEBKAC--sorry about
that.
Maybe you missed "make install" or similar.
I took the liberty of adding you as a reviewer here:
https://commitfest.postgresql.org/36/3482/
--
Justin
Attachments:
v3-0001-errcontext-if-server-fails-to-start-due-to-librar.patchtext/x-diff; charset=us-asciiDownload
From 53bf0c82ea76a00fbf1c95c4833f1bd302228d25 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 18 Dec 2021 22:51:01 -0600
Subject: [PATCH v3 1/2] 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
v3-0002-warn-when-setting-GUC-to-a-nonextant-library.patchtext/x-diff; charset=us-asciiDownload
From 76f2caade9c0e3554ecb645c99ad148e593a9fe6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH v3 2/2] warn when setting GUC to a nonextant library
XXX: the SPI test is unstable:
-SELECT * FROM schema4.counted;
WARNING: could not load library: $libdir/plugins/worker_spi: cannot open shared object file: No such file or directory
+SELECT * FROM schema4.counted;
---
src/backend/utils/misc/guc.c | 85 ++++++++++++++++++-
.../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 | 5 ++
src/test/regress/sql/rules.sql | 1 +
6 files changed, 93 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 4c94f09c645..5a228a37ee2 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,
@@ -4188,7 +4194,7 @@ static struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4199,7 +4205,7 @@ static struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4210,7 +4216,7 @@ static struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
@@ -12100,6 +12106,79 @@ 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)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ 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) == -1) // F_OK
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename));
+ }
+
+ 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);
+}
+
+static bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
+
+static bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, 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 d652f7b5fb4..ce396814332 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3574,6 +3574,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
@@ -3583,6 +3584,10 @@ 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"
+WARNING: could not access file "c:/'a"/path"
+WARNING: could not access file ""
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
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 b732833e63c..1e42d092862 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
On Tue, Dec 28, 2021 at 12:45 PM 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
-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
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
Thanks for loooking
On Fri, Jan 28, 2022 at 11:36:20PM +0000, Cary Huang wrote:
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
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
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: 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:
On Fri, Jan 28, 2022 at 6:42 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Dec 28, 2021 at 12:45 PM 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-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.
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
On Tue, Feb 1, 2022 at 11:06 PM Maciek Sakrejda <m.sakrejda@gmail.com>
wrote:
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 SYSTEMI 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.
On Wed, Feb 2, 2022 at 7:39 AM David G. Johnston <david.g.johnston@gmail.com>
wrote:
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.
While I don't have a problem with that behavior, given that there are
currently no such facilities for asserting "yes, really" with ALTER SYSTEM,
I don't think it's worth introducing that just for this patch. A warning
seems like a reasonable first step. This can always be expanded later. I'd
rather see a warning ship than move the goalposts out of reach.