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 <pry...@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
>From defdb57fe0ec373c1eea8df42f0e1831b3f9c3cc Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From 0e197f9e3599b555c019574aaf4b36f15fe82cec Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From ee5328948981306355516b5e7b873f2c2caf4a4a Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From f90a59e09bccc97b93de8eec982c0ce719155a60 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
