From 3fc0faad9748971a94c977f1c2833fa4fa22ea69 Mon Sep 17 00:00:00 2001 From: shihao zhong Date: Fri, 30 Oct 2026 23:39:39 -0400 Subject: [PATCH v1 2/2] pg_upgrade: Check stored settings against the new cluster pg_dump and pg_dumpall turn the settings stored for roles, databases and functions into ALTER ROLE/DATABASE ... SET and CREATE FUNCTION ... SET commands, and the privileges on parameters into GRANT ... ON PARAMETER. If the new server rejects one of them, for example because the parameter was removed or the value is no longer accepted, pg_upgrade --check passes and the upgrade then fails while restoring the schema. Collect these settings from the old cluster, and in the new cluster run the same kind of command for each one inside a transaction that is rolled back. Checking the values with set_config() instead would be too strict. When restoring, the server only warns about values that refer to objects that do not exist yet, like a tablespace or a text search configuration, and the new cluster is still empty at this point. Since commit 69d5ca484b6, the check hooks themselves decide which of their checks depend on the database state, so running the real command lets them make that call. Only settings that are actually dumped are checked. The entry made by ALTER ROLE ALL SET, role-wide settings of predefined roles, and settings of template0 are skipped. Discussion: https://postgr.es/m/CAGRkXqT+5+mQhoKZFTiM4yOZtVV=NW506vDwFtVeG+VmL_W5wQ@mail.gmail.com --- src/bin/pg_upgrade/Makefile | 1 + src/bin/pg_upgrade/check.c | 4 + src/bin/pg_upgrade/meson.build | 2 + src/bin/pg_upgrade/nls.mk | 1 + src/bin/pg_upgrade/pg_upgrade.h | 5 + src/bin/pg_upgrade/settings.c | 304 ++++++++++++++++++++ src/bin/pg_upgrade/t/009_stored_settings.pl | 189 ++++++++++++ src/tools/pgindent/typedefs.list | 2 + 8 files changed, 508 insertions(+) create mode 100644 src/bin/pg_upgrade/settings.c create mode 100644 src/bin/pg_upgrade/t/009_stored_settings.pl diff --git a/src/bin/pg_upgrade/Makefile b/src/bin/pg_upgrade/Makefile index 771addb675a..56b86875370 100644 --- a/src/bin/pg_upgrade/Makefile +++ b/src/bin/pg_upgrade/Makefile @@ -25,6 +25,7 @@ OBJS = \ pg_upgrade.o \ relfilenumber.o \ server.o \ + settings.o \ slru_io.o \ tablespace.o \ task.o \ diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 2d4aaf22a0f..e8b407611a3 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -601,6 +601,8 @@ check_and_dump_old_cluster(void) get_loadable_libraries(); + get_stored_settings(); + /* * Check for various failure cases @@ -709,6 +711,8 @@ check_new_cluster(void) check_loadable_libraries(); + check_stored_settings(); + switch (user_opts.transfer_mode) { case TRANSFER_MODE_CLONE: diff --git a/src/bin/pg_upgrade/meson.build b/src/bin/pg_upgrade/meson.build index ffbf6ae8d75..f79a6bcd43b 100644 --- a/src/bin/pg_upgrade/meson.build +++ b/src/bin/pg_upgrade/meson.build @@ -15,6 +15,7 @@ pg_upgrade_sources = files( 'pg_upgrade.c', 'relfilenumber.c', 'server.c', + 'settings.c', 'slru_io.c', 'tablespace.c', 'task.c', @@ -69,6 +70,7 @@ tests += { 't/006_transfer_modes.pl', 't/007_multixact_conversion.pl', 't/008_extension_control_path.pl', + 't/009_stored_settings.pl', ], 'deps': [test_ext], 'test_kwargs': {'priority': 40}, # pg_upgrade tests are slow diff --git a/src/bin/pg_upgrade/nls.mk b/src/bin/pg_upgrade/nls.mk index 10ea17c9963..af49b9e02db 100644 --- a/src/bin/pg_upgrade/nls.mk +++ b/src/bin/pg_upgrade/nls.mk @@ -14,6 +14,7 @@ GETTEXT_FILES = check.c \ pg_upgrade.c \ relfilenumber.c \ server.c \ + settings.c \ slru_io.c \ tablespace.c \ task.c \ diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h index 30a2e263d2d..cf497fffd00 100644 --- a/src/bin/pg_upgrade/pg_upgrade.h +++ b/src/bin/pg_upgrade/pg_upgrade.h @@ -442,6 +442,11 @@ void transfer_all_new_dbs(DbInfoArr *old_db_arr, DbInfoArr *new_db_arr, char *old_pgdata, char *new_pgdata, char *old_tablespace, char *new_tablespace); +/* settings.c */ + +void get_stored_settings(void); +void check_stored_settings(void); + /* tablespace.c */ void init_tablespaces(void); diff --git a/src/bin/pg_upgrade/settings.c b/src/bin/pg_upgrade/settings.c new file mode 100644 index 00000000000..47697565868 --- /dev/null +++ b/src/bin/pg_upgrade/settings.c @@ -0,0 +1,304 @@ +/* + * settings.c + * + * checks for stored configuration settings + * + * Copyright (c) 2026, PostgreSQL Global Development Group + * src/bin/pg_upgrade/settings.c + */ + +#include "postgres_fe.h" + +#include "access/transam.h" +#include "fe_utils/string_utils.h" +#include "pg_upgrade.h" + +/* + * item is a "name=value" string from setconfig or proconfig, or a parameter + * name from pg_parameter_acl. object says where it is stored. + */ +typedef struct +{ + char *item; + char *object; +} StoredSetting; + +typedef struct +{ + StoredSetting *items; + int nitems; + int maxitems; +} StoredSettingArr; + +static StoredSettingArr config_settings; +static StoredSettingArr param_privileges; + +static void +add_stored_setting(StoredSettingArr *arr, const char *item, char *object) +{ + if (arr->nitems >= arr->maxitems) + { + arr->maxitems = Max(arr->maxitems * 2, 16); + arr->items = pg_realloc_array(arr->items, StoredSetting, arr->maxitems); + } + arr->items[arr->nitems].item = pg_strdup(item); + arr->items[arr->nitems].object = object; + arr->nitems++; +} + +/* Sort by item, then object, so that each item is probed once */ +static int +stored_setting_compare(const void *p1, const void *p2) +{ + const StoredSetting *s1 = (const StoredSetting *) p1; + const StoredSetting *s2 = (const StoredSetting *) p2; + int cmp = strcmp(s1->item, s2->item); + + if (cmp != 0) + return cmp; + return strcmp(s1->object, s2->object); +} + +/* Collect the function settings of one database */ +static void +process_function_settings(DbInfo *dbinfo, PGresult *res, void *arg) +{ + int ntups = PQntuples(res); + int i_proname = PQfnumber(res, "proname"); + int i_setting = PQfnumber(res, "setting"); + + for (int rowno = 0; rowno < ntups; rowno++) + add_stored_setting(&config_settings, + PQgetvalue(res, rowno, i_setting), + psprintf(_("function %s in database \"%s\""), + PQgetvalue(res, rowno, i_proname), + dbinfo->db_name)); +} + +/* + * get_stored_settings() + * + * Collect the role, database and function settings and the parameter + * privileges of the old cluster, for check_stored_settings(). + */ +void +get_stored_settings(void) +{ + PGconn *conn = connectToServer(&old_cluster, "template1"); + PGresult *res; + UpgradeTask *task; + char *query; + + /* + * Role and database settings. Skip what is not dumped: ALTER ROLE ALL, + * role-wide settings of pg_ roles, and template0. + */ + res = executeQueryOrDie(conn, + "SELECT s.setting, r.rolname, d.datname " + "FROM (SELECT setrole, setdatabase, " + " pg_catalog.unnest(setconfig) AS setting " + " FROM pg_catalog.pg_db_role_setting) s " + "LEFT JOIN pg_catalog.pg_authid r ON r.oid = s.setrole " + "LEFT JOIN pg_catalog.pg_database d ON d.oid = s.setdatabase " + "WHERE CASE WHEN s.setdatabase = 0 " + " THEN s.setrole <> 0 AND r.rolname !~ '^pg_' " + " ELSE d.datname <> 'template0' END"); + + for (int rowno = 0; rowno < PQntuples(res); rowno++) + { + const char *setting = PQgetvalue(res, rowno, 0); + const char *rolname = PQgetvalue(res, rowno, 1); + const char *datname = PQgetvalue(res, rowno, 2); + char *object; + + if (PQgetisnull(res, rowno, 2)) + object = psprintf(_("role \"%s\""), rolname); + else if (PQgetisnull(res, rowno, 1)) + object = psprintf(_("database \"%s\""), datname); + else + object = psprintf(_("role \"%s\" in database \"%s\""), + rolname, datname); + + add_stored_setting(&config_settings, setting, object); + } + PQclear(res); + + /* Privileges on parameters, new in PostgreSQL 15 */ + if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1500) + { + res = executeQueryOrDie(conn, + "SELECT parname FROM pg_catalog.pg_parameter_acl"); + + for (int rowno = 0; rowno < PQntuples(res); rowno++) + add_stored_setting(¶m_privileges, + PQgetvalue(res, rowno, 0), + psprintf(_("privileges on parameter \"%s\""), + PQgetvalue(res, rowno, 0))); + PQclear(res); + } + + PQfinish(conn); + + /* Function settings, in every database */ + task = upgrade_task_create(); + query = psprintf("SELECT p.oid::pg_catalog.regprocedure AS proname, " + " pg_catalog.unnest(p.proconfig) AS setting " + "FROM pg_catalog.pg_proc p " + "WHERE p.proconfig IS NOT NULL AND " + " p.oid >= %u", + FirstNormalObjectId); + upgrade_task_add_step(task, query, process_function_settings, true, NULL); + upgrade_task_run(task, &old_cluster); + upgrade_task_free(task); + pfree(query); +} + +/* + * Run command in a savepoint and roll it back. Return the error message, or + * NULL on success. + */ +static char * +probe_command(PGconn *conn, const char *command) +{ + PGresult *res; + char *errmsg = NULL; + + PQclear(executeQueryOrDie(conn, "SAVEPOINT probe")); + + res = PQexec(conn, command); + if (PQresultStatus(res) != PGRES_COMMAND_OK) + { + const char *msg = PQresultErrorField(res, PG_DIAG_MESSAGE_PRIMARY); + + errmsg = pg_strdup(msg ? msg : PQerrorMessage(conn)); + } + PQclear(res); + + PQclear(executeQueryOrDie(conn, "ROLLBACK TO SAVEPOINT probe")); + + return errmsg; +} + +/* + * Probe each distinct item and report the objects whose item fails. The + * report file is opened on first use and returned. + */ +static FILE * +probe_stored_settings(PGconn *conn, StoredSettingArr *arr, + void (*build_command) (PGconn *conn, const char *item, + PQExpBuffer buf), + bool show_item, FILE *script, const char *output_path) +{ + PQExpBuffer cmd = createPQExpBuffer(); + char *errmsg = NULL; + + qsort(arr->items, arr->nitems, sizeof(StoredSetting), + stored_setting_compare); + + for (int i = 0; i < arr->nitems; i++) + { + StoredSetting *s = &arr->items[i]; + + /* New item, probe it */ + if (i == 0 || strcmp(s->item, arr->items[i - 1].item) != 0) + { + pg_free(errmsg); + errmsg = NULL; + + resetPQExpBuffer(cmd); + build_command(conn, s->item, cmd); + if (cmd->len > 0) + errmsg = probe_command(conn, cmd->data); + } + + if (errmsg) + { + if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL) + pg_fatal("could not open file \"%s\": %m", output_path); + if (show_item) + fprintf(script, "%s: %s: %s\n", s->object, s->item, errmsg); + else + fprintf(script, "%s: %s\n", s->object, errmsg); + } + } + + pg_free(errmsg); + destroyPQExpBuffer(cmd); + + return script; +} + +/* + * Build the ALTER ROLE ... SET that pg_dump would emit. Database and + * function settings are validated the same way, so this covers them too. + */ +static void +build_setting_command(PGconn *conn, const char *item, PQExpBuffer buf) +{ + makeAlterConfigCommand(conn, item, "ROLE", os_info.user, NULL, NULL, buf); +} + +/* GRANT SET fails if the new server does not know the parameter */ +static void +build_privilege_command(PGconn *conn, const char *item, PQExpBuffer buf) +{ + appendPQExpBuffer(buf, "GRANT SET ON PARAMETER %s TO PUBLIC", fmtId(item)); +} + +/* + * check_stored_settings() + * + * Run the restore's commands in the new cluster and roll them back. + * set_config() would be too strict, since the restore only warns about + * objects that do not exist yet, like a tablespace. + */ +void +check_stored_settings(void) +{ + PGconn *conn; + FILE *script = NULL; + char output_path[MAXPGPATH]; + + prep_status("Checking for stored settings the new cluster rejects"); + + if (config_settings.nitems == 0 && param_privileges.nitems == 0) + { + check_ok(); + return; + } + + snprintf(output_path, sizeof(output_path), "%s/%s", + log_opts.basedir, "stored_settings.txt"); + + conn = connectToServer(&new_cluster, "template1"); + setFmtEncoding(PQclientEncoding(conn)); + + /* Hide the notices about objects that are not restored yet */ + PQclear(executeQueryOrDie(conn, "SET client_min_messages = warning")); + PQclear(executeQueryOrDie(conn, "BEGIN")); + + script = probe_stored_settings(conn, &config_settings, + build_setting_command, + true, script, output_path); + script = probe_stored_settings(conn, ¶m_privileges, + build_privilege_command, + false, script, output_path); + + PQclear(executeQueryOrDie(conn, "ROLLBACK")); + PQfinish(conn); + + if (script) + { + fclose(script); + pg_log(PG_REPORT, "fatal"); + pg_fatal("Your installation contains roles, databases, functions, or parameter\n" + "privileges with stored settings that the new cluster does not accept,\n" + "so restoring them would fail. Remove or change these settings in the\n" + "old cluster, for example with ALTER ROLE ... RESET or REVOKE ... ON\n" + "PARAMETER, and restart the upgrade. A list of the problem settings is\n" + "in the file:\n" + " %s", output_path); + } + else + check_ok(); +} diff --git a/src/bin/pg_upgrade/t/009_stored_settings.pl b/src/bin/pg_upgrade/t/009_stored_settings.pl new file mode 100644 index 00000000000..22002b4e94d --- /dev/null +++ b/src/bin/pg_upgrade/t/009_stored_settings.pl @@ -0,0 +1,189 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Test that pg_upgrade --check reports stored settings the new cluster +# rejects. + +use strict; +use warnings FATAL => 'all'; + +use File::Find qw(find); +use File::Path qw(rmtree); +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $old = + PostgreSQL::Test::Cluster->new('old', install_path => $ENV{oldinstall}); +$old->init; +$old->start; + +my $have_param_acl = $old->pg_version >= 15; + +# Settings that restore with only a warning about missing objects. +$old->safe_psql( + 'postgres', q{ + CREATE ROLE r_ok; + ALTER ROLE r_ok SET default_tablespace = 'no_such_tablespace'; + ALTER ROLE r_ok SET myext.foo = 'bar'; + CREATE DATABASE d_ok; + CREATE FUNCTION f_ok() RETURNS int LANGUAGE sql + SET work_mem = '2MB' SET search_path = public, pg_temp + RETURN 1; + CREATE FUNCTION f_bad() RETURNS int LANGUAGE sql RETURN 1; + CREATE ROLE r_bad; + CREATE DATABASE d_bad; +}); +$old->safe_psql( + 'd_ok', q{ + CREATE TEXT SEARCH CONFIGURATION public.mycfg (COPY = english); + ALTER DATABASE d_ok SET default_text_search_config = 'public.mycfg'; +}); + +# Bad settings, put straight into the catalogs. The last three are not +# dumped and must not be reported. +$old->safe_psql( + 'postgres', q{ + SET allow_system_table_mods = on; + INSERT INTO pg_db_role_setting VALUES + (0, 'r_bad'::regrole, '{no_such_setting=on}'), + ((SELECT oid FROM pg_database WHERE datname = 'd_bad'), 0, + '{standard_conforming_strings=off}'), + ((SELECT oid FROM pg_database WHERE datname = 'd_bad'), 'r_bad'::regrole, + '{default_toast_compression=no_such_method}'), + (0, 0, '{no_such_setting_all=on}'), + (0, 'pg_monitor'::regrole, '{no_such_setting_predefined=on}'), + ((SELECT oid FROM pg_database WHERE datname = 'template0'), 0, + '{no_such_setting_template0=on}'); + UPDATE pg_proc SET proconfig = '{standard_conforming_strings=off}' + WHERE proname = 'f_bad'; +}); + +if ($have_param_acl) +{ + $old->safe_psql( + 'postgres', q{ + GRANT SET ON PARAMETER work_mem TO r_ok; + GRANT SET ON PARAMETER myext.removed TO r_ok; + SET allow_system_table_mods = on; + UPDATE pg_parameter_acl SET parname = 'no_such_builtin' + WHERE parname = 'myext.removed'; + }); +} + +$old->stop; + +my $new = PostgreSQL::Test::Cluster->new('new'); +$new->init; + +# In a VPATH build, we'll be started in the source directory, but we want +# to run pg_upgrade in the build directory so that any files generated finish +# in it, like delete_old_cluster.{sh,bat}. +chdir ${PostgreSQL::Test::Utils::tmp_check}; + +my @pg_upgrade_cmd = ( + 'pg_upgrade', '--no-sync', + '--old-datadir' => $old->data_dir, + '--new-datadir' => $new->data_dir, + '--old-bindir' => $old->config_data('--bindir'), + '--new-bindir' => $new->config_data('--bindir'), + '--socketdir' => $new->host, + '--old-port' => $old->port, + '--new-port' => $new->port, + '--copy'); + +command_checks_all( + [ @pg_upgrade_cmd, '--check' ], + 1, + [ + qr/Checking for stored settings the new cluster rejects\s+fatal/, + qr/stored settings that the new cluster does not accept/, + ], + [qr//], + 'pg_upgrade --check fails on stored settings the new cluster rejects'); + +my $report; +find( + sub { + $report = $File::Find::name + if $File::Find::name =~ m/stored_settings\.txt/; + }, + $new->data_dir . "/pg_upgrade_output.d"); +my $content = slurp_file($report); + +like( + $content, + qr/^role "r_bad": no_such_setting=on: unrecognized configuration parameter "no_such_setting"$/m, + 'unknown parameter in role setting is reported'); +like( + $content, + qr/^database "d_bad": standard_conforming_strings=off: /m, + 'rejected value in database setting is reported'); +like( + $content, + qr/^role "r_bad" in database "d_bad": default_toast_compression=no_such_method: invalid value/m, + 'invalid value in role setting in database is reported'); +like( + $content, + qr/^function public\.f_bad\(\) in database "postgres": standard_conforming_strings=off: /m, + 'rejected value in function setting is reported'); +SKIP: +{ + skip "parameter privileges need PostgreSQL 15 or later", 1 + unless $have_param_acl; + like( + $content, + qr/^privileges on parameter "no_such_builtin": unrecognized configuration parameter/m, + 'privileges on unknown parameter are reported'); +} +unlike($content, qr/r_ok|d_ok|f_ok|work_mem|myext/, + 'settings that restore cleanly are not reported'); +unlike($content, qr/no_such_setting_/, 'settings not dumped are not reported'); + +rmtree($new->data_dir . "/pg_upgrade_output.d"); + +# Remove the bad settings and upgrade. +$old->start; +$old->safe_psql( + 'postgres', q{ + SET allow_system_table_mods = on; + DELETE FROM pg_db_role_setting + WHERE setrole = 'r_bad'::regrole + OR setdatabase = (SELECT oid FROM pg_database WHERE datname = 'd_bad'); + UPDATE pg_proc SET proconfig = NULL WHERE proname = 'f_bad'; +}); +$old->safe_psql('postgres', + q{REVOKE SET ON PARAMETER no_such_builtin FROM r_ok}) + if $have_param_acl; +$old->stop; + +command_ok(\@pg_upgrade_cmd, + 'pg_upgrade succeeds once the rejected settings are removed'); + +$new->start; +is( $new->safe_psql( + 'postgres', q{ + SELECT coalesce(r.rolname, '-') || ' ' || coalesce(d.datname, '-') || ' ' || + array_to_string(s.setconfig, ',') + FROM pg_db_role_setting s + LEFT JOIN pg_roles r ON r.oid = s.setrole + LEFT JOIN pg_database d ON d.oid = s.setdatabase + ORDER BY 1}), + qq{- d_ok default_text_search_config=public.mycfg +r_ok - default_tablespace=no_such_tablespace,myext.foo=bar}, + 'role and database settings are kept'); +is( $new->safe_psql( + 'postgres', + q{SELECT array_to_string(proconfig, ',') FROM pg_proc WHERE proname = 'f_ok'} + ), + 'work_mem=2MB,search_path=public, pg_temp', + 'function settings are kept'); +if ($have_param_acl) +{ + is( $new->safe_psql( + 'postgres', q{SELECT parname FROM pg_parameter_acl ORDER BY 1}), + 'work_mem', + 'privileges on parameters are kept'); +} +$new->stop; + +done_testing(); diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 553162c0810..e163d660dcd 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3001,6 +3001,8 @@ StdRdOptToastValueType StdRdOptions Step StopList +StoredSetting +StoredSettingArr StrategyNumber StreamCtl StreamStopReason -- 2.37.1 (Apple Git-137.1)