On Fri, May 24, 2024 at 09:26:54AM -0400, Robert Haas wrote:
> On Thu, Jul 6, 2023 at 4:15 PM Justin Pryzby <pry...@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
>From d29d0cfcf9d15dad7db1d0dd334e3e77cdad653f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Mon, 13 Dec 2021 08:42:38 -0600
Subject: [PATCH 1/3] warn 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
>From 5377176aeb5987afbdc1a59e67eed9d3e066f724 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sat, 18 Dec 2021 22:51:01 -0600
Subject: [PATCH 2/3] errcontext if server fails to start due to library GUCs
---
src/backend/utils/fmgr/dfmgr.c | 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
>From c339d87cd7ce9f73015d62063d9104459ab71fc4 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
