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
>From 29610d4e417a1a0cf099e10736b5c14aec90f641 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From 47eb2ab393e050ac98a6197a8302c83c23ca0df3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From 71d670eec45d0c7f9bfa6684e112b8bf5d6552fe Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From b3aa9009491b2421f5c99dcd3911d1d17740d6ab Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
