agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedwarn if GUC set to an invalid shared library
42+ messages / 11 participants
[nested] [flat]
* warn if GUC set to an invalid shared library
@ 2021-12-28 17:45 Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 3 replies; 42+ messages in thread
From: Justin Pryzby @ 2021-12-28 17:45 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Maciek Sakrejda <m.sakrejda@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; pgsql-hackers@lists.postgresql.org
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:
[text/x-diff] 0001-warn-if-shared_preload_libraries-refers-to-an-nonext.patch (11.2K, ../../20211228174557.GI24477@telsasoft.com/2-0001-warn-if-shared_preload_libraries-refers-to-an-nonext.patch)
download | inline diff:
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
[text/x-diff] 0002-errcontext-if-server-fails-to-start-due-to-library-G.patch (4.2K, ../../20211228174557.GI24477@telsasoft.com/3-0002-errcontext-if-server-fails-to-start-due-to-library-G.patch)
download | inline diff:
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
[text/x-diff] 0003-wip-allow-dlopen-to-error-rather-than-stat.patch (2.9K, ../../20211228174557.GI24477@telsasoft.com/4-0003-wip-allow-dlopen-to-error-rather-than-stat.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2021-12-30 08:20 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
2 siblings, 1 reply; 42+ messages in thread
From: Bharath Rupireddy @ 2021-12-30 08:20 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Maciek Sakrejda <m.sakrejda@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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
> 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
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.
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-01-08 21:29 Maciek Sakrejda <m.sakrejda@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
2 siblings, 1 reply; 42+ messages in thread
From: Maciek Sakrejda @ 2022-01-08 21:29 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.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>
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
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-01-08 21:32 Maciek Sakrejda <m.sakrejda@gmail.com>
parent: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
0 siblings, 0 replies; 42+ messages in thread
From: Maciek Sakrejda @ 2022-01-08 21:32 UTC (permalink / raw)
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Robert Haas <robertmhaas@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-01-08 22:07 Justin Pryzby <pryzby@telsasoft.com>
parent: Maciek Sakrejda <m.sakrejda@gmail.com>
0 siblings, 1 reply; 42+ messages in thread
From: Justin Pryzby @ 2022-01-08 22:07 UTC (permalink / raw)
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>
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
Attachments:
[text/x-diff] v2-0001-errcontext-if-server-fails-to-start-due-to-librar.patch (4.2K, ../../20220108220702.GK14051@telsasoft.com/2-v2-0001-errcontext-if-server-fails-to-start-due-to-librar.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-01-09 19:58 Maciek Sakrejda <m.sakrejda@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 42+ messages in thread
From: Maciek Sakrejda @ 2022-01-09 19:58 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.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>
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 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 ?
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
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-01-27 22:01 Justin Pryzby <pryzby@telsasoft.com>
parent: Maciek Sakrejda <m.sakrejda@gmail.com>
0 siblings, 0 replies; 42+ messages in thread
From: Justin Pryzby @ 2022-01-27 22:01 UTC (permalink / raw)
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>; pgsql-hackers@lists.postgresql.org
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 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 ?
>
> 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:
[text/x-diff] v3-0001-errcontext-if-server-fails-to-start-due-to-librar.patch (4.2K, ../../20220127220103.GY23027@telsasoft.com/2-v3-0001-errcontext-if-server-fails-to-start-due-to-librar.patch)
download | inline diff:
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
[text/x-diff] v3-0002-warn-when-setting-GUC-to-a-nonextant-library.patch (8.0K, ../../20220127220103.GY23027@telsasoft.com/3-v3-0002-warn-when-setting-GUC-to-a-nonextant-library.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-01-28 14:42 Robert Haas <robertmhaas@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
2 siblings, 2 replies; 42+ messages in thread
From: Robert Haas @ 2022-01-28 14:42 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Maciek Sakrejda <m.sakrejda@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-01-28 23:36 Cary Huang <cary.huang@highgo.ca>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 42+ messages in thread
From: Cary Huang @ 2022-01-28 23:36 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Justin Pryzby <justin@telsasoft.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
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-01-29 00:09 Justin Pryzby <pryzby@telsasoft.com>
parent: Cary Huang <cary.huang@highgo.ca>
0 siblings, 1 reply; 42+ messages in thread
From: Justin Pryzby @ 2022-01-29 00:09 UTC (permalink / raw)
To: Cary Huang <cary.huang@highgo.ca>; +Cc: pgsql-hackers@lists.postgresql.org, Justin Pryzby <justin@telsasoft.com>
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
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-02-02 06:06 Maciek Sakrejda <m.sakrejda@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 42+ messages in thread
From: Maciek Sakrejda @ 2022-02-02 06:06 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Justin Pryzby <justin@telsasoft.com>
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
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-02-02 15:39 David G. Johnston <david.g.johnston@gmail.com>
parent: Maciek Sakrejda <m.sakrejda@gmail.com>
0 siblings, 1 reply; 42+ messages in thread
From: David G. Johnston @ 2022-02-02 15:39 UTC (permalink / raw)
To: Maciek Sakrejda <m.sakrejda@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Justin Pryzby <justin@telsasoft.com>
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 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.
+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.
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-02-04 04:36 Maciek Sakrejda <m.sakrejda@gmail.com>
parent: David G. Johnston <david.g.johnston@gmail.com>
0 siblings, 0 replies; 42+ messages in thread
From: Maciek Sakrejda @ 2022-02-04 04:36 UTC (permalink / raw)
To: David G. Johnston <david.g.johnston@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Justin Pryzby <justin@telsasoft.com>
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.
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-02-10 01:58 Justin Pryzby <pryzby@telsasoft.com>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 42+ messages in thread
From: Justin Pryzby @ 2022-02-10 01:58 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; Maciek Sakrejda <m.sakrejda@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; pgsql-hackers@lists.postgresql.org
On Fri, Jan 28, 2022 at 09:42:17AM -0500, Robert Haas wrote:
> -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.
On Wed, Feb 02, 2022 at 06:06:01AM +0000, Maciek Sakrejda wrote:
> I do agree that GUC is awkward here, and I like the "while..." wording suggested both for consistency with other messages and how it could work here:
> CONTEXT: while loading "shared_preload_libraries"
FYI, it has said "while..." and hasn't said "guc" since the 2nd revision of the
patch.
> 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.
I avoided the warning by checking IsUnderPostmaster, though I'm not sure if
that's the right condition..
On Wed, Feb 02, 2022 at 06:06:01AM +0000, Maciek Sakrejda wrote:
> I think this is great, but it would be really helpful to also indicate that at this point the server will fail to come back up after a restart.
> I don't really know a good wording here, but maybe a hint like "The server or session will not be able to start if it has been configured to use libraries that cannot be loaded."?
postgres=# ALTER SYSTEM SET shared_preload_libraries =a,b;
WARNING: could not access file "$libdir/plugins/a"
HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
WARNING: could not access file "$libdir/plugins/b"
HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
ALTER SYSTEM
postgres=# ALTER SYSTEM SET session_preload_libraries =c,d;
WARNING: could not access file "$libdir/plugins/c"
HINT: New sessions will fail with the existing configuration.
WARNING: could not access file "$libdir/plugins/d"
HINT: New sessions will fail with the existing configuration.
ALTER SYSTEM
$ ./tmp_install/usr/local/pgsql/bin/postgres -D src/test/regress/tmp_check/data -clogging_collector=on
2022-02-09 19:53:48.034 CST postmaster[30979] FATAL: could not access file "a": No such file or directory
2022-02-09 19:53:48.034 CST postmaster[30979] CONTEXT: while loading shared libraries for setting "shared_preload_libraries"
from /home/pryzbyj/src/postgres/src/test/regress/tmp_check/data/postgresql.auto.conf:3
2022-02-09 19:53:48.034 CST postmaster[30979] LOG: database system is shut down
Maybe it's enough to show the GucSource rather than file:line...
--
Justin
Attachments:
[text/x-diff] v4-0001-warn-when-setting-GUC-to-a-nonextant-library.patch (9.2K, ../../20220210015854.GV31460@telsasoft.com/2-v4-0001-warn-when-setting-GUC-to-a-nonextant-library.patch)
download | inline diff:
From 69560e81bfbb43a67269f54b564fe8ac5ae1b5c6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH v4 1/3] warn when setting GUC to a nonextant library
---
src/backend/utils/misc/guc.c | 103 +++++++++++++++++-
.../unsafe_tests/expected/rolenames.out | 1 +
.../worker_spi/expected/worker_spi.out | 2 +
.../modules/worker_spi/sql/worker_spi.sql | 2 +
src/test/regress/expected/rules.out | 9 ++
src/test/regress/sql/rules.sql | 1 +
6 files changed, 115 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f505413a7f9..c44b8ebbfd6 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -170,6 +170,12 @@ static bool call_enum_check_hook(struct config_enum *conf, int *newval,
void **extra, GucSource source, int elevel);
static bool check_log_destination(char **newval, void **extra, GucSource source);
+static bool check_local_preload_libraries(char **newval, void **extra,
+ GucSource source);
+static bool check_session_preload_libraries(char **newval, void **extra,
+ GucSource source);
+static bool check_shared_preload_libraries(char **newval, void **extra,
+ GucSource source);
static void assign_log_destination(const char *newval, void *extra);
static bool check_wal_consistency_checking(char **newval, void **extra,
@@ -4198,7 +4204,7 @@ static struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4209,7 +4215,7 @@ static struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4220,7 +4226,7 @@ static struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
@@ -12149,6 +12155,97 @@ check_max_worker_processes(int *newval, void **extra, GucSource source)
return true;
}
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+ bool restricted, bool is_session)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ return true;
+
+ /*
+ * Do not issue warnings while applying GUCs during startup. That would
+ * issue a FATAL error message, and the warnings would be redundant.
+ */
+ if (!IsUnderPostmaster)
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages from
+ * dlopen() are not portable, so it'd be hard to report any problem
+ * other than "does not exist".
+ */
+ if (access(filename, R_OK) == 0) // F_OK
+ continue;
+
+ if (is_session)
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errhint("New sessions will fail with the existing configuration."));
+ else
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errhint("The server will fail to start with the existing configuration."
+ " If it is is shut down, it will be necessary to manually edit the %s file to allow it to start.",
+ "postgresql.auto.conf"));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+static bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true, false);
+}
+
+static bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false, false);
+}
+
+static bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true, true);
+}
+
static bool
check_effective_io_concurrency(int *newval, void **extra, GucSource source)
{
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index eb608fdc2ea..368b10558d7 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1066,6 +1066,7 @@ GRANT pg_read_all_settings TO regress_role_haspriv;
BEGIN;
-- A GUC using GUC_SUPERUSER_ONLY is useful for negative tests.
SET LOCAL session_preload_libraries TO 'path-to-preload-libraries';
+WARNING: could not access file "$libdir/plugins/path-to-preload-libraries"
SET SESSION AUTHORIZATION regress_role_haspriv;
-- passes with role member of pg_read_all_settings
SHOW session_preload_libraries;
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
index dc0a79bf759..5b275669bcf 100644
--- a/src/test/modules/worker_spi/expected/worker_spi.out
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -28,6 +28,8 @@ SELECT pg_reload_conf();
t
(1 row)
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
index 4683523b29d..4ed5370d456 100644
--- a/src/test/modules/worker_spi/sql/worker_spi.sql
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -18,6 +18,8 @@ END
$$;
INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
SELECT pg_reload_conf();
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 1420288d67b..f3d33dda95e 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3362,6 +3362,7 @@ drop table hats;
drop table hat_data;
-- test for pg_get_functiondef properly regurgitating SET parameters
-- Note that the function is kept around to stress pg_dump.
+SET check_function_bodies=no;
CREATE FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
@@ -3371,6 +3372,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
SET datestyle to iso, mdy
SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
+WARNING: could not access file "Mixed/Case"
+HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
+WARNING: could not access file "c:/'a"/path"
+HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
+WARNING: could not access file ""
+HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start.
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
pg_get_functiondef
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index 8bdab6dec30..6308e5d27fd 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1198,6 +1198,7 @@ drop table hat_data;
-- test for pg_get_functiondef properly regurgitating SET parameters
-- Note that the function is kept around to stress pg_dump.
+SET check_function_bodies=no;
CREATE FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
--
2.17.1
[text/x-diff] v4-0002-errcontext-if-server-fails-to-start-due-to-librar.patch (4.2K, ../../20220210015854.GV31460@telsasoft.com/3-v4-0002-errcontext-if-server-fails-to-start-due-to-librar.patch)
download | inline diff:
From 8280f86eb5ebc2f4785f3df215ad71e690647c7f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 18 Dec 2021 22:51:01 -0600
Subject: [PATCH v4 2/3] errcontext if server fails to start due to library
GUCs
---
src/backend/utils/fmgr/dfmgr.c | 20 +++++++++++++++-----
src/backend/utils/init/miscinit.c | 2 +-
src/include/fmgr.h | 1 +
3 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 050da780804..fcda19b7973 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -76,7 +76,7 @@ static DynamicFileList *file_tail = NULL;
char *Dynamic_library_path;
-static void *internal_load_library(const char *libname);
+static void *internal_load_library(const char *libname, const char *gucname);
static void incompatible_module_error(const char *libname,
const Pg_magic_struct *module_magic_data) pg_attribute_noreturn();
static void internal_unload_library(const char *libname);
@@ -115,7 +115,7 @@ load_external_function(const char *filename, const char *funcname,
fullname = expand_dynamic_library_name(filename);
/* Load the shared library, unless we already did */
- lib_handle = internal_load_library(fullname);
+ lib_handle = internal_load_library(fullname, NULL);
/* Return handle if caller wants it */
if (filehandle)
@@ -144,6 +144,14 @@ load_external_function(const char *filename, const char *funcname,
*/
void
load_file(const char *filename, bool restricted)
+{
+ load_file_guc(filename, restricted, NULL);
+}
+
+/*
+ * Also accepts a GUC arg, for error reports
+ */
+void load_file_guc(const char *filename, bool restricted, const char *gucname)
{
char *fullname;
@@ -158,7 +166,7 @@ load_file(const char *filename, bool restricted)
internal_unload_library(fullname);
/* Load the shared library */
- (void) internal_load_library(fullname);
+ (void) internal_load_library(fullname, gucname);
pfree(fullname);
}
@@ -179,9 +187,10 @@ lookup_external_function(void *filehandle, const char *funcname)
* loaded. Return the pg_dl* handle for the file.
*
* Note: libname is expected to be an exact name for the library file.
+ * gucname may be passed for error reports.
*/
static void *
-internal_load_library(const char *libname)
+internal_load_library(const char *libname, const char *gucname)
{
DynamicFileList *file_scanner;
PGModuleMagicFunction magic_func;
@@ -206,6 +215,7 @@ internal_load_library(const char *libname)
if (stat(libname, &stat_buf) == -1)
ereport(ERROR,
(errcode_for_file_access(),
+ gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
errmsg("could not access file \"%s\": %m",
libname)));
@@ -764,7 +774,7 @@ RestoreLibraryState(char *start_address)
{
while (*start_address != '\0')
{
- internal_load_library(start_address);
+ internal_load_library(start_address, NULL);
start_address += strlen(start_address) + 1;
}
}
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 0868e5a24f6..17d13e6c6bf 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1659,7 +1659,7 @@ load_libraries(const char *libraries, const char *gucname, bool restricted)
expanded = psprintf("$libdir/plugins/%s", filename);
filename = expanded;
}
- load_file(filename, restricted);
+ load_file_guc(filename, restricted, gucname);
ereport(DEBUG1,
(errmsg_internal("loaded library \"%s\"", filename)));
if (expanded)
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index 6560e462d66..9dccd0fc047 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -727,6 +727,7 @@ extern void *load_external_function(const char *filename, const char *funcname,
bool signalNotFound, void **filehandle);
extern void *lookup_external_function(void *filehandle, const char *funcname);
extern void load_file(const char *filename, bool restricted);
+extern void load_file_guc(const char *filename, bool restricted, const char *gucname);
extern void **find_rendezvous_variable(const char *varName);
extern Size EstimateLibraryStateSpace(void);
extern void SerializeLibraryState(Size maxsize, char *start_address);
--
2.17.1
[text/x-diff] v4-0003-show-the-GUC-source-in-errcontext.patch (3.2K, ../../20220210015854.GV31460@telsasoft.com/4-v4-0003-show-the-GUC-source-in-errcontext.patch)
download | inline diff:
From 2da099b709b84bb446c3555d111f2f9a6084dace Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 2 Feb 2022 20:54:49 -0600
Subject: [PATCH v4 3/3] show the GUC source in errcontext
---
src/backend/utils/fmgr/dfmgr.c | 13 ++++++++++---
src/backend/utils/misc/guc.c | 21 +++++++++++++++++++++
src/include/utils/guc.h | 1 +
3 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index fcda19b7973..b679de0455e 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -34,6 +34,7 @@
#include "lib/stringinfo.h"
#include "miscadmin.h"
#include "storage/shmem.h"
+#include "utils/guc_tables.h"
#include "utils/hsearch.h"
@@ -213,11 +214,17 @@ internal_load_library(const char *libname, const char *gucname)
* Check for same files - different paths (ie, symlink or link)
*/
if (stat(libname, &stat_buf) == -1)
+ {
+ char *errstr = strerror(errno);
+ int linenum;
+ char *sourcefile = gucname ? GetConfigSourceFile(gucname, &linenum) : NULL;
ereport(ERROR,
(errcode_for_file_access(),
gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
- errmsg("could not access file \"%s\": %m",
- libname)));
+ sourcefile ? errcontext("from %s:%d", sourcefile, linenum) : 0,
+ errmsg("could not access file \"%s\": %s",
+ libname, errstr)));
+ }
for (file_scanner = file_list;
file_scanner != NULL &&
@@ -281,7 +288,7 @@ internal_load_library(const char *libname, const char *gucname)
}
else
{
- /* try to close library */
+ /* try to close library */ // Not needed due to ERROR ? //
dlclose(file_scanner->handle);
free((char *) file_scanner);
/* complain */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index c44b8ebbfd6..ee9e375039f 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8283,6 +8283,27 @@ GetConfigOptionFlags(const char *name, bool missing_ok)
return record->flags;
}
+/*
+ * Get the source file and line associated with the given option.
+ *
+ * If the option doesn't exist, return 0 if missing_ok is true,
+ * otherwise throw an ereport and don't return.
+ */
+char *
+GetConfigSourceFile(const char *name, int *linenum)
+{
+ struct config_generic *record;
+
+ record = find_option(name, false, false, ERROR);
+
+ /* Should not happen */
+ if (record == NULL)
+ return 0;
+
+ *linenum = record->sourceline;
+ return record->sourcefile;
+}
+
/*
* flatten_set_variable_args
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 6bb81707b09..7255246f339 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -360,6 +360,7 @@ extern const char *GetConfigOption(const char *name, bool missing_ok,
bool restrict_privileged);
extern const char *GetConfigOptionResetString(const char *name);
extern int GetConfigOptionFlags(const char *name, bool missing_ok);
+extern char *GetConfigSourceFile(const char *name, int *linenum);
extern void ProcessConfigFile(GucContext context);
extern void InitializeGUCOptions(void);
extern bool SelectConfigFiles(const char *userDoption, const char *progname);
--
2.17.1
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-02-14 18:12 Maciek Sakrejda <m.sakrejda@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 2 replies; 42+ messages in thread
From: Maciek Sakrejda @ 2022-02-14 18:12 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Feb 9, 2022 at 5:58 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> FYI, it has said "while..." and hasn't said "guc" since the 2nd revision of the
> patch.
The v3-0001 attached above had "while... for GUC..."--sorry I wasn't clear.
In v4, the message looks fine to me for shared_preload_libraries
(except there is a doubled "is"). However, I also get the message for
a simple SET with local_preload_libraries:
postgres=# set local_preload_libraries=xyz;
WARNING: could not access file "xyz"
HINT: The server will fail to start with the existing configuration.
If it is is shut down, it will be necessary to manually edit the
postgresql.auto.conf file to allow it to start.
SET
I'm not familiar with that setting (reading the docs, it's like a
non-superuser session_preload_libraries for compatible modules?), but
given nothing is being persisted here with ALTER SYSTEM, this seems
incorrect.
Changing session_preload_libraries emits a similar warning:
postgres=# set session_preload_libraries = foo;
WARNING: could not access file "$libdir/plugins/foo"
HINT: New sessions will fail with the existing configuration.
SET
This is also not persisted, so I think this is also incorrect, right?
(I'm not sure what setting session_preload_libraries without an ALTER
ROLE or ALTER DATABASE accomplishes, given a new session is required
for the change to take effect, but I thought I'd point this out.) I'm
guessing this may be due to trying to have the warning for ALTER ROLE?
postgres=# alter role bob set session_preload_libraries = foo;
WARNING: could not access file "$libdir/plugins/foo"
HINT: New sessions will fail with the existing configuration.
ALTER ROLE
This is great. Ideally, we'd qualify this with "New sessions for
user..." or "New sessions for database..." but given you get the
warning right after running the relevant command, maybe that's clear
enough.
> $ ./tmp_install/usr/local/pgsql/bin/postgres -D src/test/regress/tmp_check/data -clogging_collector=on
> 2022-02-09 19:53:48.034 CST postmaster[30979] FATAL: could not access file "a": No such file or directory
> 2022-02-09 19:53:48.034 CST postmaster[30979] CONTEXT: while loading shared libraries for setting "shared_preload_libraries"
> from /home/pryzbyj/src/postgres/src/test/regress/tmp_check/data/postgresql.auto.conf:3
> 2022-02-09 19:53:48.034 CST postmaster[30979] LOG: database system is shut down
>
> Maybe it's enough to show the GucSource rather than file:line...
This is great. I think the file:line output is helpful here.
Thanks,
Maciek
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-03-23 19:02 Tom Lane <tgl@sss.pgh.pa.us>
parent: Maciek Sakrejda <m.sakrejda@gmail.com>
1 sibling, 1 reply; 42+ messages in thread
From: Tom Lane @ 2022-03-23 19:02 UTC (permalink / raw)
To: Maciek Sakrejda <m.sakrejda@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Maciek Sakrejda <m.sakrejda@gmail.com> writes:
> In v4, the message looks fine to me for shared_preload_libraries
> (except there is a doubled "is"). However, I also get the message for
> a simple SET with local_preload_libraries:
> postgres=# set local_preload_libraries=xyz;
> WARNING: could not access file "xyz"
> HINT: The server will fail to start with the existing configuration.
> If it is is shut down, it will be necessary to manually edit the
> postgresql.auto.conf file to allow it to start.
> SET
I agree with Maciek's concerns about these HINTs being emitted
inappropriately, but I also have a stylistic gripe: they're only
halfway hints. Given that we fix things so they only print when they
should, the complaint about the server not starting is not a hint,
it's a fact, which per style guidelines means it should be errdetail.
So I think this ought to look more like
WARNING: could not access file "xyz"
DETAIL: The server will fail to start with this setting.
HINT: If the server is shut down, it will be necessary to manually edit the
postgresql.auto.conf file to allow it to start again.
I adjusted the wording a bit too --- YMMV, but I think my text is clearer.
regards, tom lane
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-06-16 03:14 Michael Paquier <michael@paquier.xyz>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 42+ messages in thread
From: Michael Paquier @ 2022-06-16 03:14 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Maciek Sakrejda <m.sakrejda@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Mar 23, 2022 at 03:02:23PM -0400, Tom Lane wrote:
> I agree with Maciek's concerns about these HINTs being emitted
> inappropriately, but I also have a stylistic gripe: they're only
> halfway hints. Given that we fix things so they only print when they
> should, the complaint about the server not starting is not a hint,
> it's a fact, which per style guidelines means it should be errdetail.
> So I think this ought to look more like
>
> WARNING: could not access file "xyz"
> DETAIL: The server will fail to start with this setting.
> HINT: If the server is shut down, it will be necessary to manually edit the
> postgresql.auto.conf file to allow it to start again.
>
> I adjusted the wording a bit too --- YMMV, but I think my text is clearer.
It seems to me that there is no objection to the proposed patch, but
an update is required. Justin?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../YqqgJvYwkWLSBXeL@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-06-16 03:50 Justin Pryzby <pryzby@telsasoft.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 42+ messages in thread
From: Justin Pryzby @ 2022-06-16 03:50 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Maciek Sakrejda <m.sakrejda@gmail.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; pgsql-hackers@lists.postgresql.org
I've started to think that we should really WARN whenever a (set of) GUC is set
in a manner that the server will fail to start - not just for shared libraries.
In particular, I think the server should also warn if it's going to fail to
start like this:
2022-06-15 22:48:34.279 CDT postmaster[20782] FATAL: WAL streaming (max_wal_senders > 0) requires wal_level "replica" or "logical"
--
Justin
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-07-22 01:54 Justin Pryzby <pryzby@telsasoft.com>
parent: Maciek Sakrejda <m.sakrejda@gmail.com>
1 sibling, 1 reply; 42+ messages in thread
From: Justin Pryzby @ 2022-07-22 01:54 UTC (permalink / raw)
To: Maciek Sakrejda <m.sakrejda@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; pgsql-hackers@lists.postgresql.org
Finally returning to this .. rebased and updated per feedback.
I'm not sure of a good place to put test cases for this..
Attachments:
[text/x-diff] v5-0001-warn-when-setting-GUC-to-a-nonextant-library.patch (9.9K, ../../20220722015412.GA7789@telsasoft.com/2-v5-0001-warn-when-setting-GUC-to-a-nonextant-library.patch)
download | inline diff:
From 13e8c8b96d1a5313fd3edde515a5278cf8c6b23e Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH v5 1/3] warn when setting GUC to a nonextant library
---
src/backend/utils/misc/guc.c | 106 +++++++++++++++++-
.../unsafe_tests/expected/rolenames.out | 19 ++++
.../modules/unsafe_tests/sql/rolenames.sql | 13 +++
.../worker_spi/expected/worker_spi.out | 2 +
.../modules/worker_spi/sql/worker_spi.sql | 2 +
src/test/regress/expected/rules.out | 8 ++
6 files changed, 147 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index af4a1c30689..4e38e73a277 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -176,6 +176,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,
@@ -4272,7 +4278,7 @@ static struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4283,7 +4289,7 @@ static struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4294,7 +4300,7 @@ static struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
@@ -12506,6 +12512,100 @@ 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;
+
+ /*
+ * issue warnings only during an interactive SET, from ALTER
+ * ROLE/DATABASE/SYSTEM.
+ */
+ if (source != PGC_S_TEST && source != PGC_S_FILE)
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages from
+ * dlopen() are not portable, so it'd be hard to report any problem
+ * other than "does not exist".
+ */
+ if (access(filename, R_OK) == 0)
+ continue;
+
+
+ if (source == PGC_S_FILE)
+ /* ALTER SYSTEM */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("The server will currently fail to start with this setting."),
+ errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.",
+ "postgresql.auto.conf"));
+ else
+ /* ALTER ROLE/DATABASE */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("New sessions will currently fail to connect with the new setting."));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+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 88b1ff843be..ac161ce7c33 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1082,6 +1082,25 @@ RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+CREATE DATABASE regress_nosuch_db;
+ALTER DATABASE regress_nosuch_db SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER DATABASE regress_nosuch_db SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+DROP DATABASE regress_nosuch_db;
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
-- clean up
\c
DROP SCHEMA test_roles_schema;
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index adac36536db..cf605b08d30 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -494,6 +494,19 @@ RESET SESSION AUTHORIZATION;
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+CREATE DATABASE regress_nosuch_db;
+ALTER DATABASE regress_nosuch_db SET session_preload_libraries="DoesNotExist";
+ALTER DATABASE regress_nosuch_db SET local_preload_libraries="DoesNotExist";
+DROP DATABASE regress_nosuch_db;
+
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
+
-- clean up
\c
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
index dc0a79bf759..5b275669bcf 100644
--- a/src/test/modules/worker_spi/expected/worker_spi.out
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -28,6 +28,8 @@ SELECT pg_reload_conf();
t
(1 row)
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
index 4683523b29d..4ed5370d456 100644
--- a/src/test/modules/worker_spi/sql/worker_spi.sql
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -18,6 +18,8 @@ END
$$;
INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
SELECT pg_reload_conf();
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 7ec3d2688f0..ea4f9eb39e2 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3392,6 +3392,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
SET datestyle to iso, mdy
SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
+WARNING: could not access file "Mixed/Case"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "c:/'a"/path"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file ""
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+DETAIL: New sessions will currently fail to connect with the new setting.
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
pg_get_functiondef
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
--
2.17.1
[text/x-diff] v5-0002-errcontext-if-server-fails-to-start-due-to-librar.patch (4.5K, ../../20220722015412.GA7789@telsasoft.com/3-v5-0002-errcontext-if-server-fails-to-start-due-to-librar.patch)
download | inline diff:
From c08b511a3f76dc423b71a2c2584696b2d41afc63 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 18 Dec 2021 22:51:01 -0600
Subject: [PATCH v5 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 7f9ea972804..081d75fd724 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -75,7 +75,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 bool file_exists(const char *name);
@@ -113,7 +113,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)
@@ -142,6 +142,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;
@@ -153,7 +161,7 @@ load_file(const char *filename, bool restricted)
fullname = expand_dynamic_library_name(filename);
/* Load the shared library */
- (void) internal_load_library(fullname);
+ (void) internal_load_library(fullname, gucname);
pfree(fullname);
}
@@ -172,6 +180,7 @@ lookup_external_function(void *filehandle, const char *funcname)
/*
* Load the specified dynamic-link library file, unless it already is
* loaded. Return the pg_dl* handle for the file.
+ * gucname may be passed for error reports.
*
* Note: libname is expected to be an exact name for the library file.
*
@@ -181,7 +190,7 @@ lookup_external_function(void *filehandle, const char *funcname)
* perhaps other things that are definitely unsafe currently.
*/
static void *
-internal_load_library(const char *libname)
+internal_load_library(const char *libname, const char *gucname)
{
DynamicFileList *file_scanner;
PGModuleMagicFunction magic_func;
@@ -206,6 +215,7 @@ internal_load_library(const char *libname)
if (stat(libname, &stat_buf) == -1)
ereport(ERROR,
(errcode_for_file_access(),
+ gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
errmsg("could not access file \"%s\": %m",
libname)));
@@ -694,7 +704,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 eb43b2c5e57..4237677c222 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1664,7 +1664,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 380a82b9de3..3af652cdf99 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -737,6 +737,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
[text/x-diff] v5-0003-show-the-GUC-source-in-errcontext.patch (3.3K, ../../20220722015412.GA7789@telsasoft.com/4-v5-0003-show-the-GUC-source-in-errcontext.patch)
download | inline diff:
From 62f43d2731e4aa782ebd6503266f596103538a48 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 2 Feb 2022 20:54:49 -0600
Subject: [PATCH v5 3/3] show the GUC source in errcontext
---
src/backend/utils/fmgr/dfmgr.c | 13 ++++++++++---
src/backend/utils/misc/guc.c | 24 ++++++++++++++++++++++++
src/include/utils/guc.h | 1 +
3 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 081d75fd724..9ccb12637a0 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -34,6 +34,7 @@
#include "lib/stringinfo.h"
#include "miscadmin.h"
#include "storage/shmem.h"
+#include "utils/guc_tables.h"
#include "utils/hsearch.h"
@@ -213,11 +214,17 @@ internal_load_library(const char *libname, const char *gucname)
* Check for same files - different paths (ie, symlink or link)
*/
if (stat(libname, &stat_buf) == -1)
+ {
+ char *errstr = strerror(errno);
+ int linenum;
+ char *sourcefile = gucname ? GetConfigSourceFile(gucname, &linenum) : NULL;
ereport(ERROR,
(errcode_for_file_access(),
gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
- errmsg("could not access file \"%s\": %m",
- libname)));
+ sourcefile ? errcontext("from %s:%d", sourcefile, linenum) : 0,
+ errmsg("could not access file \"%s\": %s",
+ libname, errstr)));
+ }
for (file_scanner = file_list;
file_scanner != NULL &&
@@ -281,7 +288,7 @@ internal_load_library(const char *libname, const char *gucname)
}
else
{
- /* try to close library */
+ /* try to close library */ // Not needed due to ERROR ? //
dlclose(file_scanner->handle);
free((char *) file_scanner);
/* complain */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 4e38e73a277..4e6fae2bd8b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8577,6 +8577,30 @@ GetConfigOptionFlags(const char *name, bool missing_ok)
return record->flags;
}
+/*
+ * Get the source file and line associated with the given option, if it was set
+ * by a file.
+ *
+ * If the option doesn't exist, throw an ereport and don't return.
+ */
+char *
+GetConfigSourceFile(const char *name, int *linenum)
+{
+ struct config_generic *record;
+
+ record = find_option(name, false, false, ERROR);
+
+ /* Should not happen */
+ if (record == NULL)
+ return NULL;
+
+ if (record->source != PGC_S_FILE)
+ return NULL;
+
+ *linenum = record->sourceline;
+ return record->sourcefile;
+}
+
/*
* flatten_set_variable_args
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e734493a484..cdb5e3488c5 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -367,6 +367,7 @@ extern const char *GetConfigOption(const char *name, bool missing_ok,
bool restrict_privileged);
extern const char *GetConfigOptionResetString(const char *name);
extern int GetConfigOptionFlags(const char *name, bool missing_ok);
+extern char *GetConfigSourceFile(const char *name, int *linenum);
extern void ProcessConfigFile(GucContext context);
extern char *convert_GUC_name_for_parameter_acl(const char *name);
extern bool check_GUC_name_for_parameter_acl(const char *name);
--
2.17.1
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-07-22 17:42 Maciek Sakrejda <m.sakrejda@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 42+ messages in thread
From: Maciek Sakrejda @ 2022-07-22 17:42 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Thanks for picking this back up, Justin.
>I've started to think that we should really WARN whenever a (set of) GUC is set
>in a manner that the server will fail to start - not just for shared libraries.
+0.5. I think it's a reasonable change, but I've never broken my
server with anything other than shared_preload_libraries, so I'd
rather see an improvement here first rather than expanding scope. I
think shared_preload_libraries (and friends) is especially tricky due
to the syntax, and more likely to lead to problems.
On the update patch itself, I have some minor feedback about message wording
postgres=# set local_preload_libraries=xyz;
SET
Great, it's nice that this no longer gives a warning.
postgres=# alter role bob set local_preload_libraries = xyz;
WARNING: could not access file "xyz"
DETAIL: New sessions will currently fail to connect with the new setting.
ALTER ROLE
The warning makes sense, but the detail feels a little awkward. I
think "currently" is sort of redundant with "new setting". And it
could be clearer that the setting did in fact take effect (I know the
ALTER ROLE command tag echo tells you that, but we could reinforce
that in the warning).
Also, I know I said last time that the scope of the warning is clear
from the setting, but looking at it again, I think we could do better.
I guess because when we're generating the error, we don't know whether
the source was ALTER DATABASE or ALTER ROLE, we can't give a more
specific message? Ideally, I think the DETAIL would be something like
"New sessions for this role will fail to connect due to this setting".
Maybe even with a HINT of "Run ALTER ROLE again with a valid value to
fix this"? If that's not feasible, maybe "New sessions for this role
or database will fail to connect due to this setting"? That message is
not as clear about the impact of the change as it could be, but
hopefully you know what command you just ran, so that should make it
unambiguous. I do think without qualifying that, it suggests that all
new sessions are affected.
Hmm, or maybe just "New sessions affected by this setting will fail to
connect"? That also makes the scope clear without the warning having
to be aware of the scope: if you just ran ALTER DATABASE it's pretty
clear that what is affected by the setting is the database. I think
this is probably the way to go, but leaving my thought process above
for context.
postgres=# alter system set shared_preload_libraries = lol;
WARNING: could not access file "$libdir/plugins/lol"
DETAIL: The server will currently fail to start with this setting.
HINT: If the server is shut down, it will be necessary to manually
edit the postgresql.auto.conf file to allow it to start again.
ALTER SYSTEM
I think this works. Tom's copy edit above omitted "currently" from the
DETAIL and did not include the "$libdir/plugins/" prefix. I don't feel
strongly about these either way.
2022-07-22 10:37:50.217 PDT [1131187] LOG: database system is shut down
2022-07-22 10:37:50.306 PDT [1134058] WARNING: could not access file
"$libdir/plugins/lol"
2022-07-22 10:37:50.306 PDT [1134058] DETAIL: The server will
currently fail to start with this setting.
2022-07-22 10:37:50.306 PDT [1134058] HINT: If the server is shut
down, it will be necessary to manually edit the postgresql.auto.conf
file to allow it to start again.
2022-07-22 10:37:50.312 PDT [1134058] FATAL: could not access file
"lol": No such file or directory
2022-07-22 10:37:50.312 PDT [1134058] CONTEXT: while loading shared
libraries for setting "shared_preload_libraries"
from /home/maciek/code/aux/postgres/tmpdb/postgresql.auto.conf:3
2022-07-22 10:37:50.312 PDT [1134058] LOG: database system is shut down
Hmm, I guess this is a side effect of where these messages are
emitted, but at this point, lines 4-6 here are a little confusing, no?
The server was already shut down, and we're trying to start it back
up. If there's no reasonable way to avoid that output, I think it's
okay, but it'd be better to skip it (or adjust it to the new context).
Thanks,
Maciek
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-07-22 17:53 Tom Lane <tgl@sss.pgh.pa.us>
parent: Maciek Sakrejda <m.sakrejda@gmail.com>
0 siblings, 1 reply; 42+ messages in thread
From: Tom Lane @ 2022-07-22 17:53 UTC (permalink / raw)
To: Maciek Sakrejda <m.sakrejda@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Maciek Sakrejda <m.sakrejda@gmail.com> writes:
>> I've started to think that we should really WARN whenever a (set of) GUC is set
>> in a manner that the server will fail to start - not just for shared libraries.
> +0.5. I think it's a reasonable change, but I've never broken my
> server with anything other than shared_preload_libraries, so I'd
> rather see an improvement here first rather than expanding scope.
Generally speaking, anything that tries to check a combination of
GUC settings is going to be so fragile as to be worthless. We've
learned that lesson the hard way in the past.
> 2022-07-22 10:37:50.217 PDT [1131187] LOG: database system is shut down
> 2022-07-22 10:37:50.306 PDT [1134058] WARNING: could not access file
> "$libdir/plugins/lol"
> 2022-07-22 10:37:50.306 PDT [1134058] DETAIL: The server will
> currently fail to start with this setting.
> 2022-07-22 10:37:50.306 PDT [1134058] HINT: If the server is shut
> down, it will be necessary to manually edit the postgresql.auto.conf
> file to allow it to start again.
> 2022-07-22 10:37:50.312 PDT [1134058] FATAL: could not access file
> "lol": No such file or directory
> 2022-07-22 10:37:50.312 PDT [1134058] CONTEXT: while loading shared
> libraries for setting "shared_preload_libraries"
> from /home/maciek/code/aux/postgres/tmpdb/postgresql.auto.conf:3
> 2022-07-22 10:37:50.312 PDT [1134058] LOG: database system is shut down
> Hmm, I guess this is a side effect of where these messages are
> emitted, but at this point, lines 4-6 here are a little confusing, no?
This indicates that the warning is being issued in the wrong place.
It's okay if it comes out during ALTER SYSTEM. It's not okay if it
comes out during server start; then it's just an annoyance.
regards, tom lane
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-07-22 18:35 Justin Pryzby <pryzby@telsasoft.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 42+ messages in thread
From: Justin Pryzby @ 2022-07-22 18:35 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Maciek Sakrejda <m.sakrejda@gmail.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; pgsql-hackers@lists.postgresql.org
On Fri, Jul 22, 2022 at 01:53:21PM -0400, Tom Lane wrote:
> > 2022-07-22 10:37:50.217 PDT [1131187] LOG: database system is shut down
> > 2022-07-22 10:37:50.306 PDT [1134058] WARNING: could not access file "$libdir/plugins/lol"
> > 2022-07-22 10:37:50.306 PDT [1134058] DETAIL: The server will currently fail to start with this setting.
> > 2022-07-22 10:37:50.306 PDT [1134058] HINT: If the server is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start again.
> > 2022-07-22 10:37:50.312 PDT [1134058] FATAL: could not access file "lol": No such file or directory
> > 2022-07-22 10:37:50.312 PDT [1134058] CONTEXT: while loading shared libraries for setting "shared_preload_libraries" from /home/maciek/code/aux/postgres/tmpdb/postgresql.auto.conf:3
> > 2022-07-22 10:37:50.312 PDT [1134058] LOG: database system is shut down
>
> > Hmm, I guess this is a side effect of where these messages are
> > emitted, but at this point, lines 4-6 here are a little confusing, no?
>
> This indicates that the warning is being issued in the wrong place.
> It's okay if it comes out during ALTER SYSTEM. It's not okay if it
> comes out during server start; then it's just an annoyance.
This was a regression from the previous patch version, and I even noticed the
problem, but then forgot when returning to the patch :(
The previous patch version checked if (!IsUnderPostmaster()) before warning.
Is there a better way ?
ALTER SYSTEM uses PGC_S_FILE, the same as during startup..
--
Justin
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-07-22 19:00 Tom Lane <tgl@sss.pgh.pa.us>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 42+ messages in thread
From: Tom Lane @ 2022-07-22 19:00 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Maciek Sakrejda <m.sakrejda@gmail.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; pgsql-hackers@lists.postgresql.org
Justin Pryzby <pryzby@telsasoft.com> writes:
> On Fri, Jul 22, 2022 at 01:53:21PM -0400, Tom Lane wrote:
>> This indicates that the warning is being issued in the wrong place.
>> It's okay if it comes out during ALTER SYSTEM. It's not okay if it
>> comes out during server start; then it's just an annoyance.
> The previous patch version checked if (!IsUnderPostmaster()) before warning.
> Is there a better way ?
> ALTER SYSTEM uses PGC_S_FILE, the same as during startup..
Shouldn't you be doing this when the source is PGC_S_TEST, instead?
That's pretty much what it's for. See check_default_table_access_method
and other examples.
regards, tom lane
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-07-22 19:14 Justin Pryzby <pryzby@telsasoft.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 42+ messages in thread
From: Justin Pryzby @ 2022-07-22 19:14 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Maciek Sakrejda <m.sakrejda@gmail.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; pgsql-hackers@lists.postgresql.org
On Fri, Jul 22, 2022 at 03:00:23PM -0400, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > On Fri, Jul 22, 2022 at 01:53:21PM -0400, Tom Lane wrote:
> >> This indicates that the warning is being issued in the wrong place.
> >> It's okay if it comes out during ALTER SYSTEM. It's not okay if it
> >> comes out during server start; then it's just an annoyance.
>
> > The previous patch version checked if (!IsUnderPostmaster()) before warning.
> > Is there a better way ?
>
> > ALTER SYSTEM uses PGC_S_FILE, the same as during startup..
>
> Shouldn't you be doing this when the source is PGC_S_TEST, instead?
> That's pretty much what it's for. See check_default_table_access_method
> and other examples.
That makes sense, but it doesn't work for ALTER SYSTEM, which uses PGC_S_FILE.
postgres=# ALTER SYSTEM SET shared_preload_libraries =a;
2022-07-22 14:07:25.489 CDT client backend[23623] psql WARNING: source 3
WARNING: source 3
2022-07-22 14:07:25.489 CDT client backend[23623] psql WARNING: could not access file "$libdir/plugins/a"
2022-07-22 14:07:25.489 CDT client backend[23623] psql DETAIL: The server will currently fail to start with this setting.
2022-07-22 14:07:25.489 CDT client backend[23623] psql HINT: If the server is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start again.
postgres=# ALTER SYSTEM SET default_table_access_method=abc;
Breakpoint 1, check_default_table_access_method (newval=0x7ffe4c6fe820, extra=0x7ffe4c6fe828, source=PGC_S_FILE) at tableamapi.c:112
--
Justin
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-07-22 19:26 Tom Lane <tgl@sss.pgh.pa.us>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 42+ messages in thread
From: Tom Lane @ 2022-07-22 19:26 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Maciek Sakrejda <m.sakrejda@gmail.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; pgsql-hackers@lists.postgresql.org
Justin Pryzby <pryzby@telsasoft.com> writes:
> On Fri, Jul 22, 2022 at 03:00:23PM -0400, Tom Lane wrote:
>> Shouldn't you be doing this when the source is PGC_S_TEST, instead?
> That makes sense, but it doesn't work for ALTER SYSTEM, which uses PGC_S_FILE.
Hmph. I wonder if we shouldn't change that, because it's a lie.
The value isn't actually coming from the config file, at least
not yet.
We might need to invent a separate PGC_S_TEST_FILE value; or maybe it'd
be better to pass the "this is a test" flag separately? But that'd
require changing the signature of all GUC check hooks, so probably
it's unduly invasive. I'm not sure whether any users of the TEST
capability need to distinguish values proposed for postgresql.auto.conf
from those proposed for pg_db_role_setting ... but I guess it's
plausible that somebody might.
regards, tom lane
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-09-02 22:24 Justin Pryzby <pryzby@telsasoft.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 42+ messages in thread
From: Justin Pryzby @ 2022-09-02 22:24 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Maciek Sakrejda <m.sakrejda@gmail.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; pgsql-hackers@lists.postgresql.org
On Fri, Jul 22, 2022 at 03:26:47PM -0400, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > On Fri, Jul 22, 2022 at 03:00:23PM -0400, Tom Lane wrote:
> >> Shouldn't you be doing this when the source is PGC_S_TEST, instead?
>
> > That makes sense, but it doesn't work for ALTER SYSTEM, which uses PGC_S_FILE.
>
> Hmph. I wonder if we shouldn't change that, because it's a lie.
I think so, and I was going to raise this question some months ago when
I first picked up the patch.
The question is, which behavior do we want ?
postgres=# ALTER SYSTEM SET default_table_access_method=abc;
2022-07-22 15:24:55.445 CDT client backend[27938] psql ERROR: invalid value for parameter "default_table_access_method": "abc"
2022-07-22 15:24:55.445 CDT client backend[27938] psql DETAIL: Table access method "abc" does not exist.
2022-07-22 15:24:55.445 CDT client backend[27938] psql STATEMENT: ALTER SYSTEM SET default_table_access_method=abc;
That behavior differs from ALTER SYSTEM SET shared_preload_libraries,
which supports first seting the GUC and then installing the library. If
that wasn't supported, I think we'd just throw an error and avoid the
possibility that the server can't start.
It caused no issue when I changed:
/* Check that it's acceptable for the indicated parameter */
if (!parse_and_validate_value(record, name, value,
- PGC_S_FILE, ERROR,
+ PGC_S_TEST, ERROR,
&newval, &newextra))
I'm not sure where to go from here.
--
Justin
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-10-12 05:34 Michael Paquier <michael@paquier.xyz>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 42+ messages in thread
From: Michael Paquier @ 2022-10-12 05:34 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Maciek Sakrejda <m.sakrejda@gmail.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; pgsql-hackers@lists.postgresql.org
On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> I'm not sure where to go from here.
Not sure either, but the thread has no activity for a bit more than 1
month, so marked as RwF for now.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../Y0ZR0a1DnD2g39Nm@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-10-29 17:40 Justin Pryzby <pryzby@telsasoft.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 1 reply; 42+ messages in thread
From: Justin Pryzby @ 2022-10-29 17:40 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Maciek Sakrejda <m.sakrejda@gmail.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; pgsql-hackers@lists.postgresql.org
On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> It caused no issue when I changed:
>
> /* Check that it's acceptable for the indicated parameter */
> if (!parse_and_validate_value(record, name, value,
> - PGC_S_FILE, ERROR,
> + PGC_S_TEST, ERROR,
> &newval, &newextra))
>
> I'm not sure where to go from here.
I'm hoping for some guidance ; this simple change may be naive, but I'm not
sure what a wider change would look like.
--
Justin
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-10-30 23:12 Maciek Sakrejda <m.sakrejda@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 42+ messages in thread
From: Maciek Sakrejda @ 2022-10-30 23:12 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; pgsql-hackers@lists.postgresql.org
On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> > It caused no issue when I changed:
> >
> > /* Check that it's acceptable for the indicated parameter */
> > if (!parse_and_validate_value(record, name, value,
> > - PGC_S_FILE, ERROR,
> > + PGC_S_TEST, ERROR,
> > &newval, &newextra))
> >
> > I'm not sure where to go from here.
>
> I'm hoping for some guidance ; this simple change may be naive, but I'm not
> sure what a wider change would look like.
I assume you mean guidance on implementation details here, and not on
design. I still think this is a useful patch and I'd be happy to
review and try out future iterations, but I don't have any useful
input here.
Also, for what it's worth, I think requiring the libraries to be in
place before running ALTER SYSTEM does not really seem that onerous. I
can't really think of use cases it precludes.
Thanks,
Maciek
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-10-31 13:31 Justin Pryzby <pryzby@telsasoft.com>
parent: Maciek Sakrejda <m.sakrejda@gmail.com>
0 siblings, 2 replies; 42+ messages in thread
From: Justin Pryzby @ 2022-10-31 13:31 UTC (permalink / raw)
To: Maciek Sakrejda <m.sakrejda@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; pgsql-hackers@lists.postgresql.org
On Sun, Oct 30, 2022 at 04:12:33PM -0700, Maciek Sakrejda wrote:
> On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> > > It caused no issue when I changed:
> > >
> > > /* Check that it's acceptable for the indicated parameter */
> > > if (!parse_and_validate_value(record, name, value,
> > > - PGC_S_FILE, ERROR,
> > > + PGC_S_TEST, ERROR,
> > > &newval, &newextra))
> > >
> > > I'm not sure where to go from here.
> >
> > I'm hoping for some guidance ; this simple change may be naive, but I'm not
> > sure what a wider change would look like.
>
> I assume you mean guidance on implementation details here, and not on
ALTER SYSTEM tests the new/proposed setting using PGC_S_FILE ("which is
a lie").
It seems better to address that lie before attempting to change the
behavior of *_preload_libraries.
PGC_S_TEST is a better fit, so my question is whether it's really that
simple ?
> Also, for what it's worth, I think requiring the libraries to be in
> place before running ALTER SYSTEM does not really seem that onerous. I
> can't really think of use cases it precludes.
Right now, it's allowed to set the GUC before installing the shlib.
That's a supported case (see the 11 month old messages toward the
beginning of this thread).
--
Justin
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-10-31 13:43 Tom Lane <tgl@sss.pgh.pa.us>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 42+ messages in thread
From: Tom Lane @ 2022-10-31 13:43 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Maciek Sakrejda <m.sakrejda@gmail.com>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; pgsql-hackers@lists.postgresql.org
Justin Pryzby <pryzby@telsasoft.com> writes:
> On Sun, Oct 30, 2022 at 04:12:33PM -0700, Maciek Sakrejda wrote:
>> Also, for what it's worth, I think requiring the libraries to be in
>> place before running ALTER SYSTEM does not really seem that onerous. I
>> can't really think of use cases it precludes.
> Right now, it's allowed to set the GUC before installing the shlib.
> That's a supported case (see the 11 month old messages toward the
> beginning of this thread).
Yeah, I am afraid that you will break assorted dump/restore and
pg_upgrade scenarios if you insist on that.
regards, tom lane
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2022-11-01 22:26 Justin Pryzby <pryzby@telsasoft.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 1 reply; 42+ messages in thread
From: Justin Pryzby @ 2022-11-01 22:26 UTC (permalink / raw)
To: Maciek Sakrejda <m.sakrejda@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; pgsql-hackers@lists.postgresql.org
On Mon, Oct 31, 2022 at 08:31:20AM -0500, Justin Pryzby wrote:
> On Sun, Oct 30, 2022 at 04:12:33PM -0700, Maciek Sakrejda wrote:
> > On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> > > > It caused no issue when I changed:
> > > >
> > > > /* Check that it's acceptable for the indicated parameter */
> > > > if (!parse_and_validate_value(record, name, value,
> > > > - PGC_S_FILE, ERROR,
> > > > + PGC_S_TEST, ERROR,
> > > > &newval, &newextra))
> > > >
> > > > I'm not sure where to go from here.
> > >
> > > I'm hoping for some guidance ; this simple change may be naive, but I'm not
> > > sure what a wider change would look like.
> >
> > I assume you mean guidance on implementation details here, and not on
>
> ALTER SYSTEM tests the new/proposed setting using PGC_S_FILE ("which is
> a lie").
>
> It seems better to address that lie before attempting to change the
> behavior of *_preload_libraries.
>
> PGC_S_TEST is a better fit, so my question is whether it's really that
> simple ?
I've added the trivial change as 0001 and re-opened the patch (which ended
up in January's CF)
If for some reason it's not really as simple as that, then 001 will
serve as a "straw-man patch" hoping to elicit discussion on that point.
--
Justin
Attachments:
[text/x-diff] v6-0001-WIP-test-GUCs-from-ALTER-SYSTEM-as-PGC_S_TEST-not.patch (1.6K, ../../20221101222634.GE16921@telsasoft.com/2-v6-0001-WIP-test-GUCs-from-ALTER-SYSTEM-as-PGC_S_TEST-not.patch)
download | inline diff:
From defdb57fe0ec373c1eea8df42f0e1831b3f9c3cc Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 22 Jul 2022 15:52:11 -0500
Subject: [PATCH v6 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not
FILE
WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE
Since the value didn't come from a file. Or maybe we should have
another PGC_S_ value for this, or a flag for 'is a test'.
---
src/backend/utils/misc/guc.c | 2 +-
src/include/utils/guc.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6f21752b844..ae8810591d6 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4435,7 +4435,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
/* Check that it's acceptable for the indicated parameter */
if (!parse_and_validate_value(record, name, value,
- PGC_S_FILE, ERROR,
+ PGC_S_TEST, ERROR,
&newval, &newextra))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index b3aaff9665b..2f98df8ed4a 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -119,6 +119,7 @@ typedef enum
PGC_S_OVERRIDE, /* special case to forcibly set default */
PGC_S_INTERACTIVE, /* dividing line for error reporting */
PGC_S_TEST, /* test per-database or per-user setting */
+ // PGC_S_TEST_FILE, /* test global cluster settings (ALTER SYSTEM) */
PGC_S_SESSION /* SET command */
} GucSource;
--
2.25.1
[text/x-diff] v6-0002-warn-when-setting-GUC-to-a-nonextant-library.patch (10.7K, ../../20221101222634.GE16921@telsasoft.com/3-v6-0002-warn-when-setting-GUC-to-a-nonextant-library.patch)
download | inline diff:
From 0e197f9e3599b555c019574aaf4b36f15fe82cec Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH v6 2/4] warn when setting GUC to a nonextant library
TODO: test that warnings aren't issued during startup (only fatals)
---
src/backend/commands/variable.c | 96 +++++++++++++++++++
src/backend/utils/misc/guc.c | 2 +-
src/backend/utils/misc/guc_tables.c | 6 +-
src/include/utils/guc_hooks.h | 7 ++
.../unsafe_tests/expected/rolenames.out | 19 ++++
.../modules/unsafe_tests/sql/rolenames.sql | 13 +++
.../worker_spi/expected/worker_spi.out | 2 +
.../modules/worker_spi/sql/worker_spi.sql | 2 +
src/test/regress/expected/rules.out | 8 ++
9 files changed, 151 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 791bac6715c..85642ab8453 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -40,6 +40,7 @@
#include "utils/timestamp.h"
#include "utils/tzparser.h"
#include "utils/varlena.h"
+#include <unistd.h>
/*
* DATESTYLE
@@ -1210,3 +1211,98 @@ check_ssl(bool *newval, void **extra, GucSource source)
#endif
return true;
}
+
+
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+ bool restricted)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ return true;
+
+ /*
+ * issue warnings only during an interactive SET, from ALTER
+ * ROLE/DATABASE/SYSTEM.
+ */
+ if (source != PGC_S_TEST)
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages from
+ * dlopen() are not portable, so it'd be hard to report any problem
+ * other than "does not exist".
+ */
+ if (access(filename, R_OK) == 0)
+ continue;
+
+
+ if (source == PGC_S_FILE)
+ /* ALTER SYSTEM */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("The server will currently fail to start with this setting."),
+ errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.",
+ "postgresql.auto.conf"));
+ else
+ /* ALTER ROLE/DATABASE */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("New sessions will currently fail to connect with the new setting."));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
+bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
+
+bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ae8810591d6..9c284906d6e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -24,7 +24,7 @@
*/
#include "postgres.h"
-#include <limits.h>
+// #include <limits.h>
#include <sys/stat.h>
#include <unistd.h>
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 05ab087934c..b7c8d8dcdfb 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4015,7 +4015,7 @@ struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4026,7 +4026,7 @@ struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4037,7 +4037,7 @@ struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index f1a9a183b49..3f51436a6f5 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -155,4 +155,11 @@ extern bool check_wal_consistency_checking(char **newval, void **extra,
extern void assign_wal_consistency_checking(const char *newval, void *extra);
extern void assign_xlog_sync_method(int new_sync_method, void *extra);
+extern bool check_local_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_session_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_shared_preload_libraries(char **newval, void **extra,
+ GucSource source);
+
#endif /* GUC_HOOKS_H */
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 88b1ff843be..ac161ce7c33 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1082,6 +1082,25 @@ RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+CREATE DATABASE regress_nosuch_db;
+ALTER DATABASE regress_nosuch_db SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER DATABASE regress_nosuch_db SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+DROP DATABASE regress_nosuch_db;
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
-- clean up
\c
DROP SCHEMA test_roles_schema;
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index adac36536db..cf605b08d30 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -494,6 +494,19 @@ RESET SESSION AUTHORIZATION;
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+CREATE DATABASE regress_nosuch_db;
+ALTER DATABASE regress_nosuch_db SET session_preload_libraries="DoesNotExist";
+ALTER DATABASE regress_nosuch_db SET local_preload_libraries="DoesNotExist";
+DROP DATABASE regress_nosuch_db;
+
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
+
-- clean up
\c
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
index dc0a79bf759..5b275669bcf 100644
--- a/src/test/modules/worker_spi/expected/worker_spi.out
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -28,6 +28,8 @@ SELECT pg_reload_conf();
t
(1 row)
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
index 4683523b29d..4ed5370d456 100644
--- a/src/test/modules/worker_spi/sql/worker_spi.sql
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -18,6 +18,8 @@ END
$$;
INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
SELECT pg_reload_conf();
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 624d0e5aae1..3a36898b8d3 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3398,6 +3398,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
SET datestyle to iso, mdy
SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
+WARNING: could not access file "Mixed/Case"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "c:/'a"/path"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file ""
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+DETAIL: New sessions will currently fail to connect with the new setting.
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
pg_get_functiondef
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
--
2.25.1
[text/x-diff] v6-0003-errcontext-if-server-fails-to-start-due-to-librar.patch (4.5K, ../../20221101222634.GE16921@telsasoft.com/4-v6-0003-errcontext-if-server-fails-to-start-due-to-librar.patch)
download | inline diff:
From ee5328948981306355516b5e7b873f2c2caf4a4a Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 18 Dec 2021 22:51:01 -0600
Subject: [PATCH v6 3/4] 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 6ae6fc1556d..1f5c0c6ca4e 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -75,7 +75,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 bool file_exists(const char *name);
@@ -113,7 +113,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)
@@ -142,6 +142,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;
@@ -153,7 +161,7 @@ load_file(const char *filename, bool restricted)
fullname = expand_dynamic_library_name(filename);
/* Load the shared library */
- (void) internal_load_library(fullname);
+ (void) internal_load_library(fullname, gucname);
pfree(fullname);
}
@@ -172,6 +180,7 @@ lookup_external_function(void *filehandle, const char *funcname)
/*
* Load the specified dynamic-link library file, unless it already is
* loaded. Return the pg_dl* handle for the file.
+ * gucname may be passed for error reports.
*
* Note: libname is expected to be an exact name for the library file.
*
@@ -181,7 +190,7 @@ lookup_external_function(void *filehandle, const char *funcname)
* perhaps other things that are definitely unsafe currently.
*/
static void *
-internal_load_library(const char *libname)
+internal_load_library(const char *libname, const char *gucname)
{
DynamicFileList *file_scanner;
PGModuleMagicFunction magic_func;
@@ -206,6 +215,7 @@ internal_load_library(const char *libname)
if (stat(libname, &stat_buf) == -1)
ereport(ERROR,
(errcode_for_file_access(),
+ gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
errmsg("could not access file \"%s\": %m",
libname)));
@@ -694,7 +704,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 dfdcd3d78eb..f5d49730c89 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1812,7 +1812,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 380a82b9de3..3af652cdf99 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -737,6 +737,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.25.1
[text/x-diff] v6-0004-show-the-GUC-source-in-errcontext.patch (3.3K, ../../20221101222634.GE16921@telsasoft.com/5-v6-0004-show-the-GUC-source-in-errcontext.patch)
download | inline diff:
From f90a59e09bccc97b93de8eec982c0ce719155a60 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 2 Feb 2022 20:54:49 -0600
Subject: [PATCH v6 4/4] show the GUC source in errcontext
---
src/backend/utils/fmgr/dfmgr.c | 13 ++++++++++---
src/backend/utils/misc/guc_funcs.c | 26 ++++++++++++++++++++++++++
src/include/utils/guc.h | 1 +
3 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 1f5c0c6ca4e..a2b8386a594 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -34,6 +34,7 @@
#include "lib/stringinfo.h"
#include "miscadmin.h"
#include "storage/shmem.h"
+#include "utils/guc_tables.h"
#include "utils/hsearch.h"
@@ -213,11 +214,17 @@ internal_load_library(const char *libname, const char *gucname)
* Check for same files - different paths (ie, symlink or link)
*/
if (stat(libname, &stat_buf) == -1)
+ {
+ char *errstr = strerror(errno);
+ int linenum;
+ char *sourcefile = gucname ? GetConfigSourceFile(gucname, &linenum) : NULL;
ereport(ERROR,
(errcode_for_file_access(),
gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
- errmsg("could not access file \"%s\": %m",
- libname)));
+ sourcefile ? errcontext("from %s:%d", sourcefile, linenum) : 0,
+ errmsg("could not access file \"%s\": %s",
+ libname, errstr)));
+ }
for (file_scanner = file_list;
file_scanner != NULL &&
@@ -281,7 +288,7 @@ internal_load_library(const char *libname, const char *gucname)
}
else
{
- /* try to close library */
+ /* try to close library */ // Not needed due to ERROR ? //
dlclose(file_scanner->handle);
free(file_scanner);
/* complain */
diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index 108b3bd1290..e689b16af25 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -177,6 +177,32 @@ ExtractSetVariableArgs(VariableSetStmt *stmt)
}
}
+
+/*
+ * Get the source file and line associated with the given option, if it was set
+ * by a file.
+ *
+ * If the option doesn't exist, throw an ereport and don't return.
+ */
+char *
+GetConfigSourceFile(const char *name, int *linenum)
+{
+ struct config_generic *record;
+
+ record = find_option(name, false, false, ERROR);
+
+ /* Should not happen */
+ if (record == NULL)
+ return NULL;
+
+ if (record->source != PGC_S_FILE)
+ return NULL;
+
+ *linenum = record->sourceline;
+ return record->sourcefile;
+}
+
+
/*
* flatten_set_variable_args
* Given a parsenode List as emitted by the grammar for SET,
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 2f98df8ed4a..585e90ed35a 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -362,6 +362,7 @@ extern const char *GetConfigOption(const char *name, bool missing_ok,
bool restrict_privileged);
extern const char *GetConfigOptionResetString(const char *name);
extern int GetConfigOptionFlags(const char *name, bool missing_ok);
+extern char *GetConfigSourceFile(const char *name, int *linenum);
extern void ProcessConfigFile(GucContext context);
extern char *convert_GUC_name_for_parameter_acl(const char *name);
extern bool check_GUC_name_for_parameter_acl(const char *name);
--
2.25.1
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2023-07-06 20:15 Justin Pryzby <pryzby@telsasoft.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 3 replies; 42+ messages in thread
From: Justin Pryzby @ 2023-07-06 20:15 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; Maciek Sakrejda <m.sakrejda@gmail.com>
On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> > > > > It caused no issue when I changed:
> > > > >
> > > > > /* Check that it's acceptable for the indicated parameter */
> > > > > if (!parse_and_validate_value(record, name, value,
> > > > > - PGC_S_FILE, ERROR,
> > > > > + PGC_S_TEST, ERROR,
> > > > > &newval, &newextra))
> > > > >
> > > > > I'm not sure where to go from here.
> > > >
> > > > I'm hoping for some guidance ; this simple change may be naive, but I'm not
> > > > sure what a wider change would look like.
I'm still hoping.
> > PGC_S_TEST is a better fit, so my question is whether it's really that
> > simple ?
>
> I've added the trivial change as 0001 and re-opened the patch (which ended
> up in January's CF)
>
> If for some reason it's not really as simple as that, then 001 will
> serve as a "straw-man patch" hoping to elicit discussion on that point.
> From defdb57fe0ec373c1eea8df42f0e1831b3f9c3cc Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj@telsasoft.com>
> Date: Fri, 22 Jul 2022 15:52:11 -0500
> Subject: [PATCH v6 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not
> FILE
>
> WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE
>
> Since the value didn't come from a file. Or maybe we should have
> another PGC_S_ value for this, or a flag for 'is a test'.
> ---
> src/backend/utils/misc/guc.c | 2 +-
> src/include/utils/guc.h | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index 6f21752b844..ae8810591d6 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -4435,7 +4435,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
>
> /* Check that it's acceptable for the indicated parameter */
> if (!parse_and_validate_value(record, name, value,
> - PGC_S_FILE, ERROR,
> + PGC_S_TEST, ERROR,
> &newval, &newextra))
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
This is rebased over my own patch to enable checks for
REGRESSION_TEST_NAME_RESTRICTIONS.
--
Justin
Attachments:
[text/x-diff] 0001-WIP-test-GUCs-from-ALTER-SYSTEM-as-PGC_S_TEST-not-FI.patch (1.6K, ../../ZKcg1w4FoYq%2F4M4V@telsasoft.com/2-0001-WIP-test-GUCs-from-ALTER-SYSTEM-as-PGC_S_TEST-not-FI.patch)
download | inline diff:
From aa4c3ae08b3379bcd222e00aa896a40f811155a5 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 22 Jul 2022 15:52:11 -0500
Subject: [PATCH 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not FILE
WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE
Since the value didn't come from a file. Or maybe we should have
another PGC_S_ value for this, or a flag for 'is a test'.
---
src/backend/utils/misc/guc.c | 2 +-
src/include/utils/guc.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 5308896c87f..dda54310a56 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4540,7 +4540,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
/* Check that it's acceptable for the indicated parameter */
if (!parse_and_validate_value(record, name, value,
- PGC_S_FILE, ERROR,
+ PGC_S_TEST, ERROR,
&newval, &newextra))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index d5253c7ed23..bd45d40c106 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -119,6 +119,7 @@ typedef enum
PGC_S_OVERRIDE, /* special case to forcibly set default */
PGC_S_INTERACTIVE, /* dividing line for error reporting */
PGC_S_TEST, /* test per-database or per-user setting */
+ // PGC_S_TEST_FILE, /* test global cluster settings (ALTER SYSTEM) */
PGC_S_SESSION /* SET command */
} GucSource;
--
2.34.1
[text/x-diff] 0002-warn-when-setting-GUC-to-a-nonextant-library.patch (10.8K, ../../ZKcg1w4FoYq%2F4M4V@telsasoft.com/3-0002-warn-when-setting-GUC-to-a-nonextant-library.patch)
download | inline diff:
From b05c738bddfca11d4d0510c497a121cc8fcb585a Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH 2/4] warn when setting GUC to a nonextant library
Note that warnings shouldn't be issued during startup (only fatals):
$ ./tmp_install/usr/local/pgsql/bin/postgres -D ./testrun/regress/regress/tmp_check/data -c shared_preload_libraries=ab
2023-07-06 14:47:03.817 CDT postmaster[2608106] FATAL: could not access file "ab": No existe el archivo o el directorio
2023-07-06 14:47:03.817 CDT postmaster[2608106] CONTEXT: while loading shared libraries for setting "shared_preload_libraries"
---
src/backend/commands/variable.c | 95 +++++++++++++++++++
src/backend/utils/misc/guc_tables.c | 6 +-
src/include/utils/guc_hooks.h | 7 ++
.../unsafe_tests/expected/rolenames.out | 19 ++++
.../modules/unsafe_tests/sql/rolenames.sql | 13 +++
.../worker_spi/expected/worker_spi.out | 2 +
.../modules/worker_spi/sql/worker_spi.sql | 2 +
src/test/regress/expected/rules.out | 8 ++
8 files changed, 149 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f0f2e076552..9272a0f0e5a 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -40,6 +40,7 @@
#include "utils/timestamp.h"
#include "utils/tzparser.h"
#include "utils/varlena.h"
+#include <unistd.h>
/*
* DATESTYLE
@@ -1210,3 +1211,97 @@ check_ssl(bool *newval, void **extra, GucSource source)
#endif
return true;
}
+
+
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+ bool restricted)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ return true;
+
+ /*
+ * issue warnings only during an interactive SET, from ALTER
+ * ROLE/DATABASE/SYSTEM.
+ */
+ if (source != PGC_S_TEST)
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages
+ * from dlopen() are not portable, so it'd be hard to report any
+ * problem other than "does not exist".
+ */
+ if (access(filename, R_OK) == 0)
+ continue;
+
+ if (source == PGC_S_FILE)
+ /* ALTER SYSTEM */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("The server will currently fail to start with this setting."),
+ errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.",
+ "postgresql.auto.conf"));
+ else
+ /* ALTER ROLE/DATABASE */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("New sessions will currently fail to connect with the new setting."));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
+bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
+
+bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 59ab630ae40..e4a80d87928 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4101,7 +4101,7 @@ struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4112,7 +4112,7 @@ struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4123,7 +4123,7 @@ struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 2ecb9fc0866..de030d750f1 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -160,4 +160,11 @@ extern void assign_xlog_sync_method(int new_sync_method, void *extra);
extern bool check_io_direct(char **newval, void **extra, GucSource source);
extern void assign_io_direct(const char *newval, void *extra);
+extern bool check_local_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_session_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_shared_preload_libraries(char **newval, void **extra,
+ GucSource source);
+
#endif /* GUC_HOOKS_H */
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 61396b2a805..2dcd7cf9ec0 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1083,6 +1083,25 @@ RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+CREATE DATABASE regression_nosuch_db;
+ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+DROP DATABASE regression_nosuch_db;
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
-- clean up
\c
DROP SCHEMA test_roles_schema;
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index adac36536db..56fb52d4e08 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -494,6 +494,19 @@ RESET SESSION AUTHORIZATION;
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+CREATE DATABASE regression_nosuch_db;
+ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist";
+ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist";
+DROP DATABASE regression_nosuch_db;
+
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
+
-- clean up
\c
diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out
index dc0a79bf759..5b275669bcf 100644
--- a/src/test/modules/worker_spi/expected/worker_spi.out
+++ b/src/test/modules/worker_spi/expected/worker_spi.out
@@ -28,6 +28,8 @@ SELECT pg_reload_conf();
t
(1 row)
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql
index 4683523b29d..4ed5370d456 100644
--- a/src/test/modules/worker_spi/sql/worker_spi.sql
+++ b/src/test/modules/worker_spi/sql/worker_spi.sql
@@ -18,6 +18,8 @@ END
$$;
INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1);
SELECT pg_reload_conf();
+-- reconnect to avoid unstable test result due to asynchronous signal
+\c
-- wait until the worker has processed the tuple we just inserted
DO $$
DECLARE
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 7fd81e6a7d0..299c9cb4d2d 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3453,6 +3453,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
SET datestyle to iso, mdy
SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
+WARNING: could not access file "Mixed/Case"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "c:/'a"/path"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file ""
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+DETAIL: New sessions will currently fail to connect with the new setting.
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
pg_get_functiondef
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
--
2.34.1
[text/x-diff] 0003-errcontext-if-server-fails-to-start-due-to-library-G.patch (4.5K, ../../ZKcg1w4FoYq%2F4M4V@telsasoft.com/4-0003-errcontext-if-server-fails-to-start-due-to-library-G.patch)
download | inline diff:
From 4a37cfde4b6e84cc309e2e2c99d24b88dcb1ddaf Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 18 Dec 2021 22:51:01 -0600
Subject: [PATCH 3/4] errcontext if server fails to start due to library GUCs
---
src/backend/utils/fmgr/dfmgr.c | 21 ++++++++++++++++-----
src/backend/utils/init/miscinit.c | 2 +-
src/include/fmgr.h | 1 +
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index b85d52c913c..9674760fc02 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -75,7 +75,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 bool file_exists(const char *name);
@@ -113,7 +113,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)
@@ -142,6 +142,15 @@ 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;
@@ -153,7 +162,7 @@ load_file(const char *filename, bool restricted)
fullname = expand_dynamic_library_name(filename);
/* Load the shared library */
- (void) internal_load_library(fullname);
+ (void) internal_load_library(fullname, gucname);
pfree(fullname);
}
@@ -172,6 +181,7 @@ lookup_external_function(void *filehandle, const char *funcname)
/*
* Load the specified dynamic-link library file, unless it already is
* loaded. Return the pg_dl* handle for the file.
+ * gucname may be passed for error reports.
*
* Note: libname is expected to be an exact name for the library file.
*
@@ -181,7 +191,7 @@ lookup_external_function(void *filehandle, const char *funcname)
* perhaps other things that are definitely unsafe currently.
*/
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 +216,7 @@ internal_load_library(const char *libname)
if (stat(libname, &stat_buf) == -1)
ereport(ERROR,
(errcode_for_file_access(),
+ gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
errmsg("could not access file \"%s\": %m",
libname)));
@@ -694,7 +705,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 a604432126c..243b8464fc1 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1829,7 +1829,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 b120f5e7fef..c4655c8a5f2 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -745,6 +745,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.34.1
[text/x-diff] 0004-show-the-GUC-source-in-errcontext.patch (3.4K, ../../ZKcg1w4FoYq%2F4M4V@telsasoft.com/5-0004-show-the-GUC-source-in-errcontext.patch)
download | inline diff:
From 0dabe3b9d43091183563ddeb8a47866c427b9c00 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 2 Feb 2022 20:54:49 -0600
Subject: [PATCH 4/4] show the GUC source in errcontext
//-os-only: freebsd
---
src/backend/utils/fmgr/dfmgr.c | 14 +++++++++++---
src/backend/utils/misc/guc_funcs.c | 26 ++++++++++++++++++++++++++
src/include/utils/guc.h | 1 +
3 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 9674760fc02..80f4b7bed63 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -34,6 +34,7 @@
#include "lib/stringinfo.h"
#include "miscadmin.h"
#include "storage/shmem.h"
+#include "utils/guc_tables.h"
#include "utils/hsearch.h"
@@ -214,11 +215,18 @@ internal_load_library(const char *libname, const char *gucname)
* Check for same files - different paths (ie, symlink or link)
*/
if (stat(libname, &stat_buf) == -1)
+ {
+ char *errstr = strerror(errno);
+ int linenum;
+ char *sourcefile = gucname ? GetConfigSourceFile(gucname, &linenum) : NULL;
+
ereport(ERROR,
(errcode_for_file_access(),
gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
- errmsg("could not access file \"%s\": %m",
- libname)));
+ sourcefile ? errcontext("from %s:%d", sourcefile, linenum) : 0,
+ errmsg("could not access file \"%s\": %s",
+ libname, errstr)));
+ }
for (file_scanner = file_list;
file_scanner != NULL &&
@@ -282,7 +290,7 @@ internal_load_library(const char *libname, const char *gucname)
}
else
{
- /* try to close library */
+ /* try to close library */ // Not needed due to ERROR ? //
dlclose(file_scanner->handle);
free(file_scanner);
/* complain */
diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index 0903ba43a96..3d4b0249886 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -177,6 +177,32 @@ ExtractSetVariableArgs(VariableSetStmt *stmt)
}
}
+
+/*
+ * Get the source file and line associated with the given option, if it was set
+ * by a file.
+ *
+ * If the option doesn't exist, throw an ereport and don't return.
+ */
+char *
+GetConfigSourceFile(const char *name, int *linenum)
+{
+ struct config_generic *record;
+
+ record = find_option(name, false, false, ERROR);
+
+ /* Should not happen */
+ if (record == NULL)
+ return NULL;
+
+ if (record->source != PGC_S_FILE)
+ return NULL;
+
+ *linenum = record->sourceline;
+ return record->sourcefile;
+}
+
+
/*
* flatten_set_variable_args
* Given a parsenode List as emitted by the grammar for SET,
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index bd45d40c106..93f419d0328 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -362,6 +362,7 @@ extern const char *GetConfigOption(const char *name, bool missing_ok,
bool restrict_privileged);
extern const char *GetConfigOptionResetString(const char *name);
extern int GetConfigOptionFlags(const char *name, bool missing_ok);
+extern char *GetConfigSourceFile(const char *name, int *linenum);
extern void ProcessConfigFile(GucContext context);
extern char *convert_GUC_name_for_parameter_acl(const char *name);
extern bool check_GUC_name_for_parameter_acl(const char *name);
--
2.34.1
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2023-12-28 05:26 Shubham Khanna <khannashubham1197@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
2 siblings, 1 reply; 42+ messages in thread
From: Shubham Khanna @ 2023-12-28 05:26 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; Maciek Sakrejda <m.sakrejda@gmail.com>
On Thu, Dec 28, 2023 at 10:54 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> > > > > > It caused no issue when I changed:
> > > > > >
> > > > > > /* Check that it's acceptable for the indicated parameter */
> > > > > > if (!parse_and_validate_value(record, name, value,
> > > > > > - PGC_S_FILE, ERROR,
> > > > > > + PGC_S_TEST, ERROR,
> > > > > > &newval, &newextra))
> > > > > >
> > > > > > I'm not sure where to go from here.
> > > > >
> > > > > I'm hoping for some guidance ; this simple change may be naive, but I'm not
> > > > > sure what a wider change would look like.
>
> I'm still hoping.
>
> > > PGC_S_TEST is a better fit, so my question is whether it's really that
> > > simple ?
> >
> > I've added the trivial change as 0001 and re-opened the patch (which ended
> > up in January's CF)
> >
> > If for some reason it's not really as simple as that, then 001 will
> > serve as a "straw-man patch" hoping to elicit discussion on that point.
>
> > From defdb57fe0ec373c1eea8df42f0e1831b3f9c3cc Mon Sep 17 00:00:00 2001
> > From: Justin Pryzby <pryzbyj@telsasoft.com>
> > Date: Fri, 22 Jul 2022 15:52:11 -0500
> > Subject: [PATCH v6 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not
> > FILE
> >
> > WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE
> >
> > Since the value didn't come from a file. Or maybe we should have
> > another PGC_S_ value for this, or a flag for 'is a test'.
> > ---
> > src/backend/utils/misc/guc.c | 2 +-
> > src/include/utils/guc.h | 1 +
> > 2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> > index 6f21752b844..ae8810591d6 100644
> > --- a/src/backend/utils/misc/guc.c
> > +++ b/src/backend/utils/misc/guc.c
> > @@ -4435,7 +4435,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
> >
> > /* Check that it's acceptable for the indicated parameter */
> > if (!parse_and_validate_value(record, name, value,
> > - PGC_S_FILE, ERROR,
> > + PGC_S_TEST, ERROR,
> > &newval, &newextra))
> > ereport(ERROR,
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>
> This is rebased over my own patch to enable checks for
> REGRESSION_TEST_NAME_RESTRICTIONS.
>
I was reviewing the Patch and came across a minor issue that the Patch
does not apply on the current Head. Please provide the updated version
of the patch.
Thanks and Regards,
Shubham Khanna.
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2024-01-01 12:16 John Naylor <johncnaylorls@gmail.com>
parent: Shubham Khanna <khannashubham1197@gmail.com>
0 siblings, 0 replies; 42+ messages in thread
From: John Naylor @ 2024-01-01 12:16 UTC (permalink / raw)
To: Shubham Khanna <khannashubham1197@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; Maciek Sakrejda <m.sakrejda@gmail.com>
On Thu, Dec 28, 2023 at 12:27 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> I was reviewing the Patch and came across a minor issue that the Patch
> does not apply on the current Head. Please provide the updated version
> of the patch.
For your information, the commitfest manager has the ability to send
private messages to authors about procedural issues like this. There
is no need to tell the whole list about it.
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2024-01-07 13:27 Justin Pryzby <pryzby@telsasoft.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
2 siblings, 0 replies; 42+ messages in thread
From: Justin Pryzby @ 2024-01-07 13:27 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Robert Haas <robertmhaas@gmail.com>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; Maciek Sakrejda <m.sakrejda@gmail.com>; Shubham Khanna <khannashubham1197@gmail.com>
On Fri, Jul 22, 2022 at 03:26:47PM -0400, Tom Lane wrote:
> Hmph. I wonder if we shouldn't change that, because it's a lie.
> The value isn't actually coming from the config file, at least
> not yet.
On Thu, Jul 06, 2023 at 03:15:20PM -0500, Justin Pryzby wrote:
> On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote:
> > > > > > It caused no issue when I changed:
> > > > > >
> > > > > > /* Check that it's acceptable for the indicated parameter */
> > > > > > if (!parse_and_validate_value(record, name, value,
> > > > > > - PGC_S_FILE, ERROR,
> > > > > > + PGC_S_TEST, ERROR,
> > > > > > &newval, &newextra))
> > > > > >
> > > > > > I'm not sure where to go from here.
> > > > >
> > > > > I'm hoping for some guidance ; this simple change may be naive, but I'm not
> > > > > sure what a wider change would look like.
>
> I'm still hoping.
@cfbot: rebased
Attachments:
[text/x-diff] v8-0001-WIP-test-GUCs-from-ALTER-SYSTEM-as-PGC_S_TEST-not.patch (1.5K, ../../ZZqmpB8a0CokcxwC@pryzbyj2023/2-v8-0001-WIP-test-GUCs-from-ALTER-SYSTEM-as-PGC_S_TEST-not.patch)
download | inline diff:
From e983189c5ae64524f724f317f024d6938f6ad271 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 22 Jul 2022 15:52:11 -0500
Subject: [PATCH v8 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not
FILE
WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE
Since the value didn't come from a file. Or maybe we should have
another PGC_S_ value for this, or a flag for 'is a test'.
---
src/backend/utils/misc/guc.c | 2 +-
src/include/utils/guc.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 09353bb613a..0339d2bc722 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4701,7 +4701,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
void *newextra = NULL;
if (!parse_and_validate_value(record, name, value,
- PGC_S_FILE, ERROR,
+ PGC_S_TEST, ERROR,
&newval, &newextra))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 109e7c5abbd..d0ac2fd4944 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -120,6 +120,7 @@ typedef enum
PGC_S_INTERACTIVE, /* dividing line for error reporting */
PGC_S_TEST, /* test per-database or per-user setting */
PGC_S_SESSION, /* SET command */
+ // PGC_S_TEST_FILE, /* test global cluster settings (ALTER SYSTEM) */
} GucSource;
/*
--
2.42.0
[text/x-diff] v8-0002-warn-when-setting-GUC-to-a-nonextant-library.patch (9.7K, ../../ZZqmpB8a0CokcxwC@pryzbyj2023/3-v8-0002-warn-when-setting-GUC-to-a-nonextant-library.patch)
download | inline diff:
From 790fc2db29d28c7d00dceaa3f1ca22919ba09213 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH v8 2/4] warn when setting GUC to a nonextant library
Note that warnings shouldn't be issued during startup (only fatals):
$ ./tmp_install/usr/local/pgsql/bin/postgres -D ./testrun/regress/regress/tmp_check/data -c shared_preload_libraries=ab
2023-07-06 14:47:03.817 CDT postmaster[2608106] FATAL: could not access file "ab": No existe el archivo o el directorio
2023-07-06 14:47:03.817 CDT postmaster[2608106] CONTEXT: while loading shared libraries for setting "shared_preload_libraries"
---
src/backend/commands/variable.c | 95 +++++++++++++++++++
src/backend/utils/misc/guc_tables.c | 6 +-
src/include/utils/guc_hooks.h | 7 ++
.../unsafe_tests/expected/rolenames.out | 19 ++++
.../modules/unsafe_tests/sql/rolenames.sql | 13 +++
src/test/regress/expected/rules.out | 8 ++
6 files changed, 145 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 30efcd554ae..7109091440c 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -40,6 +40,7 @@
#include "utils/timestamp.h"
#include "utils/tzparser.h"
#include "utils/varlena.h"
+#include <unistd.h>
/*
* DATESTYLE
@@ -1234,3 +1235,97 @@ check_ssl(bool *newval, void **extra, GucSource source)
#endif
return true;
}
+
+
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+ bool restricted)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ return true;
+
+ /*
+ * issue warnings only during an interactive SET, from ALTER
+ * ROLE/DATABASE/SYSTEM.
+ */
+ if (source != PGC_S_TEST)
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages
+ * from dlopen() are not portable, so it'd be hard to report any
+ * problem other than "does not exist".
+ */
+ if (access(filename, R_OK) == 0)
+ continue;
+
+ if (source == PGC_S_FILE)
+ /* ALTER SYSTEM */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("The server will currently fail to start with this setting."),
+ errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.",
+ "postgresql.auto.conf"));
+ else
+ /* ALTER ROLE/DATABASE */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("New sessions will currently fail to connect with the new setting."));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
+bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
+
+bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index f89f148ff3c..2509167b2d4 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4161,7 +4161,7 @@ struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4172,7 +4172,7 @@ struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4183,7 +4183,7 @@ struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 5300c44f3b0..870006a83fe 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -163,4 +163,11 @@ extern void assign_wal_consistency_checking(const char *newval, void *extra);
extern bool check_wal_segment_size(int *newval, void **extra, GucSource source);
extern void assign_wal_sync_method(int new_wal_sync_method, void *extra);
+extern bool check_local_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_session_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_shared_preload_libraries(char **newval, void **extra,
+ GucSource source);
+
#endif /* GUC_HOOKS_H */
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 291dd9f8d53..28c2d382595 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1099,6 +1099,25 @@ ERROR: permission denied to examine "session_preload_libraries"
DETAIL: Only roles with privileges of the "pg_read_all_settings" role may examine this parameter.
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+CREATE DATABASE regression_nosuch_db;
+ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+DROP DATABASE regression_nosuch_db;
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
-- clean up
\c
DROP SCHEMA test_roles_schema;
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index cf72aaf13a3..d957fd3dd0e 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -503,6 +503,19 @@ ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+CREATE DATABASE regression_nosuch_db;
+ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist";
+ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist";
+DROP DATABASE regression_nosuch_db;
+
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
+
-- clean up
\c
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index d878a971df9..7ef2fdf768b 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3486,6 +3486,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
SET datestyle to iso, mdy
SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
+WARNING: could not access file "Mixed/Case"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "c:/'a"/path"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file ""
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+DETAIL: New sessions will currently fail to connect with the new setting.
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
pg_get_functiondef
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
--
2.42.0
[text/x-diff] v8-0003-errcontext-if-server-fails-to-start-due-to-librar.patch (4.5K, ../../ZZqmpB8a0CokcxwC@pryzbyj2023/4-v8-0003-errcontext-if-server-fails-to-start-due-to-librar.patch)
download | inline diff:
From 2bfff366b754b77815604a2916dd97f2971a5a4b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 18 Dec 2021 22:51:01 -0600
Subject: [PATCH v8 3/4] errcontext if server fails to start due to library
GUCs
---
src/backend/utils/fmgr/dfmgr.c | 21 ++++++++++++++++-----
src/backend/utils/init/miscinit.c | 2 +-
src/include/fmgr.h | 1 +
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 638eddf19f4..28082890bf0 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -75,7 +75,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 bool file_exists(const char *name);
@@ -113,7 +113,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)
@@ -142,6 +142,15 @@ 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;
@@ -153,7 +162,7 @@ load_file(const char *filename, bool restricted)
fullname = expand_dynamic_library_name(filename);
/* Load the shared library */
- (void) internal_load_library(fullname);
+ (void) internal_load_library(fullname, gucname);
pfree(fullname);
}
@@ -172,6 +181,7 @@ lookup_external_function(void *filehandle, const char *funcname)
/*
* Load the specified dynamic-link library file, unless it already is
* loaded. Return the pg_dl* handle for the file.
+ * gucname may be passed for error reports.
*
* Note: libname is expected to be an exact name for the library file.
*
@@ -181,7 +191,7 @@ lookup_external_function(void *filehandle, const char *funcname)
* perhaps other things that are definitely unsafe currently.
*/
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 +216,7 @@ internal_load_library(const char *libname)
if (stat(libname, &stat_buf) == -1)
ereport(ERROR,
(errcode_for_file_access(),
+ gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
errmsg("could not access file \"%s\": %m",
libname)));
@@ -694,7 +705,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 23f77a59e58..a2322a25769 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1821,7 +1821,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 ccb4070a251..a2f6fa60196 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -745,6 +745,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.42.0
[text/x-diff] v8-0004-show-the-GUC-source-in-errcontext.patch (3.4K, ../../ZZqmpB8a0CokcxwC@pryzbyj2023/5-v8-0004-show-the-GUC-source-in-errcontext.patch)
download | inline diff:
From 002be0e6ed8cc4d00505cec5c3d9097571d93cfa Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 2 Feb 2022 20:54:49 -0600
Subject: [PATCH v8 4/4] show the GUC source in errcontext
//-os-only: freebsd
---
src/backend/utils/fmgr/dfmgr.c | 14 +++++++++++---
src/backend/utils/misc/guc_funcs.c | 26 ++++++++++++++++++++++++++
src/include/utils/guc.h | 1 +
3 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 28082890bf0..acbcaef7957 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -34,6 +34,7 @@
#include "lib/stringinfo.h"
#include "miscadmin.h"
#include "storage/shmem.h"
+#include "utils/guc_tables.h"
#include "utils/hsearch.h"
@@ -214,11 +215,18 @@ internal_load_library(const char *libname, const char *gucname)
* Check for same files - different paths (ie, symlink or link)
*/
if (stat(libname, &stat_buf) == -1)
+ {
+ char *errstr = strerror(errno);
+ int linenum;
+ char *sourcefile = gucname ? GetConfigSourceFile(gucname, &linenum) : NULL;
+
ereport(ERROR,
(errcode_for_file_access(),
gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
- errmsg("could not access file \"%s\": %m",
- libname)));
+ sourcefile ? errcontext("from %s:%d", sourcefile, linenum) : 0,
+ errmsg("could not access file \"%s\": %s",
+ libname, errstr)));
+ }
for (file_scanner = file_list;
file_scanner != NULL &&
@@ -282,7 +290,7 @@ internal_load_library(const char *libname, const char *gucname)
}
else
{
- /* try to close library */
+ /* try to close library */ // Not needed due to ERROR ? //
dlclose(file_scanner->handle);
free(file_scanner);
/* complain */
diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index 0ad7e6cf1f1..4229eb3a794 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -177,6 +177,32 @@ ExtractSetVariableArgs(VariableSetStmt *stmt)
}
}
+
+/*
+ * Get the source file and line associated with the given option, if it was set
+ * by a file.
+ *
+ * If the option doesn't exist, throw an ereport and don't return.
+ */
+char *
+GetConfigSourceFile(const char *name, int *linenum)
+{
+ struct config_generic *record;
+
+ record = find_option(name, false, false, ERROR);
+
+ /* Should not happen */
+ if (record == NULL)
+ return NULL;
+
+ if (record->source != PGC_S_FILE)
+ return NULL;
+
+ *linenum = record->sourceline;
+ return record->sourcefile;
+}
+
+
/*
* flatten_set_variable_args
* Given a parsenode List as emitted by the grammar for SET,
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index d0ac2fd4944..2bab6baadf0 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -367,6 +367,7 @@ extern const char *GetConfigOption(const char *name, bool missing_ok,
bool restrict_privileged);
extern const char *GetConfigOptionResetString(const char *name);
extern int GetConfigOptionFlags(const char *name, bool missing_ok);
+extern char *GetConfigSourceFile(const char *name, int *linenum);
extern void ProcessConfigFile(GucContext context);
extern char *convert_GUC_name_for_parameter_acl(const char *name);
extern void check_GUC_name_for_parameter_acl(const char *name);
--
2.42.0
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2024-05-24 13:26 Robert Haas <robertmhaas@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
2 siblings, 1 reply; 42+ messages in thread
From: Robert Haas @ 2024-05-24 13:26 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; Maciek Sakrejda <m.sakrejda@gmail.com>
On Thu, Jul 6, 2023 at 4:15 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> I'm still hoping.
Hi,
I got asked to take a look at this thread.
First, I want to explain why I think this thread hasn't gotten as much
feedback as Justin was hoping. It is always possible for any thread to
have that problem just because people are busy or not interested.
However, I think in this case an aggravating factor is that the
discussion is very "high context"; it's hard to understand what the
open problems are without reading a lot of emails and understanding
how they all relate to each other. One of the key questions is whether
we should replace PGC_S_FILE with PGC_S_TEST in
AlterSystemSetConfigFile. I originally thought, based on reading one
of the emails, that the question was whether we should do that out of
some sense of intellectual purity, and my answer was "probably not,
because that would change the behavior in a way that doesn't seem
good." But then I realized, reading another email, that Justin already
knew that the behavior would change, or at least I'm 90% certain that
he knows that. So now I think the question is whether we want that
behavior change, but he only provided one example of how the behavior
changes, and it's not clear how many other scenarios are affected or
in what way, so it's still a bit hard to answer. Plus, it took me 10
minutes to figure out what the question was. I think that if the
question had been phrased in a way that was easily understandable to
any experienced PostgreSQL user, it's a lot more likely that one or
more people would have had an opinion on whether it was good or bad.
As it is, I think most people probably didn't understand the question,
and the people who did understand the question may not have wanted to
spend the time to do the research that they would have needed to do to
come up with an intelligent answer. I'm not saying any of this to
criticize Justin or to say that he did anything wrong, but I think we
have lots of examples of stuff like this on the mailing list, where
people are sad because they didn't get an answer, but don't always
realize that there might be things they could do to improve their
chances.
On the behavior change itself, it seems to me that there's a big
difference between shared_preload_libraries=bogus and work_mem=bogus.
The former is valid or invalid according to whether bogus.so exists in
an appropriate directory on the local machine, but the latter is
categorically invalid. I'm not sure to what degree we have the
infrastructure to distinguish those cases, but to the extent that we
do, handling them differently is completely defensible. It's
reasonable to allow the first one on the theory that the
presently-invalid configuration may at a later time become valid, but
that's not reasonable in the second case. So if changing PGC_S_FILE to
PGC_S_TEST in AlterSystemSetConfigFile is going to have the effect of
allowing garbage values into postgresql.auto.conf that would currently
get blocked, I think that's a bad plan and we shouldn't do it. But
it's quite possible I'm not fully understanding the situation.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2024-05-24 15:48 Justin Pryzby <pryzby@telsasoft.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 42+ messages in thread
From: Justin Pryzby @ 2024-05-24 15:48 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; Maciek Sakrejda <m.sakrejda@gmail.com>
On Fri, May 24, 2024 at 09:26:54AM -0400, Robert Haas wrote:
> On Thu, Jul 6, 2023 at 4:15 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> But then I realized, reading another email, that Justin already knew
> that the behavior would change, or at least I'm 90% certain that he
> knows that.
You give me too much credit..
> On the behavior change itself, it seems to me that there's a big
> difference between shared_preload_libraries=bogus and work_mem=bogus.
..
> So if changing PGC_S_FILE to
> PGC_S_TEST in AlterSystemSetConfigFile is going to have the effect of
> allowing garbage values into postgresql.auto.conf that would currently
> get blocked, I think that's a bad plan and we shouldn't do it.
Right - this is something I'd failed to realize. We can't change it in
the naive way because it allows bogus values, and not just missing
libraries. Specifically, for GUCs with assign hooks conditional on
PGC_TEST.
We don't want to change the behavior to allow this to succeed -- it
would allow leaving the server in a state that it fails to start (rather
than helping to avoid doing so, as intended by this thread).
regression=# ALTER SYSTEM SET default_table_access_method=abc;
NOTICE: table access method "abc" does not exist
ALTER SYSTEM
Maybe there should be a comment explaning why PGC_FILE is used, and
maybe there should be a TAP test for the behavior, but they're pretty
unrelated to this thread. So, I've dropped the 001 patch.
--
Justin
Attachments:
[text/x-diff] 0001-warn-when-setting-GUC-to-a-nonextant-library.patch (9.6K, ../../ZlC20rkDhfwt4cYe@pryzbyj2023/2-0001-warn-when-setting-GUC-to-a-nonextant-library.patch)
download | inline diff:
From d29d0cfcf9d15dad7db1d0dd334e3e77cdad653f 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 when setting GUC to a nonextant library
Note that warnings shouldn't be issued during startup (only fatals):
$ ./tmp_install/usr/local/pgsql/bin/postgres -D ./testrun/regress/regress/tmp_check/data -c shared_preload_libraries=ab
2023-07-06 14:47:03.817 CDT postmaster[2608106] FATAL: could not access file "ab": No existe el archivo o el directorio
2023-07-06 14:47:03.817 CDT postmaster[2608106] CONTEXT: while loading shared libraries for setting "shared_preload_libraries"
---
src/backend/commands/variable.c | 95 +++++++++++++++++++
src/backend/utils/misc/guc_tables.c | 6 +-
src/include/utils/guc_hooks.h | 7 ++
.../unsafe_tests/expected/rolenames.out | 19 ++++
.../modules/unsafe_tests/sql/rolenames.sql | 13 +++
src/test/regress/expected/rules.out | 8 ++
6 files changed, 145 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 9345131711e..bab51c4572a 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -40,6 +40,7 @@
#include "utils/timestamp.h"
#include "utils/tzparser.h"
#include "utils/varlena.h"
+#include <unistd.h>
/*
* DATESTYLE
@@ -1234,3 +1235,97 @@ check_ssl(bool *newval, void **extra, GucSource source)
#endif
return true;
}
+
+
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+ bool restricted)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ return true;
+
+ /*
+ * issue warnings only during an interactive SET, from ALTER
+ * ROLE/DATABASE/SYSTEM.
+ */
+ if (source != PGC_S_TEST)
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages
+ * from dlopen() are not portable, so it'd be hard to report any
+ * problem other than "does not exist".
+ */
+ if (access(filename, R_OK) == 0)
+ continue;
+
+ if (source == PGC_S_FILE)
+ /* ALTER SYSTEM */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("The server will currently fail to start with this setting."),
+ errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.",
+ "postgresql.auto.conf"));
+ else
+ /* ALTER ROLE/DATABASE */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("New sessions will currently fail to connect with the new setting."));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
+bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
+
+bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 46c258be282..82c662089b6 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4235,7 +4235,7 @@ struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4246,7 +4246,7 @@ struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4257,7 +4257,7 @@ struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index d64dc5fcdb0..78e529ea365 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -178,4 +178,11 @@ extern bool check_standby_slot_names(char **newval, void **extra,
GucSource source);
extern void assign_standby_slot_names(const char *newval, void *extra);
+extern bool check_local_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_session_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_shared_preload_libraries(char **newval, void **extra,
+ GucSource source);
+
#endif /* GUC_HOOKS_H */
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 61396b2a805..2dcd7cf9ec0 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1083,6 +1083,25 @@ RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+CREATE DATABASE regression_nosuch_db;
+ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will currently fail to connect with the new setting.
+DROP DATABASE regression_nosuch_db;
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
-- clean up
\c
DROP SCHEMA test_roles_schema;
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index adac36536db..56fb52d4e08 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -494,6 +494,19 @@ RESET SESSION AUTHORIZATION;
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+CREATE DATABASE regression_nosuch_db;
+ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist";
+ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist";
+DROP DATABASE regression_nosuch_db;
+
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
+
-- clean up
\c
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index ef658ad7405..2c494080c54 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3489,6 +3489,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer
SET datestyle to iso, mdy
SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
+WARNING: could not access file "Mixed/Case"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "c:/'a"/path"
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file ""
+DETAIL: New sessions will currently fail to connect with the new setting.
+WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789"
+DETAIL: New sessions will currently fail to connect with the new setting.
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
pg_get_functiondef
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------
--
2.42.0
[text/x-diff] 0002-errcontext-if-server-fails-to-start-due-to-library-G.patch (4.5K, ../../ZlC20rkDhfwt4cYe@pryzbyj2023/3-0002-errcontext-if-server-fails-to-start-due-to-library-G.patch)
download | inline diff:
From 5377176aeb5987afbdc1a59e67eed9d3e066f724 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 | 21 ++++++++++++++++-----
src/backend/utils/init/miscinit.c | 2 +-
src/include/fmgr.h | 1 +
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 092004dcf3b..703bd8de6c6 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 char *expand_dynamic_library_name(const char *name);
@@ -113,7 +113,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)
@@ -142,6 +142,15 @@ 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;
@@ -153,7 +162,7 @@ load_file(const char *filename, bool restricted)
fullname = expand_dynamic_library_name(filename);
/* Load the shared library */
- (void) internal_load_library(fullname);
+ (void) internal_load_library(fullname, gucname);
pfree(fullname);
}
@@ -172,6 +181,7 @@ lookup_external_function(void *filehandle, const char *funcname)
/*
* Load the specified dynamic-link library file, unless it already is
* loaded. Return the pg_dl* handle for the file.
+ * gucname may be passed for error reports.
*
* Note: libname is expected to be an exact name for the library file.
*
@@ -181,7 +191,7 @@ lookup_external_function(void *filehandle, const char *funcname)
* perhaps other things that are definitely unsafe currently.
*/
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 +216,7 @@ internal_load_library(const char *libname)
if (stat(libname, &stat_buf) == -1)
ereport(ERROR,
(errcode_for_file_access(),
+ gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
errmsg("could not access file \"%s\": %m",
libname)));
@@ -677,7 +688,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 537d92c0cfd..8f11f718135 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1825,7 +1825,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 ccb4070a251..a2f6fa60196 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -745,6 +745,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.42.0
[text/x-diff] 0003-show-the-GUC-source-in-errcontext.patch (3.4K, ../../ZlC20rkDhfwt4cYe@pryzbyj2023/4-0003-show-the-GUC-source-in-errcontext.patch)
download | inline diff:
From c339d87cd7ce9f73015d62063d9104459ab71fc4 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 2 Feb 2022 20:54:49 -0600
Subject: [PATCH 3/3] show the GUC source in errcontext
//-os-only: freebsd
---
src/backend/utils/fmgr/dfmgr.c | 14 +++++++++++---
src/backend/utils/misc/guc_funcs.c | 26 ++++++++++++++++++++++++++
src/include/utils/guc.h | 1 +
3 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 703bd8de6c6..42ee908ae50 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -35,6 +35,7 @@
#include "miscadmin.h"
#include "storage/fd.h"
#include "storage/shmem.h"
+#include "utils/guc_tables.h"
#include "utils/hsearch.h"
@@ -214,11 +215,18 @@ internal_load_library(const char *libname, const char *gucname)
* Check for same files - different paths (ie, symlink or link)
*/
if (stat(libname, &stat_buf) == -1)
+ {
+ char *errstr = strerror(errno);
+ int linenum;
+ char *sourcefile = gucname ? GetConfigSourceFile(gucname, &linenum) : NULL;
+
ereport(ERROR,
(errcode_for_file_access(),
gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
- errmsg("could not access file \"%s\": %m",
- libname)));
+ sourcefile ? errcontext("from %s:%d", sourcefile, linenum) : 0,
+ errmsg("could not access file \"%s\": %s",
+ libname, errstr)));
+ }
for (file_scanner = file_list;
file_scanner != NULL &&
@@ -282,7 +290,7 @@ internal_load_library(const char *libname, const char *gucname)
}
else
{
- /* try to close library */
+ /* try to close library */ // Not needed due to ERROR ? //
dlclose(file_scanner->handle);
free(file_scanner);
/* complain */
diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index 9c9edd3d2f5..eb7c3d726b2 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -177,6 +177,32 @@ ExtractSetVariableArgs(VariableSetStmt *stmt)
}
}
+
+/*
+ * Get the source file and line associated with the given option, if it was set
+ * by a file.
+ *
+ * If the option doesn't exist, throw an ereport and don't return.
+ */
+char *
+GetConfigSourceFile(const char *name, int *linenum)
+{
+ struct config_generic *record;
+
+ record = find_option(name, false, false, ERROR);
+
+ /* Should not happen */
+ if (record == NULL)
+ return NULL;
+
+ if (record->source != PGC_S_FILE)
+ return NULL;
+
+ *linenum = record->sourceline;
+ return record->sourcefile;
+}
+
+
/*
* flatten_set_variable_args
* Given a parsenode List as emitted by the grammar for SET,
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e4a594b5e80..1f460e4ea90 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -364,6 +364,7 @@ extern const char *GetConfigOption(const char *name, bool missing_ok,
bool restrict_privileged);
extern const char *GetConfigOptionResetString(const char *name);
extern int GetConfigOptionFlags(const char *name, bool missing_ok);
+extern char *GetConfigSourceFile(const char *name, int *linenum);
extern void ProcessConfigFile(GucContext context);
extern char *convert_GUC_name_for_parameter_acl(const char *name);
extern void check_GUC_name_for_parameter_acl(const char *name);
--
2.42.0
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2024-05-24 17:15 Robert Haas <robertmhaas@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 42+ messages in thread
From: Robert Haas @ 2024-05-24 17:15 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; Maciek Sakrejda <m.sakrejda@gmail.com>
On Fri, May 24, 2024 at 11:48 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> You give me too much credit..
Gee, usually I'm very good at avoiding that mistake. :-)
> We don't want to change the behavior to allow this to succeed -- it
> would allow leaving the server in a state that it fails to start (rather
> than helping to avoid doing so, as intended by this thread).
+1.
> Maybe there should be a comment explaning why PGC_FILE is used, and
> maybe there should be a TAP test for the behavior, but they're pretty
> unrelated to this thread. So, I've dropped the 001 patch.
+1 for that, too.
+ /* Note that filename was already canonicalized */
I see that this comment is copied from load_libraries(), but I don't
immediately see where the canonicalization actually happens. Do you
know, or can you find out? Because that's crucial here, else stat()
might not target the real filename. I wonder if it will anyway. Like,
couldn't the library be versioned, and might not dlopen() try a few
possibilities?
+ errdetail("The server will currently fail to start with this setting."),
+ errdetail("New sessions will currently fail to connect with the new
setting."));
I understand why these messages have the word "currently" in them, but
I bet the user won't. I'm not sure exactly what to recommend at the
moment (and I'm quite busy today due to the conference upcoming) but I
think we should try to find some way to rephrase these.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2024-07-22 16:28 Justin Pryzby <pryzby@telsasoft.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 42+ messages in thread
From: Justin Pryzby @ 2024-07-22 16:28 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; Maciek Sakrejda <m.sakrejda@gmail.com>
On Fri, May 24, 2024 at 01:15:13PM -0400, Robert Haas wrote:
> + /* Note that filename was already canonicalized */
>
> I see that this comment is copied from load_libraries(), but I don't
> immediately see where the canonicalization actually happens. Do you
> know, or can you find out? Because that's crucial here, else stat()
> might not target the real filename. I wonder if it will anyway. Like,
> couldn't the library be versioned, and might not dlopen() try a few
> possibilities?
This comment made me realize that we've been fixated on the warning.
But the patch was broken, and would've always warned. I think almost
all of the previous patch versions had this issue - oops.
I added a call to expand_dynamic_library_name(), which seems to answer
your question.
And added a preparatory patch to distinguish ALTER USER/DATABASE SET
from SET in a function, to avoid warning in that case.
--
Justin
Attachments:
[text/x-diff] 0001-change-function-SET-to-use-a-separate-GUC-source.patch (11.7K, ../../Zp6IpxVT1U-qmMOC@pryzbyj2023/2-0001-change-function-SET-to-use-a-separate-GUC-source.patch)
download | inline diff:
From 29610d4e417a1a0cf099e10736b5c14aec90f641 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 21 Jul 2024 09:49:17 -0500
Subject: [PATCH 1/4] change function SET to use a separate GUC source
This allows distinguishing it from ALTER ROLE SET and ALTER DATABASE
SET, as needed for the following patch.
See also:
2abae34a2e8fde42be731b4e18d44cd08901464d - added function SET
0c66a223774dec62edb5281a47e72fe480a8f7aa - (partially) updated comments about PGC_S_TEST
---
src/backend/access/table/tableamapi.c | 2 +-
src/backend/catalog/pg_db_role_setting.c | 8 ++++----
src/backend/commands/functioncmds.c | 4 ++--
src/backend/commands/tablespace.c | 4 ++--
src/backend/commands/variable.c | 8 ++++----
src/backend/storage/buffer/localbuf.c | 2 +-
src/backend/utils/cache/ts_cache.c | 2 +-
src/backend/utils/misc/guc.c | 18 +++++++++---------
src/include/utils/guc.h | 7 ++++---
9 files changed, 28 insertions(+), 27 deletions(-)
diff --git a/src/backend/access/table/tableamapi.c b/src/backend/access/table/tableamapi.c
index e9b598256fb..7c0f08ed46d 100644
--- a/src/backend/access/table/tableamapi.c
+++ b/src/backend/access/table/tableamapi.c
@@ -132,7 +132,7 @@ check_default_table_access_method(char **newval, void **extra, GucSource source)
* nonexistent table access method, only a NOTICE. See comments in
* guc.h.
*/
- if (source == PGC_S_TEST)
+ if (source == PGC_S_TEST || source == PGC_S_TEST_FUNCTION)
{
ereport(NOTICE,
(errcode(ERRCODE_UNDEFINED_OBJECT),
diff --git a/src/backend/catalog/pg_db_role_setting.c b/src/backend/catalog/pg_db_role_setting.c
index 8c20f519fc0..abfcc5655b3 100644
--- a/src/backend/catalog/pg_db_role_setting.c
+++ b/src/backend/catalog/pg_db_role_setting.c
@@ -70,7 +70,7 @@ AlterSetting(Oid databaseid, Oid roleid, VariableSetStmt *setstmt)
RelationGetDescr(rel), &isnull);
if (!isnull)
- new = GUCArrayReset(DatumGetArrayTypeP(datum));
+ new = GUCArrayReset(DatumGetArrayTypeP(datum), PGC_S_TEST);
if (new)
{
@@ -115,9 +115,9 @@ AlterSetting(Oid databaseid, Oid roleid, VariableSetStmt *setstmt)
/* Update (valuestr is NULL in RESET cases) */
if (valuestr)
- a = GUCArrayAdd(a, setstmt->name, valuestr);
+ a = GUCArrayAdd(a, setstmt->name, valuestr, PGC_S_TEST);
else
- a = GUCArrayDelete(a, setstmt->name);
+ a = GUCArrayDelete(a, setstmt->name, PGC_S_TEST);
if (a)
{
@@ -141,7 +141,7 @@ AlterSetting(Oid databaseid, Oid roleid, VariableSetStmt *setstmt)
memset(nulls, false, sizeof(nulls));
- a = GUCArrayAdd(NULL, setstmt->name, valuestr);
+ a = GUCArrayAdd(NULL, setstmt->name, valuestr, PGC_S_TEST);
values[Anum_pg_db_role_setting_setdatabase - 1] =
ObjectIdGetDatum(databaseid);
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 6593fd7d811..30a58cf027c 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -657,9 +657,9 @@ update_proconfig_value(ArrayType *a, List *set_items)
char *valuestr = ExtractSetVariableArgs(sstmt);
if (valuestr)
- a = GUCArrayAdd(a, sstmt->name, valuestr);
+ a = GUCArrayAdd(a, sstmt->name, valuestr, PGC_S_TEST_FUNCTION);
else /* RESET */
- a = GUCArrayDelete(a, sstmt->name);
+ a = GUCArrayDelete(a, sstmt->name, PGC_S_TEST_FUNCTION);
}
}
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 113b4807315..b0a460c1622 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -1104,7 +1104,7 @@ check_default_tablespace(char **newval, void **extra, GucSource source)
* When source == PGC_S_TEST, don't throw a hard error for a
* nonexistent tablespace, only a NOTICE. See comments in guc.h.
*/
- if (source == PGC_S_TEST)
+ if (source == PGC_S_TEST || source == PGC_S_TEST_FUNCTION)
{
ereport(NOTICE,
(errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -1251,7 +1251,7 @@ check_temp_tablespaces(char **newval, void **extra, GucSource source)
curoid = get_tablespace_oid(curname, source <= PGC_S_TEST);
if (curoid == InvalidOid)
{
- if (source == PGC_S_TEST)
+ if (source == PGC_S_TEST || source == PGC_S_TEST_FUNCTION)
ereport(NOTICE,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("tablespace \"%s\" does not exist",
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 9345131711e..c9421b17952 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -831,7 +831,7 @@ check_session_authorization(char **newval, void **extra, GucSource source)
roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
if (!HeapTupleIsValid(roleTup))
{
- if (source == PGC_S_TEST)
+ if (source == PGC_S_TEST || source == PGC_S_TEST_FUNCTION)
{
ereport(NOTICE,
(errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -856,7 +856,7 @@ check_session_authorization(char **newval, void **extra, GucSource source)
if (roleid != GetAuthenticatedUserId() &&
!superuser_arg(GetAuthenticatedUserId()))
{
- if (source == PGC_S_TEST)
+ if (source == PGC_S_TEST || source == PGC_S_TEST_FUNCTION)
{
ereport(NOTICE,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -940,7 +940,7 @@ check_role(char **newval, void **extra, GucSource source)
roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
if (!HeapTupleIsValid(roleTup))
{
- if (source == PGC_S_TEST)
+ if (source == PGC_S_TEST || source == PGC_S_TEST_FUNCTION)
{
ereport(NOTICE,
(errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -965,7 +965,7 @@ check_role(char **newval, void **extra, GucSource source)
if (!InitializingParallelWorker &&
!member_can_set_role(GetSessionUserId(), roleid))
{
- if (source == PGC_S_TEST)
+ if (source == PGC_S_TEST || source == PGC_S_TEST_FUNCTION)
{
ereport(NOTICE,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 8da7dd6c98a..0e92f604061 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -707,7 +707,7 @@ check_temp_buffers(int *newval, void **extra, GucSource source)
* Once local buffers have been initialized, it's too late to change this.
* However, if this is only a test call, allow it.
*/
- if (source != PGC_S_TEST && NLocBuffer && NLocBuffer != *newval)
+ if ((source != PGC_S_TEST && source != PGC_S_TEST_FUNCTION) && NLocBuffer && NLocBuffer != *newval)
{
GUC_check_errdetail("\"temp_buffers\" cannot be changed after any temporary tables have been accessed in the session.");
return false;
diff --git a/src/backend/utils/cache/ts_cache.c b/src/backend/utils/cache/ts_cache.c
index 54de33eadd2..9d7e8b66763 100644
--- a/src/backend/utils/cache/ts_cache.c
+++ b/src/backend/utils/cache/ts_cache.c
@@ -628,7 +628,7 @@ check_default_text_search_config(char **newval, void **extra, GucSource source)
*/
if (!OidIsValid(cfgId))
{
- if (source == PGC_S_TEST)
+ if (source == PGC_S_TEST || source == PGC_S_TEST_FUNCTION)
{
ereport(NOTICE,
(errcode(ERRCODE_UNDEFINED_OBJECT),
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a043d529efa..e2734475189 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -252,7 +252,7 @@ static void reapply_stacked_values(struct config_generic *variable,
GucContext curscontext, GucSource cursource,
Oid cursrole);
static bool validate_option_array_item(const char *name, const char *value,
- bool skipIfNoPermissions);
+ bool skipIfNoPermissions, int source);
static void write_auto_conf_file(int fd, const char *filename, ConfigVariable *head);
static void replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p,
const char *name, const char *value);
@@ -6444,7 +6444,7 @@ ProcessGUCArray(ArrayType *array,
* to indicate the current table entry is NULL.
*/
ArrayType *
-GUCArrayAdd(ArrayType *array, const char *name, const char *value)
+GUCArrayAdd(ArrayType *array, const char *name, const char *value, int source)
{
struct config_generic *record;
Datum datum;
@@ -6455,7 +6455,7 @@ GUCArrayAdd(ArrayType *array, const char *name, const char *value)
Assert(value);
/* test if the option is valid and we're allowed to set it */
- (void) validate_option_array_item(name, value, false);
+ (void) validate_option_array_item(name, value, false, source);
/* normalize name (converts obsolete GUC names to modern spellings) */
record = find_option(name, false, true, WARNING);
@@ -6522,7 +6522,7 @@ GUCArrayAdd(ArrayType *array, const char *name, const char *value)
* is NULL then a null should be stored.
*/
ArrayType *
-GUCArrayDelete(ArrayType *array, const char *name)
+GUCArrayDelete(ArrayType *array, const char *name, int source)
{
struct config_generic *record;
ArrayType *newarray;
@@ -6532,7 +6532,7 @@ GUCArrayDelete(ArrayType *array, const char *name)
Assert(name);
/* test if the option is valid and we're allowed to set it */
- (void) validate_option_array_item(name, NULL, false);
+ (void) validate_option_array_item(name, NULL, false, source);
/* normalize name (converts obsolete GUC names to modern spellings) */
record = find_option(name, false, true, WARNING);
@@ -6592,7 +6592,7 @@ GUCArrayDelete(ArrayType *array, const char *name)
* those that are PGC_USERSET or we have permission to set
*/
ArrayType *
-GUCArrayReset(ArrayType *array)
+GUCArrayReset(ArrayType *array, int source)
{
ArrayType *newarray;
int i;
@@ -6630,7 +6630,7 @@ GUCArrayReset(ArrayType *array)
*eqsgn = '\0';
/* skip if we have permission to delete it */
- if (validate_option_array_item(val, NULL, true))
+ if (validate_option_array_item(val, NULL, true, source))
continue;
/* else add it to the output array */
@@ -6665,7 +6665,7 @@ GUCArrayReset(ArrayType *array)
*/
static bool
validate_option_array_item(const char *name, const char *value,
- bool skipIfNoPermissions)
+ bool skipIfNoPermissions, int source)
{
struct config_generic *gconf;
@@ -6726,7 +6726,7 @@ validate_option_array_item(const char *name, const char *value,
/* test for permissions and valid option value */
(void) set_config_option(name, value,
superuser() ? PGC_SUSET : PGC_USERSET,
- PGC_S_TEST, GUC_ACTION_SET, false, 0, false);
+ source, GUC_ACTION_SET, false, 0, false);
return true;
}
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index ff506bf48d9..84203109af7 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -119,6 +119,7 @@ typedef enum
PGC_S_OVERRIDE, /* special case to forcibly set default */
PGC_S_INTERACTIVE, /* dividing line for error reporting */
PGC_S_TEST, /* test per-database or per-user setting */
+ PGC_S_TEST_FUNCTION, /* function SET */
PGC_S_SESSION, /* SET command */
} GucSource;
@@ -406,9 +407,9 @@ extern void TransformGUCArray(ArrayType *array, List **names,
List **values);
extern void ProcessGUCArray(ArrayType *array,
GucContext context, GucSource source, GucAction action);
-extern ArrayType *GUCArrayAdd(ArrayType *array, const char *name, const char *value);
-extern ArrayType *GUCArrayDelete(ArrayType *array, const char *name);
-extern ArrayType *GUCArrayReset(ArrayType *array);
+extern ArrayType *GUCArrayAdd(ArrayType *array, const char *name, const char *value, int source);
+extern ArrayType *GUCArrayDelete(ArrayType *array, const char *name, int source);
+extern ArrayType *GUCArrayReset(ArrayType *array, int source);
extern void *guc_malloc(int elevel, size_t size);
extern pg_nodiscard void *guc_realloc(int elevel, void *old, size_t size);
--
2.42.0
[text/x-diff] 0002-warn-when-setting-GUC-to-a-nonextant-library.patch (10.6K, ../../Zp6IpxVT1U-qmMOC@pryzbyj2023/3-0002-warn-when-setting-GUC-to-a-nonextant-library.patch)
download | inline diff:
From 47eb2ab393e050ac98a6197a8302c83c23ca0df3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH 2/4] warn when setting GUC to a nonextant library
Note that warnings shouldn't be issued during startup (only fatals):
$ ./tmp_install/usr/local/pgsql/bin/postgres -D ./testrun/regress/regress/tmp_check/data -c shared_preload_libraries=ab
2023-07-06 14:47:03.817 CDT postmaster[2608106] FATAL: could not access file "ab": No existe el archivo o el directorio
2023-07-06 14:47:03.817 CDT postmaster[2608106] CONTEXT: while loading shared libraries for setting "shared_preload_libraries"
---
src/backend/commands/variable.c | 101 ++++++++++++++++++
src/backend/utils/fmgr/dfmgr.c | 3 +-
src/backend/utils/misc/guc.c | 6 ++
src/backend/utils/misc/guc_tables.c | 6 +-
src/include/fmgr.h | 1 +
src/include/utils/guc_hooks.h | 7 ++
.../unsafe_tests/expected/rolenames.out | 19 ++++
.../modules/unsafe_tests/sql/rolenames.sql | 13 +++
8 files changed, 151 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index c9421b17952..0714b4421dc 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -40,6 +40,7 @@
#include "utils/timestamp.h"
#include "utils/tzparser.h"
#include "utils/varlena.h"
+#include <unistd.h>
/*
* DATESTYLE
@@ -1234,3 +1235,103 @@ check_ssl(bool *newval, void **extra, GucSource source)
#endif
return true;
}
+
+
+/*
+ * See also load_libraries() and internal_load_library().
+ */
+static bool
+check_preload_libraries(char **newval, void **extra, GucSource source,
+ bool restricted)
+{
+ char *rawstring;
+ List *elemlist;
+ ListCell *l;
+
+ /* nothing to do if empty */
+ if (newval == NULL || *newval[0] == '\0')
+ return true;
+
+ /*
+ * Issue warnings only during an interactive SET from ALTER
+ * ROLE/DATABASE/SYSTEM, and not while applying GUCs during startup,
+ * when relevant errors would be FATAL, and the warnings would be redundant.
+ *
+ * IsUnderPostmaster is a hack to allow warning under ALTER SYSTEM but
+ * not during startup. Both of which use PGC_S_FILE.
+ */
+ if (!IsUnderPostmaster)
+ return true;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ /* Parse string into list of filename paths */
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
+ {
+ /* Should not happen ? */
+ return false;
+ }
+
+ foreach(l, elemlist)
+ {
+ /* Note that filename was already canonicalized */
+ char *filename = (char *) lfirst(l);
+ char *expanded = NULL;
+
+ /* If restricting, insert $libdir/plugins if not mentioned already */
+ if (restricted && first_dir_separator(filename) == NULL)
+ {
+ expanded = psprintf("$libdir/plugins/%s", filename);
+ filename = expanded;
+ }
+
+ filename = expand_dynamic_library_name(filename);
+
+ /*
+ * stat()/access() only check that the library exists, not that it has
+ * the correct magic number or even a library. But error messages
+ * from dlopen() are not portable, so it'd be hard to report any
+ * problem other than "does not exist".
+ */
+ if (access(filename, R_OK) == 0)
+ continue;
+
+ if (source == PGC_S_FILE)
+ /* ALTER SYSTEM */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("The server might fail to start with this setting."),
+ errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.",
+ "postgresql.auto.conf"));
+ else if (source == PGC_S_TEST)
+ /* ALTER ROLE/DATABASE */
+ ereport(WARNING,
+ errcode_for_file_access(),
+ errmsg("could not access file \"%s\"", filename),
+ errdetail("New sessions will fail to connect with the new setting."));
+ }
+
+ list_free_deep(elemlist);
+ pfree(rawstring);
+ return true;
+}
+
+bool
+check_shared_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
+
+bool
+check_local_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, true);
+}
+
+bool
+check_session_preload_libraries(char **newval, void **extra, GucSource source)
+{
+ return check_preload_libraries(newval, extra, source, false);
+}
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 092004dcf3b..b03017f6275 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -79,7 +79,6 @@ char *Dynamic_library_path;
static void *internal_load_library(const char *libname);
static void incompatible_module_error(const char *libname,
const Pg_magic_struct *module_magic_data) pg_attribute_noreturn();
-static char *expand_dynamic_library_name(const char *name);
static void check_restricted_library_name(const char *name);
static char *substitute_libpath_macro(const char *name);
static char *find_in_dynamic_libpath(const char *basename);
@@ -410,7 +409,7 @@ incompatible_module_error(const char *libname,
*
* The result will always be freshly palloc'd.
*/
-static char *
+char *
expand_dynamic_library_name(const char *name)
{
bool have_slash;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e2734475189..4cd40ca9678 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4654,6 +4654,12 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
union config_var_val newval;
void *newextra = NULL;
+ /*
+ * PGC_S_FILE is used even though we should use PGC_S_TEST,
+ * since various GUC validation functions use TEST to give a
+ * NOTICE rather than an ERROR, and we don't want to allow
+ * setting values that would always return an error later.
+ */
if (!parse_and_validate_value(record, name, value,
PGC_S_FILE, ERROR,
&newval, &newextra))
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 630ed0f1629..6c424be4c47 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4237,7 +4237,7 @@ struct config_string ConfigureNamesString[] =
},
&session_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_session_preload_libraries, NULL, NULL
},
{
@@ -4248,7 +4248,7 @@ struct config_string ConfigureNamesString[] =
},
&shared_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_shared_preload_libraries, NULL, NULL
},
{
@@ -4259,7 +4259,7 @@ struct config_string ConfigureNamesString[] =
},
&local_preload_libraries_string,
"",
- NULL, NULL, NULL
+ check_local_preload_libraries, NULL, NULL
},
{
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index ccb4070a251..c6319782a99 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -749,6 +749,7 @@ extern void **find_rendezvous_variable(const char *varName);
extern Size EstimateLibraryStateSpace(void);
extern void SerializeLibraryState(Size maxsize, char *start_address);
extern void RestoreLibraryState(char *start_address);
+char *expand_dynamic_library_name(const char *name);
/*
* Support for aggregate functions
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 153c652c93e..a11c5488041 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -172,4 +172,11 @@ extern bool check_synchronized_standby_slots(char **newval, void **extra,
GucSource source);
extern void assign_synchronized_standby_slots(const char *newval, void *extra);
+extern bool check_local_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_session_preload_libraries(char **newval, void **extra,
+ GucSource source);
+extern bool check_shared_preload_libraries(char **newval, void **extra,
+ GucSource source);
+
#endif /* GUC_HOOKS_H */
diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out
index 61396b2a805..de707a4803e 100644
--- a/src/test/modules/unsafe_tests/expected/rolenames.out
+++ b/src/test/modules/unsafe_tests/expected/rolenames.out
@@ -1083,6 +1083,25 @@ RESET SESSION AUTHORIZATION;
ERROR: current transaction is aborted, commands ignored until end of transaction block
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will fail to connect with the new setting.
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will fail to connect with the new setting.
+CREATE DATABASE regression_nosuch_db;
+ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist";
+WARNING: could not access file "DoesNotExist"
+DETAIL: New sessions will fail to connect with the new setting.
+ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist";
+WARNING: could not access file "$libdir/plugins/DoesNotExist"
+DETAIL: New sessions will fail to connect with the new setting.
+DROP DATABASE regression_nosuch_db;
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
-- clean up
\c
DROP SCHEMA test_roles_schema;
diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql
index adac36536db..56fb52d4e08 100644
--- a/src/test/modules/unsafe_tests/sql/rolenames.sql
+++ b/src/test/modules/unsafe_tests/sql/rolenames.sql
@@ -494,6 +494,19 @@ RESET SESSION AUTHORIZATION;
ROLLBACK;
REVOKE pg_read_all_settings FROM regress_role_haspriv;
+-- test some warnings
+ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist";
+ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist";
+CREATE DATABASE regression_nosuch_db;
+ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist";
+ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist";
+DROP DATABASE regression_nosuch_db;
+
+-- SET doesn't do anything, but should not warn, either
+SET session_preload_libraries="DoesNotExist";
+SET SESSION session_preload_libraries="DoesNotExist";
+begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback;
+
-- clean up
\c
--
2.42.0
[text/x-diff] 0003-errcontext-if-server-fails-to-start-due-to-library-G.patch (4.5K, ../../Zp6IpxVT1U-qmMOC@pryzbyj2023/4-0003-errcontext-if-server-fails-to-start-due-to-library-G.patch)
download | inline diff:
From 71d670eec45d0c7f9bfa6684e112b8bf5d6552fe Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 18 Dec 2021 22:51:01 -0600
Subject: [PATCH 3/4] errcontext if server fails to start due to library GUCs
---
src/backend/utils/fmgr/dfmgr.c | 21 ++++++++++++++++-----
src/backend/utils/init/miscinit.c | 2 +-
src/include/fmgr.h | 1 +
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index b03017f6275..fd8371cb582 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 check_restricted_library_name(const char *name);
@@ -112,7 +112,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)
@@ -141,6 +141,15 @@ 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;
@@ -152,7 +161,7 @@ load_file(const char *filename, bool restricted)
fullname = expand_dynamic_library_name(filename);
/* Load the shared library */
- (void) internal_load_library(fullname);
+ (void) internal_load_library(fullname, gucname);
pfree(fullname);
}
@@ -171,6 +180,7 @@ lookup_external_function(void *filehandle, const char *funcname)
/*
* Load the specified dynamic-link library file, unless it already is
* loaded. Return the pg_dl* handle for the file.
+ * gucname may be passed for error reports.
*
* Note: libname is expected to be an exact name for the library file.
*
@@ -180,7 +190,7 @@ lookup_external_function(void *filehandle, const char *funcname)
* perhaps other things that are definitely unsafe currently.
*/
static void *
-internal_load_library(const char *libname)
+internal_load_library(const char *libname, const char *gucname)
{
DynamicFileList *file_scanner;
PGModuleMagicFunction magic_func;
@@ -205,6 +215,7 @@ internal_load_library(const char *libname)
if (stat(libname, &stat_buf) == -1)
ereport(ERROR,
(errcode_for_file_access(),
+ gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
errmsg("could not access file \"%s\": %m",
libname)));
@@ -676,7 +687,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 537d92c0cfd..8f11f718135 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1825,7 +1825,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 c6319782a99..22e70dc43ea 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -745,6 +745,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.42.0
[text/x-diff] 0004-show-the-GUC-source-in-errcontext.patch (3.4K, ../../Zp6IpxVT1U-qmMOC@pryzbyj2023/5-0004-show-the-GUC-source-in-errcontext.patch)
download | inline diff:
From b3aa9009491b2421f5c99dcd3911d1d17740d6ab Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 2 Feb 2022 20:54:49 -0600
Subject: [PATCH 4/4] show the GUC source in errcontext
//-os-only: freebsd
---
src/backend/utils/fmgr/dfmgr.c | 14 +++++++++++---
src/backend/utils/misc/guc_funcs.c | 26 ++++++++++++++++++++++++++
src/include/utils/guc.h | 1 +
3 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index fd8371cb582..7666919d78d 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -35,6 +35,7 @@
#include "miscadmin.h"
#include "storage/fd.h"
#include "storage/shmem.h"
+#include "utils/guc_tables.h"
#include "utils/hsearch.h"
@@ -213,11 +214,18 @@ internal_load_library(const char *libname, const char *gucname)
* Check for same files - different paths (ie, symlink or link)
*/
if (stat(libname, &stat_buf) == -1)
+ {
+ char *errstr = strerror(errno);
+ int linenum;
+ char *sourcefile = gucname ? GetConfigSourceFile(gucname, &linenum) : NULL;
+
ereport(ERROR,
(errcode_for_file_access(),
gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0,
- errmsg("could not access file \"%s\": %m",
- libname)));
+ sourcefile ? errcontext("from %s:%d", sourcefile, linenum) : 0,
+ errmsg("could not access file \"%s\": %s",
+ libname, errstr)));
+ }
for (file_scanner = file_list;
file_scanner != NULL &&
@@ -281,7 +289,7 @@ internal_load_library(const char *libname, const char *gucname)
}
else
{
- /* try to close library */
+ /* try to close library */ // Not needed due to ERROR ? //
dlclose(file_scanner->handle);
free(file_scanner);
/* complain */
diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index 9c9edd3d2f5..eb7c3d726b2 100644
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -177,6 +177,32 @@ ExtractSetVariableArgs(VariableSetStmt *stmt)
}
}
+
+/*
+ * Get the source file and line associated with the given option, if it was set
+ * by a file.
+ *
+ * If the option doesn't exist, throw an ereport and don't return.
+ */
+char *
+GetConfigSourceFile(const char *name, int *linenum)
+{
+ struct config_generic *record;
+
+ record = find_option(name, false, false, ERROR);
+
+ /* Should not happen */
+ if (record == NULL)
+ return NULL;
+
+ if (record->source != PGC_S_FILE)
+ return NULL;
+
+ *linenum = record->sourceline;
+ return record->sourcefile;
+}
+
+
/*
* flatten_set_variable_args
* Given a parsenode List as emitted by the grammar for SET,
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 84203109af7..6aaff5768ba 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -365,6 +365,7 @@ extern const char *GetConfigOption(const char *name, bool missing_ok,
bool restrict_privileged);
extern const char *GetConfigOptionResetString(const char *name);
extern int GetConfigOptionFlags(const char *name, bool missing_ok);
+extern char *GetConfigSourceFile(const char *name, int *linenum);
extern void ProcessConfigFile(GucContext context);
extern char *convert_GUC_name_for_parameter_acl(const char *name);
extern void check_GUC_name_for_parameter_acl(const char *name);
--
2.42.0
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2024-11-11 10:09 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 42+ messages in thread
From: Heikki Linnakangas @ 2024-11-11 10:09 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; Maciek Sakrejda <m.sakrejda@gmail.com>
I had a quick glance at this, and I agree with Robert's comment earlier
that this requires a lot of context to understand.
Does this change the behavior of what is accepted or not? What are those
cases? Can you give some examples, please?
When do you get the new warnings? Examples please.
Does this require documentation changes?
On 22/07/2024 19:28, Justin Pryzby wrote:
> And added a preparatory patch to distinguish ALTER USER/DATABASE SET
> from SET in a function, to avoid warning in that case.
The comment above enum GucSource needs updating.
PGC_S_TEST_FUNCTION is missing from GucSource_Names (there's a reminder
of this in the comment above enum GucSource). Are there other places
where PGC_S_TEST_FUNCTION is missing?
Do extensions need any changes for this? Consider renaming PGC_S_TEST
too, to intentionally break any code that might now need to also check
for PGC_S_TEST_FUNCTION.
Or perhaps there's a better way to distinguish between ALTER
DATABASE/ROLE and CREATE FUNCTION settings. We already have different
GucSource codes for per-database and per-user settings; I wonder if it
would be better to use those codes and a separate "is_testing" flag.
That would also naturally allow the combination of "source == PGC_S_FILE
&& is_testing==true", if some settings would need different checks in
ALTER SYSTEM for example.
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 42+ messages in thread
* Re: warn if GUC set to an invalid shared library
@ 2024-12-11 05:12 Michael Paquier <michael@paquier.xyz>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 0 replies; 42+ messages in thread
From: Michael Paquier @ 2024-12-11 05:12 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-hackers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>; David G. Johnston <david.g.johnston@gmail.com>; Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>; Cary Huang <cary.huang@highgo.ca>; Maciek Sakrejda <m.sakrejda@gmail.com>
On Mon, Nov 11, 2024 at 12:09:15PM +0200, Heikki Linnakangas wrote:
> I had a quick glance at this, and I agree with Robert's comment earlier that
> this requires a lot of context to understand.
All this feedback was 4 weeks ago, and is unanswered, so I've marked
the CF entry as returned with feedback. Feel free to resubmit if
necessary.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../Z1kfMUoZkl9P0egB@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 42+ messages in thread
end of thread, other threads:[~2024-12-11 05:12 UTC | newest]
Thread overview: 42+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-12-28 17:45 warn if GUC set to an invalid shared library Justin Pryzby <pryzby@telsasoft.com>
2021-12-30 08:20 ` Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
2022-01-08 21:32 ` Maciek Sakrejda <m.sakrejda@gmail.com>
2022-01-08 21:29 ` Maciek Sakrejda <m.sakrejda@gmail.com>
2022-01-08 22:07 ` Justin Pryzby <pryzby@telsasoft.com>
2022-01-09 19:58 ` Maciek Sakrejda <m.sakrejda@gmail.com>
2022-01-27 22:01 ` Justin Pryzby <pryzby@telsasoft.com>
2022-01-28 14:42 ` Robert Haas <robertmhaas@gmail.com>
2022-01-28 23:36 ` Cary Huang <cary.huang@highgo.ca>
2022-01-29 00:09 ` Justin Pryzby <pryzby@telsasoft.com>
2022-02-02 06:06 ` Maciek Sakrejda <m.sakrejda@gmail.com>
2022-02-02 15:39 ` David G. Johnston <david.g.johnston@gmail.com>
2022-02-04 04:36 ` Maciek Sakrejda <m.sakrejda@gmail.com>
2022-02-10 01:58 ` Justin Pryzby <pryzby@telsasoft.com>
2022-02-14 18:12 ` Maciek Sakrejda <m.sakrejda@gmail.com>
2022-03-23 19:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-06-16 03:14 ` Michael Paquier <michael@paquier.xyz>
2022-06-16 03:50 ` Justin Pryzby <pryzby@telsasoft.com>
2022-07-22 01:54 ` Justin Pryzby <pryzby@telsasoft.com>
2022-07-22 17:42 ` Maciek Sakrejda <m.sakrejda@gmail.com>
2022-07-22 17:53 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-07-22 18:35 ` Justin Pryzby <pryzby@telsasoft.com>
2022-07-22 19:00 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-07-22 19:14 ` Justin Pryzby <pryzby@telsasoft.com>
2022-07-22 19:26 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-09-02 22:24 ` Justin Pryzby <pryzby@telsasoft.com>
2022-10-12 05:34 ` Michael Paquier <michael@paquier.xyz>
2022-10-29 17:40 ` Justin Pryzby <pryzby@telsasoft.com>
2022-10-30 23:12 ` Maciek Sakrejda <m.sakrejda@gmail.com>
2022-10-31 13:31 ` Justin Pryzby <pryzby@telsasoft.com>
2022-10-31 13:43 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-11-01 22:26 ` Justin Pryzby <pryzby@telsasoft.com>
2023-07-06 20:15 ` Justin Pryzby <pryzby@telsasoft.com>
2023-12-28 05:26 ` Shubham Khanna <khannashubham1197@gmail.com>
2024-01-01 12:16 ` John Naylor <johncnaylorls@gmail.com>
2024-01-07 13:27 ` Justin Pryzby <pryzby@telsasoft.com>
2024-05-24 13:26 ` Robert Haas <robertmhaas@gmail.com>
2024-05-24 15:48 ` Justin Pryzby <pryzby@telsasoft.com>
2024-05-24 17:15 ` Robert Haas <robertmhaas@gmail.com>
2024-07-22 16:28 ` Justin Pryzby <pryzby@telsasoft.com>
2024-11-11 10:09 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-12-11 05:12 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox