public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] do only critical work during single-user vacuum? 4+ messages / 3 participants [nested] [flat]
* [PATCH] do only critical work during single-user vacuum? @ 2022-01-21 22:41 John Naylor <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: John Naylor @ 2022-01-21 22:41 UTC (permalink / raw) Jan 21 John Naylor --- src/backend/commands/vacuum.c | 76 +++++++++++++++++++++++++++++++++-- src/include/commands/vacuum.h | 1 + 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index d1dadc54e47..c7bc97d3f76 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -52,6 +52,7 @@ #include "storage/proc.h" #include "storage/procarray.h" #include "utils/acl.h" +#include "utils/builtins.h" #include "utils/fmgroids.h" #include "utils/guc.h" #include "utils/memutils.h" @@ -114,6 +115,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) bool full = false; bool disable_page_skipping = false; bool process_toast = true; + bool wraparound = false; ListCell *lc; /* index_cleanup and truncate values unspecified for now */ @@ -200,6 +202,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) params.nworkers = nworkers; } } + else if (strcmp(opt->defname, "wraparound") == 0) + wraparound = defGetBoolean(opt); else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -246,17 +250,51 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) } } + if (wraparound) + { + /* exclude incompatible options */ + foreach(lc, vacstmt->options) + { + DefElem *opt = (DefElem *) lfirst(lc); + + // WIP is there a better way? + if (strcmp(opt->defname, "wraparound") != 0 && + strcmp(opt->defname, "verbose") != 0 && + defGetBoolean(opt)) + + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("option \"%s\" is incompatible with WRAPAROUND", opt->defname), + parser_errposition(pstate, opt->location))); + } + + /* skip unnecessary work, as in failsafe mode */ + params.index_cleanup = VACOPTVALUE_DISABLED; + params.truncate = VACOPTVALUE_DISABLED; + } + /* - * All freeze ages are zero if the FREEZE option is given; otherwise pass - * them as -1 which means to use the default values. + * Set freeze ages to zero where appropriate; otherwise pass + * them as -1 which means to use the configured values. */ if (params.options & VACOPT_FREEZE) { + /* All freeze ages are zero if the FREEZE option is given */ params.freeze_min_age = 0; params.freeze_table_age = 0; params.multixact_freeze_min_age = 0; params.multixact_freeze_table_age = 0; } + else if (params.options & VACOPT_MINIMAL) + { + /* it's unlikely any selected table will not be eligible for aggressive vacuum, but make sure */ + params.freeze_table_age = 0; + params.multixact_freeze_table_age = 0; + + // WIP: It might be worth trying to do less work here, or at least hard-coding the default values + params.freeze_min_age = -1; + params.multixact_freeze_min_age = -1; + } else { params.freeze_min_age = -1; @@ -894,6 +932,8 @@ get_all_vacuum_rels(int options) Relation pgclass; TableScanDesc scan; HeapTuple tuple; + int32 table_xid_age, + table_mxid_age; pgclass = table_open(RelationRelationId, AccessShareLock); @@ -909,12 +949,42 @@ get_all_vacuum_rels(int options) if (!vacuum_is_relation_owner(relid, classForm, options)) continue; + if (options & VACOPT_MINIMAL) + { + /* + * Only consider relations able to hold unfrozen XIDs (anything else + * should have InvalidTransactionId in relfrozenxid anyway). + */ + if (classForm->relkind != RELKIND_RELATION && + classForm->relkind != RELKIND_MATVIEW && + classForm->relkind != RELKIND_TOASTVALUE) + { + Assert(!TransactionIdIsValid(classForm->relfrozenxid)); + Assert(!MultiXactIdIsValid(classForm->relminmxid)); + continue; + } + + table_xid_age = DirectFunctionCall1(xid_age, classForm->relfrozenxid); + table_mxid_age = DirectFunctionCall1(mxid_age, classForm->relminmxid); + + /* Hard-code 1 billion for the thresholds to avoid making assumptions + * about the configuration. This leaves some headroom for when the user + * returns to normal mode while also minimizing work. + * WIP: consider passing these constants via the params struct + */ + // FIXME to speed up testing + // if ((table_xid_age < 1000 * 1000 * 1000) && + // (table_mxid_age < 1000 * 1000 * 1000)) + if ((table_xid_age < 1000 * 1000) && + (table_mxid_age < 1000 * 1000)) + continue; + } /* * We include partitioned tables here; depending on which operation is * to be performed, caller will decide whether to process or ignore * them. */ - if (classForm->relkind != RELKIND_RELATION && + else if (classForm->relkind != RELKIND_RELATION && classForm->relkind != RELKIND_MATVIEW && classForm->relkind != RELKIND_PARTITIONED_TABLE) continue; diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 5d0bdfa4279..3d8b8fbbb1a 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -188,6 +188,7 @@ typedef struct VacAttrStats #define VACOPT_SKIP_LOCKED 0x20 /* skip if cannot get lock */ #define VACOPT_PROCESS_TOAST 0x40 /* process the TOAST table, if any */ #define VACOPT_DISABLE_PAGE_SKIPPING 0x80 /* don't skip any pages */ +#define VACOPT_MINIMAL 0x100 /* do minimal freezing work to prevent or get out of shutdown */ /* * Values used by index_cleanup and truncate params. -- 2.17.1 --CtqPPqYpYc7tL2y7-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* [PATCH 1/3] do only critical work during single-user vacuum? @ 2022-02-01 21:50 John Naylor <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: John Naylor @ 2022-02-01 21:50 UTC (permalink / raw) Feb 01 John Naylor --- doc/src/sgml/maintenance.sgml | 12 ++-- doc/src/sgml/ref/vacuum.sgml | 22 ++++++ src/backend/access/transam/varsup.c | 4 +- src/backend/commands/vacuum.c | 107 +++++++++++++++++++++++++--- src/include/commands/vacuum.h | 5 ++ 5 files changed, 134 insertions(+), 16 deletions(-) diff --git a/doc/src/sgml/maintenance.sgml b/doc/src/sgml/maintenance.sgml index 36f975b1e5b..5c360499504 100644 --- a/doc/src/sgml/maintenance.sgml +++ b/doc/src/sgml/maintenance.sgml @@ -629,17 +629,19 @@ HINT: To avoid a database shutdown, execute a database-wide VACUUM in that data <programlisting> ERROR: database is not accepting commands to avoid wraparound data loss in database "mydb" -HINT: Stop the postmaster and vacuum that database in single-user mode. +HINT: Stop the postmaster and run "VACUUM (EMERGENCY)" in that database in single-user mode. </programlisting> The three-million-transaction safety margin exists to let the administrator recover without data loss, by manually executing the - required <command>VACUUM</command> commands. However, since the system will not + required <command>VACUUM</command> command. However, since the system will not execute commands once it has gone into the safety shutdown mode, the only way to do this is to stop the server and start the server in single-user - mode to execute <command>VACUUM</command>. The shutdown mode is not enforced - in single-user mode. See the <xref linkend="app-postgres"/> reference - page for details about using single-user mode. + mode to execute <command>VACUUM (EMERGENCY)</command>. The <literal>EMERGENCY</literal> option + is recommended, since it enables the vacuum to complete as quickly as possible + while still leaving a safety margin for when the system comes back online again. + The shutdown mode is not enforced in single-user mode. + See the <xref linkend="app-postgres"/> reference page for details about using single-user mode. </para> <sect3 id="vacuum-for-multixact-wraparound"> diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 3df32b58ee6..2dab01ff376 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -295,6 +295,28 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet </listitem> </varlistentry> + <varlistentry> + <term><literal>EMERGENCY</literal></term> + <listitem> + <para> + Special vacuum mode intended for when a forced shutdown has happened to avoid transaction ID wraparound. + It performs a database-wide vacuum on tables, toast tables, and materialized views whose + xid age or mxid age is older than 1 billion. To complete as quickly as possible, an emergency + vacuum will skip truncation and index cleanup, and will skip toast tables whose age has not + exceeded the cutoff. + </para> + + <para> + While this option could be used while the postmaster is running, it is expected that the wraparound + failsafe mechanism will automatically work in the same way to prevent imminent shutdown. + When <literal>EMERGENCY</literal> is specified no tables may be listed, since it is designed to + select candidate relations from the entire database. + The only other option that may be combined with <literal>VERBOSE</literal>, although in single-user mode no client messages are + output. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><replaceable class="parameter">boolean</replaceable></term> <listitem> diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c index 748120a0125..ee9d33dba37 100644 --- a/src/backend/access/transam/varsup.c +++ b/src/backend/access/transam/varsup.c @@ -128,14 +128,14 @@ GetNewTransactionId(bool isSubXact) (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("database is not accepting commands to avoid wraparound data loss in database \"%s\"", oldest_datname), - errhint("Stop the postmaster and vacuum that database in single-user mode.\n" + errhint("Stop the postmaster and run \"VACUUM (EMERGENCY)\" in that database in single-user mode.\n" "You might also need to commit or roll back old prepared transactions, or drop stale replication slots."))); else ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("database is not accepting commands to avoid wraparound data loss in database with OID %u", oldest_datoid), - errhint("Stop the postmaster and vacuum that database in single-user mode.\n" + errhint("Stop the postmaster and run \"VACUUM (EMERGENCY)\" in that database in single-user mode.\n" "You might also need to commit or roll back old prepared transactions, or drop stale replication slots."))); } else if (TransactionIdFollowsOrEquals(xid, xidWarnLimit)) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index d1dadc54e47..e36159e827f 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -52,6 +52,7 @@ #include "storage/proc.h" #include "storage/procarray.h" #include "utils/acl.h" +#include "utils/builtins.h" #include "utils/fmgroids.h" #include "utils/guc.h" #include "utils/memutils.h" @@ -86,7 +87,7 @@ int VacuumCostBalanceLocal = 0; /* non-export function prototypes */ static List *expand_vacuum_rel(VacuumRelation *vrel, int options); -static List *get_all_vacuum_rels(int options); +static List *get_all_vacuum_rels(VacuumParams *params); static void vac_truncate_clog(TransactionId frozenXID, MultiXactId minMulti, TransactionId lastSaneFrozenXid, @@ -114,6 +115,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) bool full = false; bool disable_page_skipping = false; bool process_toast = true; + bool emergency = false; ListCell *lc; /* index_cleanup and truncate values unspecified for now */ @@ -123,6 +125,10 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) /* By default parallel vacuum is enabled */ params.nworkers = 0; + /* By default don't skip any tables */ + params.min_xid_age = 0; + params.min_mxid_age = 0; + /* Parse options list */ foreach(lc, vacstmt->options) { @@ -200,6 +206,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) params.nworkers = nworkers; } } + else if (strcmp(opt->defname, "emergency") == 0) + emergency = defGetBoolean(opt); else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -216,7 +224,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) (freeze ? VACOPT_FREEZE : 0) | (full ? VACOPT_FULL : 0) | (disable_page_skipping ? VACOPT_DISABLE_PAGE_SKIPPING : 0) | - (process_toast ? VACOPT_PROCESS_TOAST : 0); + (process_toast ? VACOPT_PROCESS_TOAST : 0) | + (emergency ? VACOPT_EMERGENCY : 0); /* sanity checks on options */ Assert(params.options & (VACOPT_VACUUM | VACOPT_ANALYZE)); @@ -246,17 +255,70 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) } } + if (emergency) + { + /* exclude incompatible options */ + foreach(lc, vacstmt->options) + { + DefElem *opt = (DefElem *) lfirst(lc); + + if (strcmp(opt->defname, "emergency") != 0 && + strcmp(opt->defname, "verbose") != 0 && + defGetBoolean(opt)) + + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("option \"%s\" is incompatible with EMERGENCY", opt->defname), + parser_errposition(pstate, opt->location))); + } + + /* prevent specifying a list of tables, to keep it simple */ + if (vacstmt->rels != NULL) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("a relation list is not supported with the EMERGENCY option"))); + + /* skip unnecessary work, similar to failsafe mode */ + + params.index_cleanup = VACOPTVALUE_DISABLED; + params.truncate = VACOPTVALUE_DISABLED; + + /* Hard-code 1 billion for the thresholds to avoid making assumptions + * about the configuration. This leaves some headroom for when the user + * returns to normal mode while also minimizing work. + * WIP: consider passing these constants via the params struct + */ + // params.min_xid_age = 1000 * 1000 * 1000; + // params.min_mxid_age = 1000 * 1000 * 1000; + // FIXME to speed up testing + params.min_xid_age = 1000 * 1000; + params.min_mxid_age = 1000 * 1000; + } + /* - * All freeze ages are zero if the FREEZE option is given; otherwise pass - * them as -1 which means to use the default values. + * Set freeze ages to zero where appropriate; otherwise pass + * them as -1 which means to use the configured values. */ if (params.options & VACOPT_FREEZE) { + /* All freeze ages are zero if the FREEZE option is given */ params.freeze_min_age = 0; params.freeze_table_age = 0; params.multixact_freeze_min_age = 0; params.multixact_freeze_table_age = 0; } + else if (params.options & VACOPT_EMERGENCY) + { + /* It's highly likely any table selected will be eligible for aggressive vacuum, but make sure */ + params.freeze_table_age = 0; + params.multixact_freeze_table_age = 0; + + // WIP: It might be worth trying to do less work here, such as max age / 2 : + // params.freeze_min_age = 100 * 1000 * 1000; + // params.multixact_freeze_min_age = 200 * 1000 * 1000; + params.freeze_min_age = -1; + params.multixact_freeze_min_age = -1; + } else { params.freeze_min_age = -1; @@ -404,7 +466,7 @@ vacuum(List *relations, VacuumParams *params, relations = newrels; } else - relations = get_all_vacuum_rels(params->options); + relations = get_all_vacuum_rels(params); /* * Decide whether we need to start/commit our own transactions. @@ -461,7 +523,10 @@ vacuum(List *relations, VacuumParams *params, ListCell *cur; in_vacuum = true; - VacuumCostActive = (VacuumCostDelay > 0); + if (params->VACOPT_EMERGENCY) + VacuumCostActive = false; + else + VacuumCostActive = (VacuumCostDelay > 0); VacuumCostBalance = 0; VacuumPageHit = 0; VacuumPageMiss = 0; @@ -888,12 +953,14 @@ expand_vacuum_rel(VacuumRelation *vrel, int options) * the current database. The list is built in vac_context. */ static List * -get_all_vacuum_rels(int options) +get_all_vacuum_rels(VacuumParams *params) { List *vacrels = NIL; Relation pgclass; TableScanDesc scan; HeapTuple tuple; + int32 table_xid_age, + table_mxid_age; pgclass = table_open(RelationRelationId, AccessShareLock); @@ -906,15 +973,37 @@ get_all_vacuum_rels(int options) Oid relid = classForm->oid; /* check permissions of relation */ - if (!vacuum_is_relation_owner(relid, classForm, options)) + if (!vacuum_is_relation_owner(relid, classForm, params->options)) continue; + if (params->options & VACOPT_EMERGENCY) + { + /* + * Only consider relations able to hold unfrozen XIDs (anything else + * should have InvalidTransactionId in relfrozenxid anyway). + */ + if (classForm->relkind != RELKIND_RELATION && + classForm->relkind != RELKIND_MATVIEW && + classForm->relkind != RELKIND_TOASTVALUE) + { + Assert(!TransactionIdIsValid(classForm->relfrozenxid)); + Assert(!MultiXactIdIsValid(classForm->relminmxid)); + continue; + } + + table_xid_age = DirectFunctionCall1(xid_age, classForm->relfrozenxid); + table_mxid_age = DirectFunctionCall1(mxid_age, classForm->relminmxid); + + if ((table_xid_age < params->min_xid_age) && + (table_mxid_age < params->min_mxid_age)) + continue; + } /* * We include partitioned tables here; depending on which operation is * to be performed, caller will decide whether to process or ignore * them. */ - if (classForm->relkind != RELKIND_RELATION && + else if (classForm->relkind != RELKIND_RELATION && classForm->relkind != RELKIND_MATVIEW && classForm->relkind != RELKIND_PARTITIONED_TABLE) continue; diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 5d0bdfa4279..8aab6641628 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -188,6 +188,7 @@ typedef struct VacAttrStats #define VACOPT_SKIP_LOCKED 0x20 /* skip if cannot get lock */ #define VACOPT_PROCESS_TOAST 0x40 /* process the TOAST table, if any */ #define VACOPT_DISABLE_PAGE_SKIPPING 0x80 /* don't skip any pages */ +#define VACOPT_EMERGENCY 0x100 /* do minimal freezing work to prevent or get out of shutdown */ /* * Values used by index_cleanup and truncate params. @@ -233,6 +234,10 @@ typedef struct VacuumParams * disabled. */ int nworkers; + + /* cutoff ages for selecting tables to vacuum, 0 is the default */ + int min_xid_age; + int min_mxid_age; } VacuumParams; /* -- 2.17.1 --+mVf78ABtUsDvBix Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0002-fix-compile-error.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Fix search_path for all maintenance commands @ 2023-05-26 23:21 Jeff Davis <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Jeff Davis @ 2023-05-26 23:21 UTC (permalink / raw) To: pgsql-hackers; +Cc: Nathan Bossart <[email protected]> Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, REINDEX, and VACUUM) currently run as the table owner, and as a SECURITY_RESTRICTED_OPERATION. I propose that we also fix the search_path to "pg_catalog, pg_temp" when running maintenance commands, for two reasons: 1. Make the behavior of maintenance commands more consistent because they'd always have the same search_path. 2. Now that we have the MAINTAIN privilege in 16, privileged non- superusers can execute maintenance commands on other users' tables. That raises the possibility that a user with MAINTAIN privilege may be able to use search_path tricks to escalate privileges to the table owner. The MAINTAIN privilege is only given to highly-privileged users, but there's still some risk. For this reason I also propose that it goes in v16. There's one interesting part: in the code path for creating a materialized view, ExecCreateTableAs() has this comment: /* * For materialized views, lock down security-restricted operations and * arrange to make GUC variable changes local to this command. This is * not necessary for security, but this keeps the behavior similar to * REFRESH MATERIALIZED VIEW. Otherwise, one could create a materialized * view not possible to refresh. */ My patch doesn't address this ExecCreateTableAs() check. To do so, we would need to set the search path after DefineRelation(), otherwise it will try to create the object in pg_catalog. But DefineRelation() is happening at execution time, well after we entered the SECURITY_RESTRICTED_OPERATION, and it doesn't seem good to separate the SECURITY_RESTRICTED_OPERATION from setting search_path. This ExecCreateTableAs() check doesn't seem terribly important, so I don't think it's necessary to improve it as a part of this patch (it won't be perfect anyway: functions can behave inconsistently for all kinds of reasons). But I'm open to suggestion if someone knows a good way to do it. -- Jeff Davis PostgreSQL Contributor Team - AWS Attachments: [text/x-patch] 0001-Fix-search_path-to-a-safe-value-during-maintenance-o.patch (16.4K, ../../[email protected]/2-0001-Fix-search_path-to-a-safe-value-during-maintenance-o.patch) download | inline diff: From 5c6d707a88c887641d551ed9a6983c74d6a82a7a Mon Sep 17 00:00:00 2001 From: Jeff Davis <[email protected]> Date: Tue, 18 Apr 2023 10:45:51 -0700 Subject: [PATCH] Fix search_path to a safe value during maintenance operations. While executing maintenance operations (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, REINDEX, or VACUUM), set search_path to 'pg_catalog, pg_temp' to prevent inconsistent behavior. Functions that are used for functional indexes, in index expressions, or in materialized views and depend on a different search path must be declared with CREATE FUNCTION ... SET search_path='...'. This change addresses a small security risk introduced in commit 60684dd834, where a role with MAINTAIN privileges on a table may be able to escalate privileges to the table owner. That commit is not yet part of any release, so no need to backpatch. --- contrib/amcheck/verify_nbtree.c | 2 ++ src/backend/access/brin/brin.c | 2 ++ src/backend/catalog/index.c | 8 ++++++++ src/backend/commands/analyze.c | 2 ++ src/backend/commands/cluster.c | 2 ++ src/backend/commands/indexcmds.c | 6 ++++++ src/backend/commands/matview.c | 2 ++ src/backend/commands/vacuum.c | 2 ++ src/bin/scripts/t/100_vacuumdb.pl | 4 ---- src/include/utils/guc.h | 6 ++++++ .../test_oat_hooks/expected/test_oat_hooks.out | 4 ++++ src/test/regress/expected/privileges.out | 12 ++++++------ src/test/regress/expected/vacuum.out | 2 +- src/test/regress/sql/privileges.sql | 8 ++++---- src/test/regress/sql/vacuum.sql | 2 +- 15 files changed, 48 insertions(+), 16 deletions(-) diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c index 6979aff727..2b5c8a300b 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -282,6 +282,8 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed, SetUserIdAndSecContext(heaprel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); } else { diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 3c6a956eaa..11e78cb62c 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -1066,6 +1066,8 @@ brin_summarize_range(PG_FUNCTION_ARGS) SetUserIdAndSecContext(heapRel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); } else { diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 352e43d0e6..e8337041ac 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1475,6 +1475,8 @@ index_concurrently_build(Oid heapRelationId, SetUserIdAndSecContext(heapRel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); indexRelation = index_open(indexRelationId, RowExclusiveLock); @@ -3006,6 +3008,8 @@ index_build(Relation heapRelation, SetUserIdAndSecContext(heapRelation->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* Set up initial progress report status */ { @@ -3341,6 +3345,8 @@ validate_index(Oid heapId, Oid indexId, Snapshot snapshot) SetUserIdAndSecContext(heapRelation->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); indexRelation = index_open(indexId, RowExclusiveLock); @@ -3601,6 +3607,8 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence, SetUserIdAndSecContext(heapRelation->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); if (progress) { diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index 52ef462dba..2fdd180497 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -348,6 +348,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params, SetUserIdAndSecContext(onerel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* measure elapsed time iff autovacuum logging requires it */ if (IsAutoVacuumWorkerProcess() && params->log_min_duration >= 0) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 369fea7c04..c94a3e20d6 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -355,6 +355,8 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params) SetUserIdAndSecContext(OldHeap->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* * Since we may open a new transaction for each relation, we have to check diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index a5168c9f09..a7c6a3dc7a 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -575,6 +575,8 @@ DefineIndex(Oid relationId, int root_save_nestlevel; root_save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* * Some callers need us to run with an empty default_tablespace; this is a @@ -1300,6 +1302,8 @@ DefineIndex(Oid relationId, SetUserIdAndSecContext(childrel->rd_rel->relowner, child_save_sec_context | SECURITY_RESTRICTED_OPERATION); child_save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* * Don't try to create indexes on foreign tables, though. Skip @@ -3753,6 +3757,8 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) SetUserIdAndSecContext(heapRel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* determine safety of this index for set_indexsafe_procflags */ idx->safe = (indexRel->rd_indexprs == NIL && diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c index f9a3bdfc3a..9114a7924c 100644 --- a/src/backend/commands/matview.c +++ b/src/backend/commands/matview.c @@ -179,6 +179,8 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString, SetUserIdAndSecContext(relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* Make sure it is a materialized view. */ if (matviewRel->rd_rel->relkind != RELKIND_MATVIEW) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index a843f9ad92..c082611797 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -2172,6 +2172,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, SetUserIdAndSecContext(rel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); /* * If PROCESS_MAIN is set (the default), it's time to vacuum the main diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl index eccfcc54a1..f91b5127a8 100644 --- a/src/bin/scripts/t/100_vacuumdb.pl +++ b/src/bin/scripts/t/100_vacuumdb.pl @@ -109,15 +109,11 @@ $node->safe_psql( CREATE FUNCTION f1(int) RETURNS int LANGUAGE SQL AS 'SELECT f0($1)'; CREATE TABLE funcidx (x int); INSERT INTO funcidx VALUES (0),(1),(2),(3); - CREATE INDEX i0 ON funcidx ((f1(x))); CREATE SCHEMA "Foo"; CREATE TABLE "Foo".bar(id int); |); $node->command_ok([qw|vacuumdb -Z --table="need""q(uot"(")x") postgres|], 'column list'); -$node->command_fails( - [qw|vacuumdb -Zt funcidx postgres|], - 'unqualified name via functional index'); $node->command_fails( [ 'vacuumdb', '--analyze', '--table', 'vactable(c)', 'postgres' ], diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index d5253c7ed2..edd82a551b 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -201,6 +201,12 @@ typedef enum #define GUC_QUALIFIER_SEPARATOR '.' +/* + * Safe search path when executing code as the table owner, such as during + * maintenance operations. + */ +#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp" + /* * Bit values in "flags" of a GUC variable. Note that these don't appear * on disk, so we can reassign their values freely. diff --git a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out index f80373aecc..effdc49145 100644 --- a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out +++ b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out @@ -89,11 +89,15 @@ NOTICE: in object access: superuser finished create (subId=0x0) [internal] NOTICE: in process utility: superuser finished CREATE TABLE CREATE INDEX regress_test_table_t_idx ON regress_test_table (t); NOTICE: in process utility: superuser attempting CREATE INDEX +NOTICE: in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed] +NOTICE: in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed] NOTICE: in object access: superuser attempting create (subId=0x0) [explicit] NOTICE: in object access: superuser finished create (subId=0x0) [explicit] NOTICE: in process utility: superuser finished CREATE INDEX GRANT SELECT ON Table regress_test_table TO public; NOTICE: in process utility: superuser attempting GRANT +NOTICE: in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed] +NOTICE: in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed] NOTICE: in process utility: superuser finished GRANT CREATE FUNCTION regress_test_func (t text) RETURNS text AS $$ SELECT $1; diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 3cf4ac8c9e..997c5c57a5 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -1769,7 +1769,7 @@ SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS 'GRANT regress_priv_group2 TO regress_sro_user'; CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS - 'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true'; + 'DECLARE c CURSOR WITH HOLD FOR SELECT public.unwanted_grant(); SELECT true'; -- REFRESH of this MV will queue a GRANT at end of transaction CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA; REFRESH MATERIALIZED VIEW sro_mv; @@ -1783,12 +1783,12 @@ SET SESSION AUTHORIZATION regress_sro_user; -- INSERT to this table will queue a GRANT at end of transaction CREATE TABLE sro_trojan_table (); CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS - 'BEGIN PERFORM unwanted_grant(); RETURN NULL; END'; + 'BEGIN PERFORM public.unwanted_grant(); RETURN NULL; END'; CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan(); -- Now, REFRESH will issue such an INSERT, queueing the GRANT CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS - 'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true'; + 'INSERT INTO public.sro_trojan_table DEFAULT VALUES; SELECT true'; REFRESH MATERIALIZED VIEW sro_mv; ERROR: cannot fire deferred trigger within security-restricted operation CONTEXT: SQL function "mv_action" statement 1 @@ -1800,15 +1800,15 @@ BEGIN; SET CONSTRAINTS ALL IMMEDIATE; REFRESH MATERIALIZED VIEW sro_mv; COMMIT; ERROR: permission denied to grant role "regress_priv_group2" DETAIL: Only roles with the ADMIN option on role "regress_priv_group2" may grant this role. CONTEXT: SQL function "unwanted_grant" statement 1 -SQL statement "SELECT unwanted_grant()" -PL/pgSQL function sro_trojan() line 1 at PERFORM +SQL statement "SELECT public.unwanted_grant()" +PL/pgSQL function public.sro_trojan() line 1 at PERFORM SQL function "mv_action" statement 1 -- REFRESH MATERIALIZED VIEW CONCURRENTLY use of eval_const_expressions() SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int IMMUTABLE LANGUAGE plpgsql AS $$ BEGIN - PERFORM unwanted_grant(); + PERFORM public.unwanted_grant(); RAISE WARNING 'owned'; RETURN 1; EXCEPTION WHEN OTHERS THEN diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 41e020cf20..454821c16b 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -64,7 +64,7 @@ CLUSTER vaccluster; CREATE FUNCTION do_analyze() RETURNS VOID VOLATILE LANGUAGE SQL AS 'ANALYZE pg_am'; CREATE FUNCTION wrap_do_analyze(c INT) RETURNS INT IMMUTABLE LANGUAGE SQL - AS 'SELECT $1 FROM do_analyze()'; + AS 'SELECT $1 FROM public.do_analyze()'; CREATE INDEX ON vaccluster(wrap_do_analyze(i)); INSERT INTO vaccluster VALUES (1), (2); ANALYZE vaccluster; diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index 134809e8cc..e961748d79 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -1177,7 +1177,7 @@ SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant() RETURNS void LANGUAGE sql AS 'GRANT regress_priv_group2 TO regress_sro_user'; CREATE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS - 'DECLARE c CURSOR WITH HOLD FOR SELECT unwanted_grant(); SELECT true'; + 'DECLARE c CURSOR WITH HOLD FOR SELECT public.unwanted_grant(); SELECT true'; -- REFRESH of this MV will queue a GRANT at end of transaction CREATE MATERIALIZED VIEW sro_mv AS SELECT mv_action() WITH NO DATA; REFRESH MATERIALIZED VIEW sro_mv; @@ -1188,12 +1188,12 @@ SET SESSION AUTHORIZATION regress_sro_user; -- INSERT to this table will queue a GRANT at end of transaction CREATE TABLE sro_trojan_table (); CREATE FUNCTION sro_trojan() RETURNS trigger LANGUAGE plpgsql AS - 'BEGIN PERFORM unwanted_grant(); RETURN NULL; END'; + 'BEGIN PERFORM public.unwanted_grant(); RETURN NULL; END'; CREATE CONSTRAINT TRIGGER t AFTER INSERT ON sro_trojan_table INITIALLY DEFERRED FOR EACH ROW EXECUTE PROCEDURE sro_trojan(); -- Now, REFRESH will issue such an INSERT, queueing the GRANT CREATE OR REPLACE FUNCTION mv_action() RETURNS bool LANGUAGE sql AS - 'INSERT INTO sro_trojan_table DEFAULT VALUES; SELECT true'; + 'INSERT INTO public.sro_trojan_table DEFAULT VALUES; SELECT true'; REFRESH MATERIALIZED VIEW sro_mv; \c - REFRESH MATERIALIZED VIEW sro_mv; @@ -1204,7 +1204,7 @@ SET SESSION AUTHORIZATION regress_sro_user; CREATE FUNCTION unwanted_grant_nofail(int) RETURNS int IMMUTABLE LANGUAGE plpgsql AS $$ BEGIN - PERFORM unwanted_grant(); + PERFORM public.unwanted_grant(); RAISE WARNING 'owned'; RETURN 1; EXCEPTION WHEN OTHERS THEN diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql index 51d7b1fecc..0b63ef8dc6 100644 --- a/src/test/regress/sql/vacuum.sql +++ b/src/test/regress/sql/vacuum.sql @@ -49,7 +49,7 @@ CLUSTER vaccluster; CREATE FUNCTION do_analyze() RETURNS VOID VOLATILE LANGUAGE SQL AS 'ANALYZE pg_am'; CREATE FUNCTION wrap_do_analyze(c INT) RETURNS INT IMMUTABLE LANGUAGE SQL - AS 'SELECT $1 FROM do_analyze()'; + AS 'SELECT $1 FROM public.do_analyze()'; CREATE INDEX ON vaccluster(wrap_do_analyze(i)); INSERT INTO vaccluster VALUES (1), (2); ANALYZE vaccluster; -- 2.34.1 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2023-06-08 22:08 Greg Stark <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Greg Stark @ 2023-06-08 22:08 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: pgsql-hackers; Nathan Bossart <[email protected]> On Fri, 26 May 2023 at 19:22, Jeff Davis <[email protected]> wrote: > > Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, > REINDEX, and VACUUM) currently run as the table owner, and as a > SECURITY_RESTRICTED_OPERATION. > > I propose that we also fix the search_path to "pg_catalog, pg_temp" > when running maintenance commands, for two reasons: > > 1. Make the behavior of maintenance commands more consistent because > they'd always have the same search_path. What exactly would this impact? Offhand... expression indexes where the functions in the expression (which would already be schema qualified) themselves reference other objects without schema qualification? So this would negatively affect someone who was using such a dangerous function definition but was careful to always use the same search_path on it. Perhaps someone who had created an expression index on their own table in their own schema calling their own functions in their own schema. As long as nobody else ever calls it that would work but this would cause superuser to no longer be able to reindex it even if superuser set the same search_path? I guess that's pretty narrow and a reasonable thing to desupport. Users could just mark those functions with search_path or schema qualify the object references in them. Perhaps we should also be picking up cases like that sooner so users realize they've created a footgun for themselves? -- greg ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-06-08 22:08 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-01-21 22:41 [PATCH] do only critical work during single-user vacuum? John Naylor <[email protected]> 2022-02-01 21:50 [PATCH 1/3] do only critical work during single-user vacuum? John Naylor <[email protected]> 2023-05-26 23:21 Fix search_path for all maintenance commands Jeff Davis <[email protected]> 2023-06-08 22:08 ` Re: Fix search_path for all maintenance commands Greg Stark <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox