Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oahcy-0002so-Ve for pgsql-hackers@arkaria.postgresql.org; Tue, 20 Sep 2022 18:05:49 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1oahcx-0005dS-S1 for pgsql-hackers@arkaria.postgresql.org; Tue, 20 Sep 2022 18:05:47 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oahcx-0005dJ-Cg for pgsql-hackers@lists.postgresql.org; Tue, 20 Sep 2022 18:05:47 +0000 Received: from mail-pl1-x636.google.com ([2607:f8b0:4864:20::636]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1oahcp-0003ND-2h for pgsql-hackers@postgresql.org; Tue, 20 Sep 2022 18:05:47 +0000 Received: by mail-pl1-x636.google.com with SMTP id p18so3188463plr.8 for ; Tue, 20 Sep 2022 11:05:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date; bh=luEFHwFHiLDZdWyHR5iQ9I/AxrjEdSAAkTqxVaV0tI4=; b=aCUJtssDozyJrasJSwMrUeLYLXgorn5GyRmZRm1wgYbxF+179BVdBkpS2S9JgRKkrt 1/g/QefD+sMXC5RiJXwLK0VDsqPsAC2GKiaTzVg7zdProfker19KoTWC/7OGGRKfKP7N bM//ZeME6+TglJEviZiGjJ/ZUCL+YOfYT1BE0nxojiRqkycMkZuMt8a32wJTSs4fUPY0 ubUWu/0d/9feK/NqGaBmAhvc5NFRjLputh8pjg1jehQbJ356HuDjGglb5xXYuyY98KUj gkqo+JTDH06PcHqTg5Jp4wqmGS7s+pqAL1mBnO2CXCPvvPtY3qUxdmuTk+TJ5Kwg4wtV yznA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date; bh=luEFHwFHiLDZdWyHR5iQ9I/AxrjEdSAAkTqxVaV0tI4=; b=78nCjyfiBU7KLoV53Ehnq3uKIGWEOgWkQaSLdKjQ5QLmxl08aM01X2PKKW+l2T8BDq gWAsdIFD1PEi1Piy+WZ4HXvvqkmMCFiHy0G1fatFuUTaE/dcqxKt/eSpjxWHIw5a2bBH KWCLQNsCu1Ng8Lb/wjKQspjmYECNBd1M1ZEzVeq9eVIKYbKeR9vvdebJVKc/pGNoRku3 epiN6f1rBCNGT2/pWvIFM7c67xyZ6cfX+AvPcTo+gNs3vZ3ES4cjNxUKsbO7qjQthIEF 6RyTco8ekv6q7MskJ+G1K61UMeITKMEM4j9cw2hASgkZ9L//G+7zkZtTT0c+U94rbljR PbxQ== X-Gm-Message-State: ACrzQf1ud4r4Vrizs8dMx2ckS0rKsD8LZaXH0emoIZLTMpWoJRgxqfxi h7aR7scGVgSQN0mSuCUN4wc= X-Google-Smtp-Source: AMsMyM6X3uz3Vb//6YuKK+vExCCES6C6iCxBhQY8iaQm+SKyHuMX9HaN9eo2oetN871g5oGUxdY3wQ== X-Received: by 2002:a17:90a:9309:b0:1fa:d28b:3751 with SMTP id p9-20020a17090a930900b001fad28b3751mr5025770pjo.189.1663697136662; Tue, 20 Sep 2022 11:05:36 -0700 (PDT) Received: from nathanxps13 ([50.47.162.83]) by smtp.gmail.com with ESMTPSA id d4-20020a170902654400b00172a670607asm157240pln.300.2022.09.20.11.05.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 20 Sep 2022 11:05:35 -0700 (PDT) Date: Tue, 20 Sep 2022 11:05:33 -0700 From: Nathan Bossart To: Michael Paquier Cc: Robert Haas , Stephen Frost , Bharath Rupireddy , "David G. Johnston" , Kyotaro Horiguchi , "pgsql-hackers@postgresql.org" Subject: Re: predefined role(s) for VACUUM and ANALYZE Message-ID: <20220920180533.GA205278@nathanxps13> References: <20220905185630.GA1961927@nathanxps13> <20220906151151.GB26002@tamriel.snowman.net> <20220906155432.GA1963795@nathanxps13> <20220907211343.GE26002@tamriel.snowman.net> <20220907221103.GA2095022@nathanxps13> <20220920035147.GA114383@nathanxps13> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="ZPt4rx8FFjLCG7dd" Content-Disposition: inline In-Reply-To: List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --ZPt4rx8FFjLCG7dd Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Sep 20, 2022 at 02:45:52PM +0900, Michael Paquier wrote: > I have gone through the thread, and I'd agree with getting more > granularity when it comes to assigning ACLs to relations rather than > just an on/off switch for the objects of a given type would be nice. > I've been looking at the whole use of AclMode and AclItem in the code, > and I don't quite see why a larger size could have a noticeable > impact. There are a few things that could handle a large number of > AclItems, though, say for array operations like aclupdate(). These > could be easily checked with some micro-benchmarking or some SQL > queries that emulate a large number of items in aclitem[] arrays. I performed a few quick tests with a couple thousand ACLs on my laptop, and I'm consistently seeing a 4.3% regression. > Any impact for the column sizes of the catalogs holding ACL > information? Just asking while browsing the patch set. Since each aclitem requires 16 bytes instead of 12, I assume so. However, in my testing, I hit a "row is too big" error with the same number of aclitems in a pg_class row before and after the change. I might be missing something in my patch, or maybe I am misunderstanding how arrays of aclitems are stored on disk. > Some comments in utils/acl.h need a refresh as the number of lower and > upper bits looked at from ai_privs changes. Oops, I missed that one. I fixed it in the attached patch set. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com --ZPt4rx8FFjLCG7dd Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0001-Change-AclMode-from-a-uint32-to-a-uint64.patch" From 15e9a60b990070d36f4e1f749cbfbb638b37a666 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Wed, 7 Sep 2022 22:25:29 -0700 Subject: [PATCH v6 1/4] Change AclMode from a uint32 to a uint64. --- src/backend/nodes/outfuncs.c | 2 +- src/bin/pg_upgrade/check.c | 35 +++++++++++++++++++++++++++++++++ src/include/catalog/pg_type.dat | 2 +- src/include/nodes/parsenodes.h | 6 +++--- src/include/utils/acl.h | 28 +++++++++++++------------- 5 files changed, 54 insertions(+), 19 deletions(-) diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 60610e3a4b..dbb9ad52da 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -528,7 +528,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node) WRITE_BOOL_FIELD(lateral); WRITE_BOOL_FIELD(inh); WRITE_BOOL_FIELD(inFromCl); - WRITE_UINT_FIELD(requiredPerms); + WRITE_UINT64_FIELD(requiredPerms); WRITE_OID_FIELD(checkAsUser); WRITE_BITMAPSET_FIELD(selectedCols); WRITE_BITMAPSET_FIELD(insertedCols); diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index f1bc1e6886..615a53a864 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -28,6 +28,7 @@ static void check_for_incompatible_polymorphics(ClusterInfo *cluster); static void check_for_tables_with_oids(ClusterInfo *cluster); static void check_for_composite_data_type_usage(ClusterInfo *cluster); static void check_for_reg_data_type_usage(ClusterInfo *cluster); +static void check_for_aclitem_data_type_usage(ClusterInfo *cluster); static void check_for_jsonb_9_4_usage(ClusterInfo *cluster); static void check_for_pg_role_prefix(ClusterInfo *cluster); static void check_for_new_tablespace_dir(ClusterInfo *new_cluster); @@ -107,6 +108,13 @@ check_and_dump_old_cluster(bool live_check) check_for_reg_data_type_usage(&old_cluster); check_for_isn_and_int8_passing_mismatch(&old_cluster); + /* + * PG 16 increased the size of the 'aclitem' type, which breaks the on-disk + * format for existing data. + */ + if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1500) + check_for_aclitem_data_type_usage(&old_cluster); + /* * PG 14 changed the function signature of encoding conversion functions. * Conversions from older versions cannot be upgraded automatically @@ -1319,6 +1327,33 @@ check_for_reg_data_type_usage(ClusterInfo *cluster) check_ok(); } +/* + * check_for_aclitem_data_type_usage + * + * aclitem changed its storage format in 16, so check for it. + */ +static void +check_for_aclitem_data_type_usage(ClusterInfo *cluster) +{ + char output_path[MAXPGPATH]; + + prep_status("Checking for incompatible aclitem data type in user tables"); + + snprintf(output_path, sizeof(output_path), "tables_using_aclitem.txt"); + + if (check_for_data_type_usage(cluster, "pg_catalog.aclitem", output_path)) + { + pg_log(PG_REPORT, "fatal"); + pg_fatal("Your installation contains the \"aclitem\" data type in user tables.\n" + "The internal format of \"aclitem\" changed in PostgreSQL version 16\n" + "so this cluster cannot currently be upgraded. You can drop the\n" + "problem columns and restart the upgrade. A list of the problem\n" + "columns is in the file:\n" + " %s", output_path); + } + else + check_ok(); +} /* * check_for_jsonb_9_4_usage() diff --git a/src/include/catalog/pg_type.dat b/src/include/catalog/pg_type.dat index df45879463..484dec39e8 100644 --- a/src/include/catalog/pg_type.dat +++ b/src/include/catalog/pg_type.dat @@ -267,7 +267,7 @@ # OIDS 1000 - 1099 { oid => '1033', array_type_oid => '1034', descr => 'access control list', - typname => 'aclitem', typlen => '12', typbyval => 'f', typcategory => 'U', + typname => 'aclitem', typlen => '16', typbyval => 'f', typcategory => 'U', typinput => 'aclitemin', typoutput => 'aclitemout', typreceive => '-', typsend => '-', typalign => 'i' }, { oid => '1042', array_type_oid => '1014', diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 6958306a7d..726529476c 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -72,12 +72,12 @@ typedef enum SetQuantifier /* * Grantable rights are encoded so that we can OR them together in a bitmask. - * The present representation of AclItem limits us to 16 distinct rights, - * even though AclMode is defined as uint32. See utils/acl.h. + * The present representation of AclItem limits us to 32 distinct rights, + * even though AclMode is defined as uint64. See utils/acl.h. * * Caution: changing these codes breaks stored ACLs, hence forces initdb. */ -typedef uint32 AclMode; /* a bitmask of privilege bits */ +typedef uint64 AclMode; /* a bitmask of privilege bits */ #define ACL_INSERT (1<<0) /* for relations */ #define ACL_SELECT (1<<1) diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h index 3d6411197c..7e41332f8f 100644 --- a/src/include/utils/acl.h +++ b/src/include/utils/acl.h @@ -59,33 +59,33 @@ typedef struct AclItem } AclItem; /* - * The upper 16 bits of the ai_privs field of an AclItem are the grant option - * bits, and the lower 16 bits are the actual privileges. We use "rights" + * The upper 32 bits of the ai_privs field of an AclItem are the grant option + * bits, and the lower 32 bits are the actual privileges. We use "rights" * to mean the combined grant option and privilege bits fields. */ -#define ACLITEM_GET_PRIVS(item) ((item).ai_privs & 0xFFFF) -#define ACLITEM_GET_GOPTIONS(item) (((item).ai_privs >> 16) & 0xFFFF) +#define ACLITEM_GET_PRIVS(item) ((item).ai_privs & 0xFFFFFFFF) +#define ACLITEM_GET_GOPTIONS(item) (((item).ai_privs >> 32) & 0xFFFFFFFF) #define ACLITEM_GET_RIGHTS(item) ((item).ai_privs) -#define ACL_GRANT_OPTION_FOR(privs) (((AclMode) (privs) & 0xFFFF) << 16) -#define ACL_OPTION_TO_PRIVS(privs) (((AclMode) (privs) >> 16) & 0xFFFF) +#define ACL_GRANT_OPTION_FOR(privs) (((AclMode) (privs) & 0xFFFFFFFF) << 32) +#define ACL_OPTION_TO_PRIVS(privs) (((AclMode) (privs) >> 32) & 0xFFFFFFFF) #define ACLITEM_SET_PRIVS(item,privs) \ - ((item).ai_privs = ((item).ai_privs & ~((AclMode) 0xFFFF)) | \ - ((AclMode) (privs) & 0xFFFF)) + ((item).ai_privs = ((item).ai_privs & ~((AclMode) 0xFFFFFFFF)) | \ + ((AclMode) (privs) & 0xFFFFFFFF)) #define ACLITEM_SET_GOPTIONS(item,goptions) \ - ((item).ai_privs = ((item).ai_privs & ~(((AclMode) 0xFFFF) << 16)) | \ - (((AclMode) (goptions) & 0xFFFF) << 16)) + ((item).ai_privs = ((item).ai_privs & ~(((AclMode) 0xFFFFFFFF) << 32)) | \ + (((AclMode) (goptions) & 0xFFFFFFFF) << 32)) #define ACLITEM_SET_RIGHTS(item,rights) \ ((item).ai_privs = (AclMode) (rights)) #define ACLITEM_SET_PRIVS_GOPTIONS(item,privs,goptions) \ - ((item).ai_privs = ((AclMode) (privs) & 0xFFFF) | \ - (((AclMode) (goptions) & 0xFFFF) << 16)) + ((item).ai_privs = ((AclMode) (privs) & 0xFFFFFFFF) | \ + (((AclMode) (goptions) & 0xFFFFFFFF) << 32)) -#define ACLITEM_ALL_PRIV_BITS ((AclMode) 0xFFFF) -#define ACLITEM_ALL_GOPTION_BITS ((AclMode) 0xFFFF << 16) +#define ACLITEM_ALL_PRIV_BITS ((AclMode) 0xFFFFFFFF) +#define ACLITEM_ALL_GOPTION_BITS ((AclMode) 0xFFFFFFFF << 32) /* * Definitions for convenient access to Acl (array of AclItem). -- 2.25.1 --ZPt4rx8FFjLCG7dd Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0002-Simplify-WARNING-messages-emitted-when-skipping-v.patch" From 0ce85b65606876715fea56de6f75981a370a51a2 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Tue, 6 Sep 2022 10:23:56 -0700 Subject: [PATCH v6 2/4] Simplify WARNING messages emitted when skipping vacuum/analyze for a table. --- src/backend/commands/vacuum.c | 32 ++---- .../isolation/expected/vacuum-conflict.out | 16 +-- src/test/regress/expected/vacuum.out | 102 +++++++++--------- 3 files changed, 65 insertions(+), 85 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 7ccde07de9..651e4a7556 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -579,18 +579,9 @@ vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, bits32 options) if ((options & VACOPT_VACUUM) != 0) { - if (reltuple->relisshared) - ereport(WARNING, - (errmsg("skipping \"%s\" --- only superuser can vacuum it", - relname))); - else if (reltuple->relnamespace == PG_CATALOG_NAMESPACE) - ereport(WARNING, - (errmsg("skipping \"%s\" --- only superuser or database owner can vacuum it", - relname))); - else - ereport(WARNING, - (errmsg("skipping \"%s\" --- only table or database owner can vacuum it", - relname))); + ereport(WARNING, + (errmsg("permission denied to vacuum \"%s\", skipping it", + relname))); /* * For VACUUM ANALYZE, both logs could show up, but just generate @@ -601,20 +592,9 @@ vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, bits32 options) } if ((options & VACOPT_ANALYZE) != 0) - { - if (reltuple->relisshared) - ereport(WARNING, - (errmsg("skipping \"%s\" --- only superuser can analyze it", - relname))); - else if (reltuple->relnamespace == PG_CATALOG_NAMESPACE) - ereport(WARNING, - (errmsg("skipping \"%s\" --- only superuser or database owner can analyze it", - relname))); - else - ereport(WARNING, - (errmsg("skipping \"%s\" --- only table or database owner can analyze it", - relname))); - } + ereport(WARNING, + (errmsg("permission denied to analyze \"%s\", skipping it", + relname))); return false; } diff --git a/src/test/isolation/expected/vacuum-conflict.out b/src/test/isolation/expected/vacuum-conflict.out index ffde537305..77e45506c3 100644 --- a/src/test/isolation/expected/vacuum-conflict.out +++ b/src/test/isolation/expected/vacuum-conflict.out @@ -4,7 +4,7 @@ starting permutation: s1_begin s1_lock s2_auth s2_vacuum s1_commit s2_reset step s1_begin: BEGIN; step s1_lock: LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; step s2_auth: SET ROLE regress_vacuum_conflict; -s2: WARNING: skipping "vacuum_tab" --- only table or database owner can vacuum it +s2: WARNING: permission denied to vacuum "vacuum_tab", skipping it step s2_vacuum: VACUUM vacuum_tab; step s1_commit: COMMIT; step s2_reset: RESET ROLE; @@ -12,7 +12,7 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s2_vacuum s1_lock s1_commit s2_reset step s1_begin: BEGIN; step s2_auth: SET ROLE regress_vacuum_conflict; -s2: WARNING: skipping "vacuum_tab" --- only table or database owner can vacuum it +s2: WARNING: permission denied to vacuum "vacuum_tab", skipping it step s2_vacuum: VACUUM vacuum_tab; step s1_lock: LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; step s1_commit: COMMIT; @@ -22,14 +22,14 @@ starting permutation: s1_begin s2_auth s1_lock s2_vacuum s1_commit s2_reset step s1_begin: BEGIN; step s2_auth: SET ROLE regress_vacuum_conflict; step s1_lock: LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; -s2: WARNING: skipping "vacuum_tab" --- only table or database owner can vacuum it +s2: WARNING: permission denied to vacuum "vacuum_tab", skipping it step s2_vacuum: VACUUM vacuum_tab; step s1_commit: COMMIT; step s2_reset: RESET ROLE; starting permutation: s2_auth s2_vacuum s1_begin s1_lock s1_commit s2_reset step s2_auth: SET ROLE regress_vacuum_conflict; -s2: WARNING: skipping "vacuum_tab" --- only table or database owner can vacuum it +s2: WARNING: permission denied to vacuum "vacuum_tab", skipping it step s2_vacuum: VACUUM vacuum_tab; step s1_begin: BEGIN; step s1_lock: LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; @@ -40,7 +40,7 @@ starting permutation: s1_begin s1_lock s2_auth s2_analyze s1_commit s2_reset step s1_begin: BEGIN; step s1_lock: LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; step s2_auth: SET ROLE regress_vacuum_conflict; -s2: WARNING: skipping "vacuum_tab" --- only table or database owner can analyze it +s2: WARNING: permission denied to analyze "vacuum_tab", skipping it step s2_analyze: ANALYZE vacuum_tab; step s1_commit: COMMIT; step s2_reset: RESET ROLE; @@ -48,7 +48,7 @@ step s2_reset: RESET ROLE; starting permutation: s1_begin s2_auth s2_analyze s1_lock s1_commit s2_reset step s1_begin: BEGIN; step s2_auth: SET ROLE regress_vacuum_conflict; -s2: WARNING: skipping "vacuum_tab" --- only table or database owner can analyze it +s2: WARNING: permission denied to analyze "vacuum_tab", skipping it step s2_analyze: ANALYZE vacuum_tab; step s1_lock: LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; step s1_commit: COMMIT; @@ -58,14 +58,14 @@ starting permutation: s1_begin s2_auth s1_lock s2_analyze s1_commit s2_reset step s1_begin: BEGIN; step s2_auth: SET ROLE regress_vacuum_conflict; step s1_lock: LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; -s2: WARNING: skipping "vacuum_tab" --- only table or database owner can analyze it +s2: WARNING: permission denied to analyze "vacuum_tab", skipping it step s2_analyze: ANALYZE vacuum_tab; step s1_commit: COMMIT; step s2_reset: RESET ROLE; starting permutation: s2_auth s2_analyze s1_begin s1_lock s1_commit s2_reset step s2_auth: SET ROLE regress_vacuum_conflict; -s2: WARNING: skipping "vacuum_tab" --- only table or database owner can analyze it +s2: WARNING: permission denied to analyze "vacuum_tab", skipping it step s2_analyze: ANALYZE vacuum_tab; step s1_begin: BEGIN; step s1_lock: LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index c63a157e5f..0035d158b7 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -295,120 +295,120 @@ CREATE ROLE regress_vacuum; SET ROLE regress_vacuum; -- Simple table VACUUM vacowned; -WARNING: skipping "vacowned" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned", skipping it ANALYZE vacowned; -WARNING: skipping "vacowned" --- only table or database owner can analyze it +WARNING: permission denied to analyze "vacowned", skipping it VACUUM (ANALYZE) vacowned; -WARNING: skipping "vacowned" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned", skipping it -- Catalog VACUUM pg_catalog.pg_class; -WARNING: skipping "pg_class" --- only superuser or database owner can vacuum it +WARNING: permission denied to vacuum "pg_class", skipping it ANALYZE pg_catalog.pg_class; -WARNING: skipping "pg_class" --- only superuser or database owner can analyze it +WARNING: permission denied to analyze "pg_class", skipping it VACUUM (ANALYZE) pg_catalog.pg_class; -WARNING: skipping "pg_class" --- only superuser or database owner can vacuum it +WARNING: permission denied to vacuum "pg_class", skipping it -- Shared catalog VACUUM pg_catalog.pg_authid; -WARNING: skipping "pg_authid" --- only superuser can vacuum it +WARNING: permission denied to vacuum "pg_authid", skipping it ANALYZE pg_catalog.pg_authid; -WARNING: skipping "pg_authid" --- only superuser can analyze it +WARNING: permission denied to analyze "pg_authid", skipping it VACUUM (ANALYZE) pg_catalog.pg_authid; -WARNING: skipping "pg_authid" --- only superuser can vacuum it +WARNING: permission denied to vacuum "pg_authid", skipping it -- Partitioned table and its partitions, nothing owned by other user. -- Relations are not listed in a single command to test ownership -- independently. VACUUM vacowned_parted; -WARNING: skipping "vacowned_parted" --- only table or database owner can vacuum it -WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_parted", skipping it +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; -WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; -WARNING: skipping "vacowned_parted" --- only table or database owner can analyze it -WARNING: skipping "vacowned_part1" --- only table or database owner can analyze it -WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +WARNING: permission denied to analyze "vacowned_parted", skipping it +WARNING: permission denied to analyze "vacowned_part1", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; -WARNING: skipping "vacowned_part1" --- only table or database owner can analyze it +WARNING: permission denied to analyze "vacowned_part1", skipping it ANALYZE vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; -WARNING: skipping "vacowned_parted" --- only table or database owner can vacuum it -WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_parted", skipping it +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; -WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Partitioned table and one partition owned by other user. ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO regress_vacuum; SET ROLE regress_vacuum; VACUUM vacowned_parted; -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; VACUUM vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; -WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; ANALYZE vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Only one partition owned by other user. ALTER TABLE vacowned_parted OWNER TO CURRENT_USER; SET ROLE regress_vacuum; VACUUM vacowned_parted; -WARNING: skipping "vacowned_parted" --- only table or database owner can vacuum it -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_parted", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; VACUUM vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; -WARNING: skipping "vacowned_parted" --- only table or database owner can analyze it -WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +WARNING: permission denied to analyze "vacowned_parted", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; ANALYZE vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; -WARNING: skipping "vacowned_parted" --- only table or database owner can vacuum it -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_parted", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; -- Only partitioned table owned by other user. ALTER TABLE vacowned_parted OWNER TO regress_vacuum; ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER; SET ROLE regress_vacuum; VACUUM vacowned_parted; -WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM vacowned_part1; -WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part2", skipping it ANALYZE vacowned_parted; -WARNING: skipping "vacowned_part1" --- only table or database owner can analyze it -WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +WARNING: permission denied to analyze "vacowned_part1", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it ANALYZE vacowned_part1; -WARNING: skipping "vacowned_part1" --- only table or database owner can analyze it +WARNING: permission denied to analyze "vacowned_part1", skipping it ANALYZE vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can analyze it +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; -WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to vacuum "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; -WARNING: skipping "vacowned_part1" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; -WARNING: skipping "vacowned_part2" --- only table or database owner can vacuum it +WARNING: permission denied to vacuum "vacowned_part2", skipping it RESET ROLE; DROP TABLE vacowned; DROP TABLE vacowned_parted; -- 2.25.1 --ZPt4rx8FFjLCG7dd Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0003-Allow-granting-VACUUM-and-ANALYZE-privileges-on-r.patch" From 22dcd77455a9eaf62c8c1e52ff91e530506ed4fd Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Sat, 3 Sep 2022 23:31:38 -0700 Subject: [PATCH v6 3/4] Allow granting VACUUM and ANALYZE privileges on relations. --- doc/src/sgml/ddl.sgml | 49 ++++++++--- doc/src/sgml/func.sgml | 3 +- .../sgml/ref/alter_default_privileges.sgml | 4 +- doc/src/sgml/ref/analyze.sgml | 3 +- doc/src/sgml/ref/grant.sgml | 4 +- doc/src/sgml/ref/revoke.sgml | 2 +- doc/src/sgml/ref/vacuum.sgml | 3 +- src/backend/catalog/aclchk.c | 8 ++ src/backend/commands/analyze.c | 2 +- src/backend/commands/vacuum.c | 24 ++++-- src/backend/parser/gram.y | 7 ++ src/backend/utils/adt/acl.c | 16 ++++ src/bin/pg_dump/dumputils.c | 2 + src/bin/pg_dump/t/002_pg_dump.pl | 2 +- src/bin/psql/tab-complete.c | 4 +- src/include/nodes/parsenodes.h | 4 +- src/include/utils/acl.h | 6 +- src/test/regress/expected/dependency.out | 22 ++--- src/test/regress/expected/privileges.out | 86 ++++++++++++++----- src/test/regress/expected/rowsecurity.out | 34 ++++---- src/test/regress/expected/vacuum.out | 6 ++ src/test/regress/sql/dependency.sql | 2 +- src/test/regress/sql/privileges.sql | 40 +++++++++ 23 files changed, 249 insertions(+), 84 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 03c0193709..ed034a6b1d 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -1691,8 +1691,9 @@ ALTER TABLE products RENAME TO items; INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER, CREATE, CONNECT, TEMPORARY, - EXECUTE, USAGE, SET - and ALTER SYSTEM. + EXECUTE, USAGE, SET, + ALTER SYSTEM, VACUUM, and + ANALYZE. The privileges applicable to a particular object vary depending on the object's type (table, function, etc.). More detail about the meanings of these privileges appears below. @@ -1982,7 +1983,25 @@ REVOKE ALL ON accounts FROM PUBLIC; - + + + VACUUM + + + Allows VACUUM on a relation. + + + + + + ANALYZE + + + Allows ANALYZE on a relation. + + + + The privileges required by other commands are listed on the reference page of the respective command. @@ -2131,6 +2150,16 @@ REVOKE ALL ON accounts FROM PUBLIC; A PARAMETER + + VACUUM + v + TABLE + + + ANALYZE + z + TABLE + @@ -2221,7 +2250,7 @@ REVOKE ALL ON accounts FROM PUBLIC; TABLE (and table-like objects) - arwdDxt + arwdDxtvz none \dp @@ -2279,12 +2308,12 @@ GRANT SELECT (col1), UPDATE (col1) ON mytable TO miriam_rw; would show: => \dp mytable - Access privileges - Schema | Name | Type | Access privileges | Column privileges | Policies ---------+---------+-------+-----------------------+-----------------------+---------- - public | mytable | table | miriam=arwdDxt/miriam+| col1: +| - | | | =r/miriam +| miriam_rw=rw/miriam | - | | | admin=arw/miriam | | + Access privileges + Schema | Name | Type | Access privileges | Column privileges | Policies +--------+---------+-------+-------------------------+-----------------------+---------- + public | mytable | table | miriam=arwdDxtvz/miriam+| col1: +| + | | | =r/miriam +| miriam_rw=rw/miriam | + | | | admin=arw/miriam | | (1 row) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index e1fe4fec57..7855ac1d34 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -22962,7 +22962,8 @@ SELECT has_function_privilege('joeuser', 'myfunc(int, text)', 'execute'); are SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, - and TRIGGER. + TRIGGER, VACUUM and + ANALYZE. diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml index f1d54f5aa3..0da295daff 100644 --- a/doc/src/sgml/ref/alter_default_privileges.sgml +++ b/doc/src/sgml/ref/alter_default_privileges.sgml @@ -28,7 +28,7 @@ ALTER DEFAULT PRIVILEGES where abbreviated_grant_or_revoke is one of: -GRANT { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER } +GRANT { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | VACUUM | ANALYZE } [, ...] | ALL [ PRIVILEGES ] } ON TABLES TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ] @@ -51,7 +51,7 @@ GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] } TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ] REVOKE [ GRANT OPTION FOR ] - { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER } + { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | VACUUM | ANALYZE } [, ...] | ALL [ PRIVILEGES ] } ON TABLES FROM { [ GROUP ] role_name | PUBLIC } [, ...] diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 2ba115d1ad..400ea30cd0 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -149,7 +149,8 @@ ANALYZE [ VERBOSE ] [ table_and_columns To analyze a table, one must ordinarily be the table's owner or a - superuser. However, database owners are allowed to + superuser or have the ANALYZE privilege on the table. + However, database owners are allowed to analyze all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide ANALYZE can only be performed by a superuser.) diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml index dea19cd348..f6234d975a 100644 --- a/doc/src/sgml/ref/grant.sgml +++ b/doc/src/sgml/ref/grant.sgml @@ -21,7 +21,7 @@ PostgreSQL documentation -GRANT { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER } +GRANT { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | VACUUM | ANALYZE } [, ...] | ALL [ PRIVILEGES ] } ON { [ TABLE ] table_name [, ...] | ALL TABLES IN SCHEMA schema_name [, ...] } @@ -193,6 +193,8 @@ GRANT role_name [, ...] TO USAGE SET ALTER SYSTEM + VACUUM + ANALYZE Specific types of privileges, as defined in . diff --git a/doc/src/sgml/ref/revoke.sgml b/doc/src/sgml/ref/revoke.sgml index 4fd4bfb3d7..ece1aa721f 100644 --- a/doc/src/sgml/ref/revoke.sgml +++ b/doc/src/sgml/ref/revoke.sgml @@ -22,7 +22,7 @@ PostgreSQL documentation REVOKE [ GRANT OPTION FOR ] - { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER } + { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | VACUUM | ANALYZE } [, ...] | ALL [ PRIVILEGES ] } ON { [ TABLE ] table_name [, ...] | ALL TABLES IN SCHEMA schema_name [, ...] } diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index c582021d29..70c0d81346 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -357,7 +357,8 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ rd_rel, - params->options & VACOPT_ANALYZE)) + VACOPT_ANALYZE)) { relation_close(onerel, ShareUpdateExclusiveLock); return; diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 651e4a7556..15311e91d2 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -547,16 +547,16 @@ vacuum(List *relations, VacuumParams *params, } /* - * Check if a given relation can be safely vacuumed or analyzed. If the - * user is not the relation owner, issue a WARNING log message and return - * false to let the caller decide what to do with this relation. This - * routine is used to decide if a relation can be processed for VACUUM or - * ANALYZE. + * Check if the current user has privileges to vacuum or analyze the relation. + * If not, issue a WARNING log message and return false to let the caller + * decide what to do with this relation. This routine is used to decide if a + * relation can be processed for VACUUM or ANALYZE. */ bool vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, bits32 options) { char *relname; + AclMode mode = 0; Assert((options & (VACOPT_VACUUM | VACOPT_ANALYZE)) != 0); @@ -566,13 +566,19 @@ vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, bits32 options) * We allow the user to vacuum or analyze a table if he is superuser, the * table owner, or the database owner (but in the latter case, only if * it's not a shared relation). pg_class_ownercheck includes the - * superuser case. + * superuser case. The user might also have been granted privileges to + * vacuum or analyze the table. * * Note we choose to treat permissions failure as a WARNING and keep * trying to vacuum or analyze the rest of the DB --- is this appropriate? */ + if (options & VACOPT_VACUUM) + mode |= ACL_VACUUM; + if (options & VACOPT_ANALYZE) + mode |= ACL_ANALYZE; if (pg_class_ownercheck(relid, GetUserId()) || - (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !reltuple->relisshared)) + (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !reltuple->relisshared) || + pg_class_aclcheck(relid, GetUserId(), mode) == ACLCHECK_OK) return true; relname = NameStr(reltuple->relname); @@ -1894,12 +1900,12 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) */ if (!vacuum_is_relation_owner(RelationGetRelid(rel), rel->rd_rel, - params->options & VACOPT_VACUUM)) + VACOPT_VACUUM)) { relation_close(rel, lmode); PopActiveSnapshot(); CommitTransactionCommand(); - return false; + return true; /* user may have the ANALYZE privilege */ } /* diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index c6ec694ceb..daaaf1cf1f 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -7483,6 +7483,13 @@ privilege: SELECT opt_column_list n->cols = NIL; $$ = n; } + | analyze_keyword + { + AccessPriv *n = makeNode(AccessPriv); + n->priv_name = pstrdup("analyze"); + n->cols = NIL; + $$ = n; + } | ColId opt_column_list { AccessPriv *n = makeNode(AccessPriv); diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index fd71a9b13e..b4b4a5e6fa 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -314,6 +314,12 @@ aclparse(const char *s, AclItem *aip) case ACL_ALTER_SYSTEM_CHR: read = ACL_ALTER_SYSTEM; break; + case ACL_VACUUM_CHR: + read = ACL_VACUUM; + break; + case ACL_ANALYZE_CHR: + read = ACL_ANALYZE; + break; case 'R': /* ignore old RULE privileges */ read = 0; break; @@ -1588,6 +1594,8 @@ makeaclitem(PG_FUNCTION_ARGS) {"CONNECT", ACL_CONNECT}, {"SET", ACL_SET}, {"ALTER SYSTEM", ACL_ALTER_SYSTEM}, + {"VACUUM", ACL_VACUUM}, + {"ANALYZE", ACL_ANALYZE}, {"RULE", 0}, /* ignore old RULE privileges */ {NULL, 0} }; @@ -1696,6 +1704,10 @@ convert_aclright_to_string(int aclright) return "SET"; case ACL_ALTER_SYSTEM: return "ALTER SYSTEM"; + case ACL_VACUUM: + return "VACUUM"; + case ACL_ANALYZE: + return "ANALYZE"; default: elog(ERROR, "unrecognized aclright: %d", aclright); return NULL; @@ -2005,6 +2017,10 @@ convert_table_priv_string(text *priv_type_text) {"REFERENCES WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_REFERENCES)}, {"TRIGGER", ACL_TRIGGER}, {"TRIGGER WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_TRIGGER)}, + {"VACUUM", ACL_VACUUM}, + {"VACUUM WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_VACUUM)}, + {"ANALYZE", ACL_ANALYZE}, + {"ANALYZE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_ANALYZE)}, {"RULE", 0}, /* ignore old RULE privileges */ {"RULE WITH GRANT OPTION", 0}, {NULL, 0} diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c index 6e501a5413..9311417f18 100644 --- a/src/bin/pg_dump/dumputils.c +++ b/src/bin/pg_dump/dumputils.c @@ -457,6 +457,8 @@ do { \ CONVERT_PRIV('d', "DELETE"); CONVERT_PRIV('t', "TRIGGER"); CONVERT_PRIV('D', "TRUNCATE"); + CONVERT_PRIV('v', "VACUUM"); + CONVERT_PRIV('z', "ANALYZE"); } } diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl index 2873b662fb..f015fe2194 100644 --- a/src/bin/pg_dump/t/002_pg_dump.pl +++ b/src/bin/pg_dump/t/002_pg_dump.pl @@ -566,7 +566,7 @@ my %tests = ( \QREVOKE ALL ON TABLES FROM regress_dump_test_role;\E\n \QALTER DEFAULT PRIVILEGES \E \QFOR ROLE regress_dump_test_role \E - \QGRANT INSERT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLES TO regress_dump_test_role;\E + \QGRANT INSERT,REFERENCES,DELETE,TRIGGER,TRUNCATE,VACUUM,ANALYZE,UPDATE ON TABLES TO regress_dump_test_role;\E /xm, like => { %full_runs, section_post_data => 1, }, unlike => { no_privs => 1, }, diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index f3465adb85..e21f341526 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -3747,7 +3747,7 @@ psql_completion(const char *text, int start, int end) if (HeadMatches("ALTER", "DEFAULT", "PRIVILEGES")) COMPLETE_WITH("SELECT", "INSERT", "UPDATE", "DELETE", "TRUNCATE", "REFERENCES", "TRIGGER", - "EXECUTE", "USAGE", "ALL"); + "EXECUTE", "USAGE", "VACUUM", "ANALYZE", "ALL"); else COMPLETE_WITH_QUERY_PLUS(Query_for_list_of_roles, "GRANT", @@ -3765,6 +3765,8 @@ psql_completion(const char *text, int start, int end) "USAGE", "SET", "ALTER SYSTEM", + "VACUUM", + "ANALYZE", "ALL"); } diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 726529476c..6db572908d 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -94,7 +94,9 @@ typedef uint64 AclMode; /* a bitmask of privilege bits */ #define ACL_CONNECT (1<<11) /* for databases */ #define ACL_SET (1<<12) /* for configuration parameters */ #define ACL_ALTER_SYSTEM (1<<13) /* for configuration parameters */ -#define N_ACL_RIGHTS 14 /* 1 plus the last 1< 44); \dp - Access privileges - Schema | Name | Type | Access privileges | Column privileges | Policies ---------------------+----------+-------+---------------------------------------------+-------------------+-------------------------------------------- - regress_rls_schema | category | table | regress_rls_alice=arwdDxt/regress_rls_alice+| | - | | | =arwdDxt/regress_rls_alice | | - regress_rls_schema | document | table | regress_rls_alice=arwdDxt/regress_rls_alice+| | p1: + - | | | =arwdDxt/regress_rls_alice | | (u): (dlevel <= ( SELECT uaccount.seclv + - | | | | | FROM uaccount + - | | | | | WHERE (uaccount.pguser = CURRENT_USER)))+ - | | | | | p2r (RESTRICTIVE): + - | | | | | (u): ((cid <> 44) AND (cid < 50)) + - | | | | | to: regress_rls_dave + - | | | | | p1r (RESTRICTIVE): + - | | | | | (u): (cid <> 44) + - | | | | | to: regress_rls_dave - regress_rls_schema | uaccount | table | regress_rls_alice=arwdDxt/regress_rls_alice+| | - | | | =r/regress_rls_alice | | + Access privileges + Schema | Name | Type | Access privileges | Column privileges | Policies +--------------------+----------+-------+-----------------------------------------------+-------------------+-------------------------------------------- + regress_rls_schema | category | table | regress_rls_alice=arwdDxtvz/regress_rls_alice+| | + | | | =arwdDxtvz/regress_rls_alice | | + regress_rls_schema | document | table | regress_rls_alice=arwdDxtvz/regress_rls_alice+| | p1: + + | | | =arwdDxtvz/regress_rls_alice | | (u): (dlevel <= ( SELECT uaccount.seclv + + | | | | | FROM uaccount + + | | | | | WHERE (uaccount.pguser = CURRENT_USER)))+ + | | | | | p2r (RESTRICTIVE): + + | | | | | (u): ((cid <> 44) AND (cid < 50)) + + | | | | | to: regress_rls_dave + + | | | | | p1r (RESTRICTIVE): + + | | | | | (u): (cid <> 44) + + | | | | | to: regress_rls_dave + regress_rls_schema | uaccount | table | regress_rls_alice=arwdDxtvz/regress_rls_alice+| | + | | | =r/regress_rls_alice | | (3 rows) \d document diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out index 0035d158b7..e0fb21b36e 100644 --- a/src/test/regress/expected/vacuum.out +++ b/src/test/regress/expected/vacuum.out @@ -336,7 +336,9 @@ WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; WARNING: permission denied to vacuum "vacowned_parted", skipping it WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to analyze "vacowned_part1", skipping it WARNING: permission denied to vacuum "vacowned_part2", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; @@ -358,6 +360,7 @@ ANALYZE vacowned_part2; WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; WARNING: permission denied to vacuum "vacowned_part2", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; WARNING: permission denied to vacuum "vacowned_part2", skipping it @@ -380,6 +383,7 @@ WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; WARNING: permission denied to vacuum "vacowned_parted", skipping it WARNING: permission denied to vacuum "vacowned_part2", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; VACUUM (ANALYZE) vacowned_part2; WARNING: permission denied to vacuum "vacowned_part2", skipping it @@ -404,7 +408,9 @@ ANALYZE vacowned_part2; WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_parted; WARNING: permission denied to vacuum "vacowned_part1", skipping it +WARNING: permission denied to analyze "vacowned_part1", skipping it WARNING: permission denied to vacuum "vacowned_part2", skipping it +WARNING: permission denied to analyze "vacowned_part2", skipping it VACUUM (ANALYZE) vacowned_part1; WARNING: permission denied to vacuum "vacowned_part1", skipping it VACUUM (ANALYZE) vacowned_part2; diff --git a/src/test/regress/sql/dependency.sql b/src/test/regress/sql/dependency.sql index 2559c62d0b..99b905a938 100644 --- a/src/test/regress/sql/dependency.sql +++ b/src/test/regress/sql/dependency.sql @@ -21,7 +21,7 @@ REVOKE SELECT ON deptest FROM GROUP regress_dep_group; DROP GROUP regress_dep_group; -- can't drop the user if we revoke the privileges partially -REVOKE SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES ON deptest FROM regress_dep_user; +REVOKE SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, VACUUM, ANALYZE ON deptest FROM regress_dep_user; DROP USER regress_dep_user; -- now we are OK to drop him diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index 4ad366470d..a8ebcc8b85 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -1813,3 +1813,43 @@ DROP ROLE regress_group; DROP ROLE regress_group_direct_manager; DROP ROLE regress_group_indirect_manager; DROP ROLE regress_group_member; + +-- VACUUM and ANALYZE +CREATE ROLE regress_no_priv; +CREATE ROLE regress_only_vacuum; +CREATE ROLE regress_only_analyze; +CREATE ROLE regress_both; + +CREATE TABLE vacanalyze_test (a INT); +GRANT VACUUM ON vacanalyze_test TO regress_only_vacuum, regress_both; +GRANT ANALYZE ON vacanalyze_test TO regress_only_analyze, regress_both; + +SET ROLE regress_no_priv; +VACUUM vacanalyze_test; +ANALYZE vacanalyze_test; +VACUUM (ANALYZE) vacanalyze_test; +RESET ROLE; + +SET ROLE regress_only_vacuum; +VACUUM vacanalyze_test; +ANALYZE vacanalyze_test; +VACUUM (ANALYZE) vacanalyze_test; +RESET ROLE; + +SET ROLE regress_only_analyze; +VACUUM vacanalyze_test; +ANALYZE vacanalyze_test; +VACUUM (ANALYZE) vacanalyze_test; +RESET ROLE; + +SET ROLE regress_both; +VACUUM vacanalyze_test; +ANALYZE vacanalyze_test; +VACUUM (ANALYZE) vacanalyze_test; +RESET ROLE; + +DROP TABLE vacanalyze_test; +DROP ROLE regress_no_priv; +DROP ROLE regress_only_vacuum; +DROP ROLE regress_only_analyze; +DROP ROLE regress_both; -- 2.25.1 --ZPt4rx8FFjLCG7dd Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v6-0004-Add-pg_vacuum_all_tables-and-pg_analyze_all_table.patch" From 7612285ce4547c30a86f80c8d547a12d5f1d8dc6 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Tue, 6 Sep 2022 10:32:11 -0700 Subject: [PATCH v6 4/4] Add pg_vacuum_all_tables and pg_analyze_all_tables roles. --- doc/src/sgml/ref/analyze.sgml | 10 +++++++--- doc/src/sgml/ref/vacuum.sgml | 10 +++++++--- doc/src/sgml/user-manag.sgml | 12 ++++++++++++ src/backend/catalog/aclchk.c | 20 +++++++++++++++++++ src/include/catalog/pg_authid.dat | 10 ++++++++++ src/test/regress/expected/privileges.out | 25 ++++++++++++++++++++++++ src/test/regress/sql/privileges.sql | 24 +++++++++++++++++++++++ 7 files changed, 105 insertions(+), 6 deletions(-) diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 400ea30cd0..16c0b886fd 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -148,12 +148,16 @@ ANALYZE [ VERBOSE ] [ table_and_columnsNotes - To analyze a table, one must ordinarily be the table's owner or a - superuser or have the ANALYZE privilege on the table. + To analyze a table, one must ordinarily have the ANALYZE + privilege on the table or be the table's owner, a superuser, or a role with + privileges of the + pg_analyze_all_tables + role. However, database owners are allowed to analyze all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide - ANALYZE can only be performed by a superuser.) + ANALYZE can only be performed by superusers and roles + with privileges of pg_analyze_all_tables.) ANALYZE will skip over any tables that the calling user does not have permission to analyze. diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 70c0d81346..9cd880ea34 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -356,12 +356,16 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ pg_vacuum_all_tables + role. However, database owners are allowed to vacuum all tables in their databases, except shared catalogs. (The restriction for shared catalogs means that a true database-wide - VACUUM can only be performed by a superuser.) + VACUUM can only be performed by superusers and roles + with privileges of pg_vacuum_all_tables.) VACUUM will skip over any tables that the calling user does not have permission to vacuum. diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml index fc836d5748..b59ee37191 100644 --- a/doc/src/sgml/user-manag.sgml +++ b/doc/src/sgml/user-manag.sgml @@ -625,6 +625,18 @@ DROP ROLE doomed_role; the CHECKPOINT command. + + pg_vacuum_all_tables + Allow executing the + VACUUM command on + all tables. + + + pg_analyze_all_tables + Allow executing the + ANALYZE command on + all tables. + diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 9c9f44e42d..9d9ad0c6cc 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -4107,6 +4107,26 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask, has_privs_of_role(roleid, ROLE_PG_WRITE_ALL_DATA)) result |= (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)); + /* + * Check if ACL_VACUUM is being checked and, if so, and not already set as + * part of the result, then check if the user is a member of the + * pg_vacuum_all_tables role, which allows VACUUM on all relations. + */ + if (mask & ACL_VACUUM && + !(result & ACL_VACUUM) && + has_privs_of_role(roleid, ROLE_PG_VACUUM_ALL_TABLES)) + result |= ACL_VACUUM; + + /* + * Check if ACL_ANALYZE is being checked and, if so, and not already set as + * part of the result, then check if the user is a member of the + * pg_analyze_all_tables role, which allows ANALYZE on all relations. + */ + if (mask & ACL_ANALYZE && + !(result & ACL_ANALYZE) && + has_privs_of_role(roleid, ROLE_PG_ANALYZE_ALL_TABLES)) + result |= ACL_ANALYZE; + return result; } diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat index 3343a69ddb..2574e2906d 100644 --- a/src/include/catalog/pg_authid.dat +++ b/src/include/catalog/pg_authid.dat @@ -84,5 +84,15 @@ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', rolpassword => '_null_', rolvaliduntil => '_null_' }, +{ oid => '4549', oid_symbol => 'ROLE_PG_VACUUM_ALL_TABLES', + rolname => 'pg_vacuum_all_tables', rolsuper => 'f', rolinherit => 't', + rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', + rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', + rolpassword => '_null_', rolvaliduntil => '_null_' }, +{ oid => '4550', oid_symbol => 'ROLE_PG_ANALYZE_ALL_TABLES', + rolname => 'pg_analyze_all_tables', rolsuper => 'f', rolinherit => 't', + rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', + rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', + rolpassword => '_null_', rolvaliduntil => '_null_' }, ] diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index 023bf75161..54bf44b226 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -2814,6 +2814,9 @@ CREATE ROLE regress_no_priv; CREATE ROLE regress_only_vacuum; CREATE ROLE regress_only_analyze; CREATE ROLE regress_both; +CREATE ROLE regress_only_vacuum_all IN ROLE pg_vacuum_all_tables; +CREATE ROLE regress_only_analyze_all IN ROLE pg_analyze_all_tables; +CREATE ROLE regress_both_all IN ROLE pg_vacuum_all_tables, pg_analyze_all_tables; CREATE TABLE vacanalyze_test (a INT); GRANT VACUUM ON vacanalyze_test TO regress_only_vacuum, regress_both; GRANT ANALYZE ON vacanalyze_test TO regress_only_analyze, regress_both; @@ -2844,8 +2847,30 @@ VACUUM vacanalyze_test; ANALYZE vacanalyze_test; VACUUM (ANALYZE) vacanalyze_test; RESET ROLE; +SET ROLE regress_only_vacuum_all; +VACUUM vacanalyze_test; +ANALYZE vacanalyze_test; +WARNING: permission denied to analyze "vacanalyze_test", skipping it +VACUUM (ANALYZE) vacanalyze_test; +WARNING: permission denied to analyze "vacanalyze_test", skipping it +RESET ROLE; +SET ROLE regress_only_analyze_all; +VACUUM vacanalyze_test; +WARNING: permission denied to vacuum "vacanalyze_test", skipping it +ANALYZE vacanalyze_test; +VACUUM (ANALYZE) vacanalyze_test; +WARNING: permission denied to vacuum "vacanalyze_test", skipping it +RESET ROLE; +SET ROLE regress_both_all; +VACUUM vacanalyze_test; +ANALYZE vacanalyze_test; +VACUUM (ANALYZE) vacanalyze_test; +RESET ROLE; DROP TABLE vacanalyze_test; DROP ROLE regress_no_priv; DROP ROLE regress_only_vacuum; DROP ROLE regress_only_analyze; DROP ROLE regress_both; +DROP ROLE regress_only_vacuum_all; +DROP ROLE regress_only_analyze_all; +DROP ROLE regress_both_all; diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index a8ebcc8b85..28538444ce 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -1819,6 +1819,9 @@ CREATE ROLE regress_no_priv; CREATE ROLE regress_only_vacuum; CREATE ROLE regress_only_analyze; CREATE ROLE regress_both; +CREATE ROLE regress_only_vacuum_all IN ROLE pg_vacuum_all_tables; +CREATE ROLE regress_only_analyze_all IN ROLE pg_analyze_all_tables; +CREATE ROLE regress_both_all IN ROLE pg_vacuum_all_tables, pg_analyze_all_tables; CREATE TABLE vacanalyze_test (a INT); GRANT VACUUM ON vacanalyze_test TO regress_only_vacuum, regress_both; @@ -1848,8 +1851,29 @@ ANALYZE vacanalyze_test; VACUUM (ANALYZE) vacanalyze_test; RESET ROLE; +SET ROLE regress_only_vacuum_all; +VACUUM vacanalyze_test; +ANALYZE vacanalyze_test; +VACUUM (ANALYZE) vacanalyze_test; +RESET ROLE; + +SET ROLE regress_only_analyze_all; +VACUUM vacanalyze_test; +ANALYZE vacanalyze_test; +VACUUM (ANALYZE) vacanalyze_test; +RESET ROLE; + +SET ROLE regress_both_all; +VACUUM vacanalyze_test; +ANALYZE vacanalyze_test; +VACUUM (ANALYZE) vacanalyze_test; +RESET ROLE; + DROP TABLE vacanalyze_test; DROP ROLE regress_no_priv; DROP ROLE regress_only_vacuum; DROP ROLE regress_only_analyze; DROP ROLE regress_both; +DROP ROLE regress_only_vacuum_all; +DROP ROLE regress_only_analyze_all; +DROP ROLE regress_both_all; -- 2.25.1 --ZPt4rx8FFjLCG7dd--