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'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 <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), This is rebased over my own patch to enable checks for REGRESSION_TEST_NAME_RESTRICTIONS. -- Justin
>From aa4c3ae08b3379bcd222e00aa896a40f811155a5 Mon Sep 17 00:00:00 2001 From: Justin Pryzby <pryz...@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
>From b05c738bddfca11d4d0510c497a121cc8fcb585a 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 | 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
>From 4a37cfde4b6e84cc309e2e2c99d24b88dcb1ddaf 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 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
>From 0dabe3b9d43091183563ddeb8a47866c427b9c00 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 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