On Tue, Jun 20, 2023 at 10:04:37AM -0700, Jeff Davis wrote: > I think v4-0001 broke the handling of toast tables? It looks like you > removed the check for !skip_privs but need to add it to the flags in > vacuum_is_permitted_for_relation().
Good catch. I'm not sure why some of the calls to vacuum_is_permitted_for_relation() are masking the options. AFAICT we can simply remove the masks. I've done so in the attached patch. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
>From b7528138025f6fae062008fe4e166957789bdc25 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Wed, 14 Jun 2023 11:08:03 -0700 Subject: [PATCH v5 1/1] convert boolean parameter into a VACOPT_* option --- src/backend/commands/analyze.c | 2 +- src/backend/commands/vacuum.c | 28 +++++++++++++++++++--------- src/include/commands/vacuum.h | 1 + 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index 52ef462dba..08f7ba1623 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -167,7 +167,7 @@ analyze_rel(Oid relid, RangeVar *relation, */ if (!vacuum_is_permitted_for_relation(RelationGetRelid(onerel), onerel->rd_rel, - params->options & VACOPT_ANALYZE)) + params->options)) { relation_close(onerel, ShareUpdateExclusiveLock); return; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index a843f9ad92..108b5640d1 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -115,7 +115,7 @@ static void vac_truncate_clog(TransactionId frozenXID, TransactionId lastSaneFrozenXid, MultiXactId lastSaneMinMulti); static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, - bool skip_privs, BufferAccessStrategy bstrategy); + BufferAccessStrategy bstrategy); static double compute_parallel_delay(void); static VacOptValue get_vacoptval_from_boolean(DefElem *def); static bool vac_tid_reaped(ItemPointer itemptr, void *state); @@ -620,8 +620,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy, if (params->options & VACOPT_VACUUM) { - if (!vacuum_rel(vrel->oid, vrel->relation, params, false, - bstrategy)) + if (!vacuum_rel(vrel->oid, vrel->relation, params, bstrategy)) continue; } @@ -712,6 +711,13 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, Assert((options & (VACOPT_VACUUM | VACOPT_ANALYZE)) != 0); + /* + * Privilege checks are bypassed in some cases (e.g., when recursing to a + * relation's TOAST table). + */ + if (options & VACOPT_SKIP_PRIVS) + return true; + /*---------- * A role has privileges to vacuum or analyze the relation if any of the * following are true: @@ -1953,7 +1959,7 @@ vac_truncate_clog(TransactionId frozenXID, */ static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, - bool skip_privs, BufferAccessStrategy bstrategy) + BufferAccessStrategy bstrategy) { LOCKMODE lmode; Relation rel; @@ -2040,10 +2046,9 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, * happen across multiple transactions where privileges could have changed * in-between. Make sure to only generate logs for VACUUM in this case. */ - if (!skip_privs && - !vacuum_is_permitted_for_relation(RelationGetRelid(rel), + if (!vacuum_is_permitted_for_relation(RelationGetRelid(rel), rel->rd_rel, - params->options & VACOPT_VACUUM)) + params->options)) { relation_close(rel, lmode); PopActiveSnapshot(); @@ -2229,11 +2234,16 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params, { VacuumParams toast_vacuum_params; - /* force VACOPT_PROCESS_MAIN so vacuum_rel() processes it */ + /* + * Force VACOPT_PROCESS_MAIN so vacuum_rel() processes it. Likewise, + * set VACOPT_SKIP_PRIVS since privileges on the main relation are + * sufficient to process it. + */ memcpy(&toast_vacuum_params, params, sizeof(VacuumParams)); toast_vacuum_params.options |= VACOPT_PROCESS_MAIN; + toast_vacuum_params.options |= VACOPT_SKIP_PRIVS; - vacuum_rel(toast_relid, NULL, &toast_vacuum_params, true, bstrategy); + vacuum_rel(toast_relid, NULL, &toast_vacuum_params, bstrategy); } /* diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 17e9b4f68e..cb5b11ab31 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -191,6 +191,7 @@ typedef struct VacAttrStats #define VACOPT_DISABLE_PAGE_SKIPPING 0x100 /* don't skip any pages */ #define VACOPT_SKIP_DATABASE_STATS 0x200 /* skip vac_update_datfrozenxid() */ #define VACOPT_ONLY_DATABASE_STATS 0x400 /* only vac_update_datfrozenxid() */ +#define VACOPT_SKIP_PRIVS 0x800 /* skip privilege checks */ /* * Values used by index_cleanup and truncate params. -- 2.25.1