agora inbox for pgsql-hackers@postgresql.orghelp / color / mirror / Atom feed
[PATCH v6 2/4] warn when setting GUC to a nonextant library 14+ messages / 2 participants [nested] [flat]
* [PATCH v6 2/4] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) 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 --vDrIcvfimaPJgjx+ Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0003-errcontext-if-server-fails-to-start-due-to-librar.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH v5 1/3] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) --- 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 --3siQDZowHQqNOShm Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5-0002-errcontext-if-server-fails-to-start-due-to-librar.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH v3 2/2] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) 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 --9Q79yvCDNTHf0qBd-- ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH 2/4] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) 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 --e3dHrZwCppoiYgqG Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0003-errcontext-if-server-fails-to-start-due-to-library-G.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH 1/3] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) 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 --yJSDWdBuJ1ro2+cx Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0002-errcontext-if-server-fails-to-start-due-to-library-G.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH v8 2/4] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) 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 --E4B85hAaG7CiJkTa Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0003-errcontext-if-server-fails-to-start-due-to-librar.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH v8 2/4] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) 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 --E4B85hAaG7CiJkTa Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0003-errcontext-if-server-fails-to-start-due-to-librar.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH 2/4] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) 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 --Cz2/h24zxIeGl+Za Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0003-errcontext-if-server-fails-to-start-due-to-library-G.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH v6 2/4] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) 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 --vDrIcvfimaPJgjx+ Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0003-errcontext-if-server-fails-to-start-due-to-librar.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH v4 1/3] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) --- 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 --1Ow488MNN9B9o/ov Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v4-0002-errcontext-if-server-fails-to-start-due-to-librar.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH 1/3] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) 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 --yJSDWdBuJ1ro2+cx Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0002-errcontext-if-server-fails-to-start-due-to-library-G.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH 2/4] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) 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 --e3dHrZwCppoiYgqG Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0003-errcontext-if-server-fails-to-start-due-to-library-G.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH v8 2/4] warn when setting GUC to a nonextant library @ 2021-12-13 14:42 Justin Pryzby <pryzbyj@telsasoft.com> 0 siblings, 0 replies; 14+ messages in thread From: Justin Pryzby @ 2021-12-13 14:42 UTC (permalink / raw) 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 --E4B85hAaG7CiJkTa Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0003-errcontext-if-server-fails-to-start-due-to-librar.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
* [PATCH v53 1/7] Introduce an option to make logical replication database specific. @ 2026-04-04 15:16 Antonin Houska <ah@cybertec.at> 0 siblings, 0 replies; 14+ messages in thread From: Antonin Houska @ 2026-04-04 15:16 UTC (permalink / raw) By default, the logical decoding assumes access to shared catalogs, so the snapshot builder needs to consider cluster-wide XIDs during startup. That in turn means that, if any transaction is already running (and has XID assigned), the snapshot builder needs to wait for its completion, as it does not know if that transaction performed catalog changes earlier. Possible problem with this concept is that if REPACK (CONCURRENTLY) is running in some database, backends running the same command in other databases get stuck until the first one has committed. Thus only as single backend in the cluster can run REPACK (CONCURRENTLY) at any time. Likewise, REPACK (CONCURRENTLY) can block walsenders starting on behalf of subscriptions throughout the cluster. This patch adds a new option to logical replication output plugin, to declare that it does not use shared catalogs (i.e. catalogs that can be changed by transactions running in other databases in the cluster). In that case, no snapshot the backend will use during the decoding needs to contain information about transactions running in other databases. Thus the snapshot builder only needs to wait for completion of transactions in the current database. Currently we only use this option in the REPACK background worker. It could possibly be used in the plugin for logical replication too, however that would need thorough analysis of that plugin. The patch bumps WAL version number, due to a new field in xl_running_xacts. --- contrib/pg_visibility/pg_visibility.c | 4 +- doc/src/sgml/logicaldecoding.sgml | 4 ++ src/backend/access/index/genam.c | 8 +++ src/backend/access/rmgrdesc/standbydesc.c | 2 + src/backend/access/transam/xlog.c | 2 +- src/backend/access/transam/xlogfuncs.c | 2 +- src/backend/postmaster/bgwriter.c | 2 +- src/backend/replication/logical/decode.c | 8 ++- src/backend/replication/logical/logical.c | 3 ++ src/backend/replication/logical/snapbuild.c | 59 ++++++++++++++++++--- src/backend/replication/slot.c | 2 +- src/backend/storage/ipc/procarray.c | 23 +++++++- src/backend/storage/ipc/standby.c | 14 ++++- src/include/access/genam.h | 7 +++ src/include/access/xlog_internal.h | 2 +- src/include/replication/output_plugin.h | 1 + src/include/replication/snapbuild.h | 3 +- src/include/storage/procarray.h | 2 +- src/include/storage/standby.h | 3 +- src/include/storage/standbydefs.h | 1 + 20 files changed, 131 insertions(+), 21 deletions(-) diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c index dfab0b64cf5..d564bd2a00c 100644 --- a/contrib/pg_visibility/pg_visibility.c +++ b/contrib/pg_visibility/pg_visibility.c @@ -621,7 +621,7 @@ GetStrictOldestNonRemovableTransactionId(Relation rel) else if (rel == NULL || rel->rd_rel->relisshared) { /* Shared relation: take into account all running xids */ - runningTransactions = GetRunningTransactionData(); + runningTransactions = GetRunningTransactionData(InvalidOid); LWLockRelease(ProcArrayLock); LWLockRelease(XidGenLock); return runningTransactions->oldestRunningXid; @@ -632,7 +632,7 @@ GetStrictOldestNonRemovableTransactionId(Relation rel) * Normal relation: take into account xids running within the current * database */ - runningTransactions = GetRunningTransactionData(); + runningTransactions = GetRunningTransactionData(InvalidOid); LWLockRelease(ProcArrayLock); LWLockRelease(XidGenLock); return runningTransactions->oldestDatabaseRunningXid; diff --git a/doc/src/sgml/logicaldecoding.sgml b/doc/src/sgml/logicaldecoding.sgml index 6dc49108997..28089d1053c 100644 --- a/doc/src/sgml/logicaldecoding.sgml +++ b/doc/src/sgml/logicaldecoding.sgml @@ -959,6 +959,7 @@ typedef struct OutputPluginOptions { OutputPluginOutputType output_type; bool receive_rewrites; + bool need_shared_catalogs; } OutputPluginOptions; </programlisting> <literal>output_type</literal> has to either be set to @@ -969,6 +970,9 @@ typedef struct OutputPluginOptions also be called for changes made by heap rewrites during certain DDL operations. These are of interest to plugins that handle DDL replication, but they require special handling. + <literal>need_shared_catalogs</literal> can be set to false if you are + sure the plugin functions do not access shared system catalogs. It can + speed up creation of replication slots that use this plugin. </para> <para> diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c index 1408989c568..44df2605068 100644 --- a/src/backend/access/index/genam.c +++ b/src/backend/access/index/genam.c @@ -394,6 +394,14 @@ systable_beginscan(Relation heapRelation, SysScanDesc sysscan; Relation irel; + /* + * If this backend promised that it won't access shared catalogs during + * logical decoding, this seems to be the right place to check. + */ + Assert(!HistoricSnapshotActive() || + accessSharedCatalogsInDecoding || + !heapRelation->rd_rel->relisshared); + if (indexOK && !IgnoreSystemIndexes && !ReindexIsProcessingIndex(indexId)) diff --git a/src/backend/access/rmgrdesc/standbydesc.c b/src/backend/access/rmgrdesc/standbydesc.c index 0a291354ae2..685d1bdb024 100644 --- a/src/backend/access/rmgrdesc/standbydesc.c +++ b/src/backend/access/rmgrdesc/standbydesc.c @@ -41,6 +41,8 @@ standby_desc_running_xacts(StringInfo buf, xl_running_xacts *xlrec) for (i = 0; i < xlrec->subxcnt; i++) appendStringInfo(buf, " %u", xlrec->xids[xlrec->xcnt + i]); } + + appendStringInfo(buf, "; dbid: %u", xlrec->dbid); } void diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 9e8999bbb61..1a6c1456d2c 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -7731,7 +7731,7 @@ CreateCheckPoint(int flags) * recovery we don't need to write running xact data. */ if (!shutdown && XLogStandbyInfoActive()) - LogStandbySnapshot(); + LogStandbySnapshot(InvalidOid); START_CRIT_SECTION(); diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index 65bbaeda59c..0f5979691e6 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -245,7 +245,7 @@ pg_log_standby_snapshot(PG_FUNCTION_ARGS) (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("pg_log_standby_snapshot() can only be used if \"wal_level\" >= \"replica\""))); - recptr = LogStandbySnapshot(); + recptr = LogStandbySnapshot(InvalidOid); /* * As a convenience, return the WAL location of the last inserted record diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c index 1d8947774a9..a30de4262eb 100644 --- a/src/backend/postmaster/bgwriter.c +++ b/src/backend/postmaster/bgwriter.c @@ -289,7 +289,7 @@ BackgroundWriterMain(const void *startup_data, size_t startup_data_len) if (now >= timeout && last_snapshot_lsn <= GetLastImportantRecPtr()) { - last_snapshot_lsn = LogStandbySnapshot(); + last_snapshot_lsn = LogStandbySnapshot(InvalidOid); last_snapshot_ts = now; } } diff --git a/src/backend/replication/logical/decode.c b/src/backend/replication/logical/decode.c index 57aaef57c61..4299d0bc867 100644 --- a/src/backend/replication/logical/decode.c +++ b/src/backend/replication/logical/decode.c @@ -381,7 +381,13 @@ standby_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf) { xl_running_xacts *running = (xl_running_xacts *) XLogRecGetData(r); - SnapBuildProcessRunningXacts(builder, buf->origptr, running); + + /* + * If the plugin does not access shared catalogs, only keep + * track of transactions in the current database. + */ + SnapBuildProcessRunningXacts(builder, buf->origptr, running, + !ctx->options.need_shared_catalogs); /* * Abort all transactions that we keep track of, that are diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c index f20d0c542f3..8ceaf64d164 100644 --- a/src/backend/replication/logical/logical.c +++ b/src/backend/replication/logical/logical.c @@ -285,6 +285,9 @@ StartupDecodingContext(List *output_plugin_options, ctx->write = do_write; ctx->update_progress = update_progress; + /* Assume shared catalog access. The startup callback can change it. */ + ctx->options.need_shared_catalogs = true; + ctx->output_plugin_options = output_plugin_options; ctx->fast_forward = fast_forward; diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index b4269a3b102..a6738e120c7 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -125,6 +125,7 @@ #include <sys/stat.h> #include <unistd.h> +#include "access/genam.h" #include "access/heapam_xlog.h" #include "access/transam.h" #include "access/xact.h" @@ -154,6 +155,14 @@ static ResourceOwner SavedResourceOwnerDuringExport = NULL; static bool ExportInProgress = false; +/* + * If a backend is going to do logical decoding and the output plugin does + * not need to access shared catalogs, setting this variable to false can make + * the decoding startup faster. In particular, the backend will not need to + * wait for completion of already running transactions in other databases. + */ +bool accessSharedCatalogsInDecoding = true; + /* ->committed and ->catchange manipulation */ static void SnapBuildPurgeOlderTxn(SnapBuild *builder); @@ -170,7 +179,8 @@ static inline bool SnapBuildXidHasCatalogChanges(SnapBuild *builder, Transaction uint32 xinfo); /* xlog reading helper functions for SnapBuildProcessRunningXacts */ -static bool SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *running); +static bool SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, + xl_running_xacts *running, bool db_specific); static void SnapBuildWaitSnapshot(xl_running_xacts *running, TransactionId cutoff); /* serialization functions */ @@ -1136,7 +1146,8 @@ SnapBuildXidHasCatalogChanges(SnapBuild *builder, TransactionId xid, * anymore. */ void -SnapBuildProcessRunningXacts(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *running) +SnapBuildProcessRunningXacts(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *running, + bool db_specific) { ReorderBufferTXN *txn; TransactionId xmin; @@ -1149,7 +1160,7 @@ SnapBuildProcessRunningXacts(SnapBuild *builder, XLogRecPtr lsn, xl_running_xact if (builder->state < SNAPBUILD_CONSISTENT) { /* returns false if there's no point in performing cleanup just yet */ - if (!SnapBuildFindSnapshot(builder, lsn, running)) + if (!SnapBuildFindSnapshot(builder, lsn, running, db_specific)) return; } else @@ -1166,8 +1177,14 @@ SnapBuildProcessRunningXacts(SnapBuild *builder, XLogRecPtr lsn, xl_running_xact * (see SnapBuildCommitTxn). Because of this, xmax can be lower than * xmin, which looks odd but is correct and actually more efficient, since * we hit fast paths in heapam_visibility.c. + * + * If database specific transaction info was used during startup, the info + * for the whole cluster can make xmin go backwards. That would be bad + * because we might no longer have older XIDs in ->committed. */ - builder->xmin = running->oldestRunningXid; + if (!db_specific || + NormalTransactionIdFollows(running->oldestRunningXid, builder->xmin)) + builder->xmin = running->oldestRunningXid; /* Remove transactions we don't need to keep track off anymore */ SnapBuildPurgeOlderTxn(builder); @@ -1182,7 +1199,11 @@ SnapBuildProcessRunningXacts(SnapBuild *builder, XLogRecPtr lsn, xl_running_xact * the record we're reading now. */ xmin = ReorderBufferGetOldestXmin(builder->reorder); - if (xmin == InvalidTransactionId) + + /* + * Like above, do not let slot xmin go backwards. + */ + if (xmin == InvalidTransactionId && !db_specific) xmin = running->oldestRunningXid; elog(DEBUG3, "xmin: %u, xmax: %u, oldest running: %u, oldest xmin: %u", builder->xmin, builder->xmax, running->oldestRunningXid, xmin); @@ -1238,7 +1259,8 @@ SnapBuildProcessRunningXacts(SnapBuild *builder, XLogRecPtr lsn, xl_running_xact * using the xl_running_xacts record. */ static bool -SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *running) +SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *running, + bool db_specific) { /* --- * Build catalog decoding snapshot incrementally using information about @@ -1265,6 +1287,25 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn * --- */ + if (db_specific) + { + /* + * If we must only keep track of transactions running in the current + * database, we need transaction info from exactly that database. + */ + if (running->dbid != MyDatabaseId) + { + LogStandbySnapshot(MyDatabaseId); + return false; + } + + /* + * We'd better be able to check during scan if the plugin does not + * lie. + */ + accessSharedCatalogsInDecoding = false; + } + /* * xl_running_xacts record is older than what we can use, we might not * have all necessary catalog rows anymore. @@ -1465,7 +1506,11 @@ SnapBuildWaitSnapshot(xl_running_xacts *running, TransactionId cutoff) */ if (!RecoveryInProgress()) { - LogStandbySnapshot(); + /* + * If the last transaction info was about specific database, so needs + * to be the next one - at least until we're in the CONSISTENT state. + */ + LogStandbySnapshot(running->dbid); } } diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index a9092fc2382..9533515a63b 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -1762,7 +1762,7 @@ ReplicationSlotReserveWal(void) XLogRecPtr flushptr; /* make sure we have enough information to start */ - flushptr = LogStandbySnapshot(); + flushptr = LogStandbySnapshot(InvalidOid); /* and make sure it's fsynced to disk */ XLogFlush(flushptr); diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index cc207cb56e3..92ed34128e5 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -2631,9 +2631,11 @@ ProcArrayInstallRestoredXmin(TransactionId xmin, PGPROC *proc) * * Note that if any transaction has overflowed its cached subtransactions * then there is no real need include any subtransactions. + * + * If 'dbid' is valid, only gather transactions running in that database. */ RunningTransactions -GetRunningTransactionData(void) +GetRunningTransactionData(Oid dbid) { /* result workspace */ static RunningTransactionsData CurrentRunningXactsData; @@ -2708,6 +2710,18 @@ GetRunningTransactionData(void) if (!TransactionIdIsValid(xid)) continue; + /* + * Filter by database OID if requested. + */ + if (OidIsValid(dbid)) + { + int pgprocno = arrayP->pgprocnos[index]; + PGPROC *proc = &allProcs[pgprocno]; + + if (proc->databaseId != dbid) + continue; + } + /* * Be careful not to exclude any xids before calculating the values of * oldestRunningXid and suboverflowed, since these are used to clean @@ -2758,6 +2772,12 @@ GetRunningTransactionData(void) PGPROC *proc = &allProcs[pgprocno]; int nsubxids; + /* + * Filter by database OID if requested. + */ + if (OidIsValid(dbid) && proc->databaseId != dbid) + continue; + /* * Save subtransaction XIDs. Other backends can't add or remove * entries while we're holding XidGenLock. @@ -2791,6 +2811,7 @@ GetRunningTransactionData(void) * increases if slots do. */ + CurrentRunningXacts->dbid = dbid; CurrentRunningXacts->xcnt = count - subcount; CurrentRunningXacts->subxcnt = subcount; CurrentRunningXacts->subxid_status = suboverflowed ? SUBXIDS_IN_SUBTRANS : SUBXIDS_IN_ARRAY; diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index de9092fdf5b..c653ea742bc 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -1188,6 +1188,14 @@ standby_redo(XLogReaderState *record) xl_running_xacts *xlrec = (xl_running_xacts *) XLogRecGetData(record); RunningTransactionsData running; + /* + * Records issued for specific database are not suitable for physical + * replication because that affects the whole cluster. In particular, + * the list of XID is probably incomplete here. + */ + if (OidIsValid(xlrec->dbid)) + return; + running.xcnt = xlrec->xcnt; running.subxcnt = xlrec->subxcnt; running.subxid_status = xlrec->subxid_overflow ? SUBXIDS_MISSING : SUBXIDS_IN_ARRAY; @@ -1277,11 +1285,12 @@ standby_redo(XLogReaderState *record) * as there's no independent knob to just enable logical decoding. For * details of how this is used, check snapbuild.c's introductory comment. * + * If 'dbid' is valid, only gather transactions running in that database. * * Returns the RecPtr of the last inserted record. */ XLogRecPtr -LogStandbySnapshot(void) +LogStandbySnapshot(Oid dbid) { XLogRecPtr recptr; RunningTransactions running; @@ -1314,7 +1323,7 @@ LogStandbySnapshot(void) * Log details of all in-progress transactions. This should be the last * record we write, because standby will open up when it sees this. */ - running = GetRunningTransactionData(); + running = GetRunningTransactionData(dbid); /* * GetRunningTransactionData() acquired ProcArrayLock, we must release it. @@ -1358,6 +1367,7 @@ LogCurrentRunningXacts(RunningTransactions CurrRunningXacts) xl_running_xacts xlrec; XLogRecPtr recptr; + xlrec.dbid = CurrRunningXacts->dbid; xlrec.xcnt = CurrRunningXacts->xcnt; xlrec.subxcnt = CurrRunningXacts->subxcnt; xlrec.subxid_overflow = (CurrRunningXacts->subxid_status != SUBXIDS_IN_ARRAY); diff --git a/src/include/access/genam.h b/src/include/access/genam.h index b69320a7fc8..c9a52277be1 100644 --- a/src/include/access/genam.h +++ b/src/include/access/genam.h @@ -136,6 +136,13 @@ typedef struct IndexOrderByDistance bool isnull; } IndexOrderByDistance; + +/* + * Keep track of whether logical decoding in this backend promised not to + * access shared catalogs, as a safety check. Somewhat misplaced, but ... + */ +extern bool accessSharedCatalogsInDecoding; + /* * generalized index_ interface routines (in indexam.c) */ diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h index 10c18d39ff8..13ae3ad4fbb 100644 --- a/src/include/access/xlog_internal.h +++ b/src/include/access/xlog_internal.h @@ -32,7 +32,7 @@ /* * Each page of XLOG file has a header like this: */ -#define XLOG_PAGE_MAGIC 0xD11E /* can be used as WAL version indicator */ +#define XLOG_PAGE_MAGIC 0xD11F /* can be used as WAL version indicator */ typedef struct XLogPageHeaderData { diff --git a/src/include/replication/output_plugin.h b/src/include/replication/output_plugin.h index 842fcde67f9..917f3cff232 100644 --- a/src/include/replication/output_plugin.h +++ b/src/include/replication/output_plugin.h @@ -27,6 +27,7 @@ typedef struct OutputPluginOptions { OutputPluginOutputType output_type; bool receive_rewrites; + bool need_shared_catalogs; } OutputPluginOptions; /* diff --git a/src/include/replication/snapbuild.h b/src/include/replication/snapbuild.h index a22a83a2f23..d02530a912a 100644 --- a/src/include/replication/snapbuild.h +++ b/src/include/replication/snapbuild.h @@ -92,7 +92,8 @@ extern void SnapBuildProcessNewCid(SnapBuild *builder, TransactionId xid, XLogRecPtr lsn, xl_heap_new_cid *xlrec); extern void SnapBuildProcessRunningXacts(SnapBuild *builder, XLogRecPtr lsn, - xl_running_xacts *running); + xl_running_xacts *running, + bool db_specific); extern void SnapBuildSerializationPoint(SnapBuild *builder, XLogRecPtr lsn); extern bool SnapBuildSnapshotExists(XLogRecPtr lsn); diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h index abdf021e66e..377b3060b9f 100644 --- a/src/include/storage/procarray.h +++ b/src/include/storage/procarray.h @@ -49,7 +49,7 @@ extern bool ProcArrayInstallImportedXmin(TransactionId xmin, VirtualTransactionId *sourcevxid); extern bool ProcArrayInstallRestoredXmin(TransactionId xmin, PGPROC *proc); -extern RunningTransactions GetRunningTransactionData(void); +extern RunningTransactions GetRunningTransactionData(Oid dbid); extern bool TransactionIdIsInProgress(TransactionId xid); extern TransactionId GetOldestNonRemovableTransactionId(Relation rel); diff --git a/src/include/storage/standby.h b/src/include/storage/standby.h index 6a314c693cd..8715c08e94f 100644 --- a/src/include/storage/standby.h +++ b/src/include/storage/standby.h @@ -126,6 +126,7 @@ typedef enum typedef struct RunningTransactionsData { + Oid dbid; /* only track xacts in this database */ int xcnt; /* # of xact ids in xids[] */ int subxcnt; /* # of subxact ids in xids[] */ subxids_array_status subxid_status; @@ -143,7 +144,7 @@ typedef RunningTransactionsData *RunningTransactions; extern void LogAccessExclusiveLock(Oid dbOid, Oid relOid); extern void LogAccessExclusiveLockPrepare(void); -extern XLogRecPtr LogStandbySnapshot(void); +extern XLogRecPtr LogStandbySnapshot(Oid dbid); extern void LogStandbyInvalidations(int nmsgs, SharedInvalidationMessage *msgs, bool relcacheInitFileInval); diff --git a/src/include/storage/standbydefs.h b/src/include/storage/standbydefs.h index 231d251fd51..e75b7078766 100644 --- a/src/include/storage/standbydefs.h +++ b/src/include/storage/standbydefs.h @@ -46,6 +46,7 @@ typedef struct xl_standby_locks */ typedef struct xl_running_xacts { + Oid dbid; /* only track xacts in this database */ int xcnt; /* # of xact ids in xids[] */ int subxcnt; /* # of subxact ids in xids[] */ bool subxid_overflow; /* snapshot overflowed, subxids missing */ -- 2.47.3 --qr3jlalmmcpkiodg Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename="v53-0002-Fix-a-few-problems-in-index-build-progress-repor.patch" ^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2026-04-04 15:16 UTC | newest] Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-12-13 14:42 [PATCH v6 2/4] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH v3 2/2] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH 2/4] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH 1/3] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH v8 2/4] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH v4 1/3] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH 1/3] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH v8 2/4] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH v5 1/3] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH v8 2/4] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH 2/4] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH v6 2/4] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2021-12-13 14:42 [PATCH 2/4] warn when setting GUC to a nonextant library Justin Pryzby <pryzbyj@telsasoft.com> 2026-04-04 15:16 [PATCH v53 1/7] Introduce an option to make logical replication database specific. Antonin Houska <ah@cybertec.at>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox