forking <ca+tgmoawonzqewe-gqmkerny1ug0z1qhbzkhda158xftohk...@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.sakre...@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
>From 0a47d84cc3536138188984d8f2b46ce5a71a8896 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From aea559e490f5b249e8b2ca02d59634e4a0de2e28 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From 2688096e81d415261cb65429550f80c201642acb Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
