Re: predefined role(s) for VACUUM and ANALYZE
On Tue, Dec 06, 2022 at 11:47:50AM +, Dagfinn Ilmari Mannsåker wrote:
> These checks are getting rather repetitive, how about a data-driven
> approach, along the lines of the below patch? I'm not quite happy with
> the naming of the struct and its members (and maybe it should be in a
> header?), suggestions welcome.
+1. I wonder if we should also consider checking all the bits at once
before we start checking for the predefined roles. I'm thinking of
something a bit like this:
role_mask = ACL_SELECT | ACL_INSERT | ACL_UPDATE |
ACL_DELETE | ACL_VACUUM | ACL_ANALYZE;
if (mask & role_mask != result & role_mask)
{
... existing checks here ...
}
I'm skeptical this actually produces any measurable benefit, but presumably
the predefined roles list will continue to grow, so maybe it's still worth
adding a fast path.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On Tue, Dec 06, 2022 at 04:57:37PM +0300, Pavel Luzanov wrote: > On 06.12.2022 03:04, Nathan Bossart wrote: >> I wonder why \dpS wasn't added. I wrote up a patch to add it and the >> corresponding documentation that other meta-commands already have. > > Yes, \dpS command and clarification in the documentation is exactly what is > needed. I created a new thread for this: https://postgr.es/m/20221206193606.GB3078082%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On 06.12.2022 03:04, Nathan Bossart wrote: I wonder why \dpS wasn't added. I wrote up a patch to add it and the corresponding documentation that other meta-commands already have. Yes, \dpS command and clarification in the documentation is exactly what is needed. -- Pavel Luzanov Postgres Professional: https://postgrespro.com
Re: predefined role(s) for VACUUM and ANALYZE
Nathan Bossart writes:
> diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
> index 3b5ea3c137..bd967eaa78 100644
> --- a/src/backend/catalog/aclchk.c
> +++ b/src/backend/catalog/aclchk.c
> @@ -4202,6 +4202,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;
> }
These checks are getting rather repetitive, how about a data-driven
approach, along the lines of the below patch? I'm not quite happy with
the naming of the struct and its members (and maybe it should be in a
header?), suggestions welcome.
- ilmari
>From 34bac3aced60931b2e995c5e1e6269f40c0828f5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?=
Date: Thu, 1 Dec 2022 11:49:14 +
Subject: [PATCH] Make built-in role permission checking data-driven
---
src/backend/catalog/aclchk.c | 64 +---
src/tools/pgindent/typedefs.list | 1 +
2 files changed, 27 insertions(+), 38 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index bd967eaa78..434bd39124 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -4084,6 +4084,22 @@ pg_class_aclmask(Oid table_oid, Oid roleid,
return pg_class_aclmask_ext(table_oid, roleid, mask, how, NULL);
}
+/*
+ * Actions that built-in roles can perform unconditionally
+ */
+typedef struct RoleAcl
+{
+ Oid role;
+ AclMode mode;
+} RoleAcl;
+
+static const RoleAcl builtin_role_acls[] = {
+ {ROLE_PG_READ_ALL_DATA, ACL_SELECT},
+ {ROLE_PG_WRITE_ALL_DATA, ACL_INSERT | ACL_UPDATE | ACL_DELETE},
+ {ROLE_PG_VACUUM_ALL_TABLES, ACL_VACUUM},
+ {ROLE_PG_ANALYZE_ALL_TABLES, ACL_ANALYZE},
+};
+
/*
* Routine for examining a user's privileges for a table
*
@@ -4182,45 +4198,17 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
ReleaseSysCache(tuple);
/*
- * Check if ACL_SELECT is being checked and, if so, and not set already as
- * part of the result, then check if the user is a member of the
- * pg_read_all_data role, which allows read access to all relations.
+ * For each built-in role, check if its permissions are being checked and,
+ * if so, and not set already as part of the result, then check if the
+ * user is a member of the role, and allow the action if so.
*/
- if (mask & ACL_SELECT && !(result & ACL_SELECT) &&
- has_privs_of_role(roleid, ROLE_PG_READ_ALL_DATA))
- result |= ACL_SELECT;
-
- /*
- * Check if ACL_INSERT, ACL_UPDATE, or ACL_DELETE is being checked and, if
- * so, and not set already as part of the result, then check if the user
- * is a member of the pg_write_all_data role, which allows
- * INSERT/UPDATE/DELETE access to all relations (except system catalogs,
- * which requires superuser, see above).
- */
- if (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE) &&
- !(result & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)) &&
- 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;
+ for (int i = 0; i < lengthof(builtin_role_acls); i++)
+ {
+ const RoleAcl *const builtin = _role_acls[i];
+ if (mask & builtin->mode && !(result & builtin->mode) &&
+ has_privs_of_role(roleid,
Re: predefined role(s) for VACUUM and ANALYZE
On Mon, Dec 05, 2022 at 11:21:08PM +0300, Pavel Luzanov wrote:
> But perhaps this behavior should be reviewed or at least documented?
I wonder why \dpS wasn't added. I wrote up a patch to add it and the
corresponding documentation that other meta-commands already have.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d3dd638b14..406936dd1c 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1825,14 +1825,16 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
-\dp [ pattern ]
+\dp[S] [ pattern ]
Lists tables, views and sequences with their
associated access privileges.
If pattern is
specified, only tables, views and sequences whose names match the
-pattern are listed.
+pattern are listed. By default only user-created objects are shown;
+supply a pattern or the S modifier to include system
+objects.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index de6a3a71f8..3520655dc0 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -875,7 +875,7 @@ exec_command_d(PsqlScanState scan_state, bool active_branch, const char *cmd)
success = listCollations(pattern, show_verbose, show_system);
break;
case 'p':
-success = permissionsList(pattern);
+success = permissionsList(pattern, show_system);
break;
case 'P':
{
@@ -2831,7 +2831,7 @@ exec_command_z(PsqlScanState scan_state, bool active_branch)
char *pattern = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
- success = permissionsList(pattern);
+ success = permissionsList(pattern, false);
free(pattern);
}
else
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 2eae519b1d..eb98797d67 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1002,7 +1002,7 @@ listAllDbs(const char *pattern, bool verbose)
* \z (now also \dp -- perhaps more mnemonic)
*/
bool
-permissionsList(const char *pattern)
+permissionsList(const char *pattern, bool showSystem)
{
PQExpBufferData buf;
PGresult *res;
@@ -1121,15 +1121,12 @@ permissionsList(const char *pattern)
CppAsString2(RELKIND_FOREIGN_TABLE) ","
CppAsString2(RELKIND_PARTITIONED_TABLE) ")\n");
- /*
- * Unless a schema pattern is specified, we suppress system and temp
- * tables, since they normally aren't very interesting from a permissions
- * point of view. You can see 'em by explicit request though, eg with \z
- * pg_catalog.*
- */
+ if (!showSystem && !pattern)
+ appendPQExpBufferStr(, "AND n.nspname !~ '^pg_'\n");
+
if (!validateSQLNamePattern(, pattern, true, false,
"n.nspname", "c.relname", NULL,
-"n.nspname !~ '^pg_' AND pg_catalog.pg_table_is_visible(c.oid)",
+"pg_catalog.pg_table_is_visible(c.oid)",
NULL, 3))
goto error_return;
diff --git a/src/bin/psql/describe.h b/src/bin/psql/describe.h
index bd051e09cb..58d0cf032b 100644
--- a/src/bin/psql/describe.h
+++ b/src/bin/psql/describe.h
@@ -38,7 +38,7 @@ extern bool describeRoles(const char *pattern, bool verbose, bool showSystem);
extern bool listDbRoleSettings(const char *pattern, const char *pattern2);
/* \z (or \dp) */
-extern bool permissionsList(const char *pattern);
+extern bool permissionsList(const char *pattern, bool showSystem);
/* \ddp */
extern bool listDefaultACLs(const char *pattern);
Re: predefined role(s) for VACUUM and ANALYZE
Hello,
While looking into the new feature, I found the following situation with
the \dp command displaying privileges on the system tables:
GRANT VACUUM, ANALYZE ON TABLE pg_type TO alice;
SELECT relacl FROM pg_class WHERE oid = 'pg_type'::regclass;
relacl
-
{=r/postgres,postgres=arwdDxtvz/postgres,alice=vz/postgres}
(1 row)
But the \dp command does not show the granted privileges:
\dp pg_type
Access privileges
Schema | Name | Type | Access privileges | Column privileges | Policies
+--+--+---+---+--
(0 rows)
The comment in src/bin/psql/describe.c explains the situation:
/*
* Unless a schema pattern is specified, we suppress system and temp
* tables, since they normally aren't very interesting from a
permissions
* point of view. You can see 'em by explicit request though, eg
with \z
* pg_catalog.*
*/
So to see the privileges you have to explicitly specify the schema name:
\dp pg_catalog.pg_type
Access privileges
Schema | Name | Type | Access privileges | Column
privileges | Policies
+-+---+-+---+--
pg_catalog | pg_type | table | =r/postgres +| |
| | |
postgres=arwdDxtvz/postgres+| |
| | | alice=vz/postgres | |
(1 row)
But perhaps this behavior should be reviewed or at least documented?
-
Pavel Luzanov
Re: predefined role(s) for VACUUM and ANALYZE
On Mon, Nov 28, 2022 at 12:13:13PM -0500, Andrew Dunstan wrote: > pushed. Thanks! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On 2022-11-23 We 18:54, Nathan Bossart wrote: > On Wed, Nov 23, 2022 at 02:56:28PM -0500, Andrew Dunstan wrote: >> I have committed the first couple of these to get them out of the way. > Thanks! > >> But I think we need a bit of cleanup in the next patch. >> vacuum_is_relation_owner() looks like it's now rather misnamed. Maybe >> vacuum_is_permitted_for_relation()? Also I think we need a more thorough >> reworking of the comments around line 566. And I think we need a more >> detailed explanation of why the change in vacuum_rel is ok, and if it is >> OK we should adjust the head comment on the function. >> >> In any case I think this comment would be better English with "might" >> instead of "may": >> >> /* user may have the ANALYZE privilege */ > I've attempted to address all your feedback in v13. Please let me know if > anything needs further reworking. Thanks, pushed. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: predefined role(s) for VACUUM and ANALYZE
On Wed, Nov 23, 2022 at 02:56:28PM -0500, Andrew Dunstan wrote: > I have committed the first couple of these to get them out of the way. Thanks! > But I think we need a bit of cleanup in the next patch. > vacuum_is_relation_owner() looks like it's now rather misnamed. Maybe > vacuum_is_permitted_for_relation()? Also I think we need a more thorough > reworking of the comments around line 566. And I think we need a more > detailed explanation of why the change in vacuum_rel is ok, and if it is > OK we should adjust the head comment on the function. > > In any case I think this comment would be better English with "might" > instead of "may": > > /* user may have the ANALYZE privilege */ I've attempted to address all your feedback in v13. Please let me know if anything needs further reworking. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From 09e6cb01a6d89eb952653c9700b0384e3a040f7e Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Sat, 3 Sep 2022 23:31:38 -0700 Subject: [PATCH v13 1/2] 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| 13 ++- src/backend/commands/vacuum.c | 62 ++--- 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 | 5 +- src/include/commands/vacuum.h | 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 + 24 files changed, 274 insertions(+), 113 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
Re: predefined role(s) for VACUUM and ANALYZE
On 2022-11-20 Su 11:57, Nathan Bossart wrote: > On Sat, Nov 19, 2022 at 10:50:04AM -0800, Nathan Bossart wrote: >> another rebase > Another rebase for cfbot. > I have committed the first couple of these to get them out of the way. But I think we need a bit of cleanup in the next patch. vacuum_is_relation_owner() looks like it's now rather misnamed. Maybe vacuum_is_permitted_for_relation()? Also I think we need a more thorough reworking of the comments around line 566. And I think we need a more detailed explanation of why the change in vacuum_rel is ok, and if it is OK we should adjust the head comment on the function. In any case I think this comment would be better English with "might" instead of "may": /* user may have the ANALYZE privilege */ cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: predefined role(s) for VACUUM and ANALYZE
On Sat, Nov 19, 2022 at 10:50:04AM -0800, Nathan Bossart wrote:
> another rebase
Another rebase for cfbot.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 5f716f33e93187491686381b2180894ab2b1b92c Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 7 Sep 2022 22:25:29 -0700
Subject: [PATCH v12 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 | 4 ++--
src/include/nodes/parsenodes.h | 6 +++---
src/include/utils/acl.h | 28 +-
5 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index f05e72f0dc..8f150e9a2e 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -560,7 +560,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(_cluster);
check_for_isn_and_int8_passing_mismatch(_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(_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..0763dfde39 100644
--- a/src/include/catalog/pg_type.dat
+++ b/src/include/catalog/pg_type.dat
@@ -267,9 +267,9 @@
# 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' },
+ typsend => '-', typalign => 'd' },
{ oid => '1042', array_type_oid => '1014',
descr => 'char(length), blank-padded string, fixed storage length',
typname => 'bpchar', typlen => '-1', typbyval => 'f', typcategory => 'S',
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 7caff62af7..f4ed9bbff9 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -73,12 +73,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.
Re: predefined role(s) for VACUUM and ANALYZE
On Fri, Nov 18, 2022 at 09:05:04AM -0800, Nathan Bossart wrote:
> rebased
another rebase
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 6e2001411107790991037e91f8d2f9411e2f4fa6 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 7 Sep 2022 22:25:29 -0700
Subject: [PATCH v11 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 | 4 ++--
src/include/nodes/parsenodes.h | 6 +++---
src/include/utils/acl.h | 28 +-
5 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index f05e72f0dc..8f150e9a2e 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -560,7 +560,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(_cluster);
check_for_isn_and_int8_passing_mismatch(_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(_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..0763dfde39 100644
--- a/src/include/catalog/pg_type.dat
+++ b/src/include/catalog/pg_type.dat
@@ -267,9 +267,9 @@
# 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' },
+ typsend => '-', typalign => 'd' },
{ oid => '1042', array_type_oid => '1014',
descr => 'char(length), blank-padded string, fixed storage length',
typname => 'bpchar', typlen => '-1', typbyval => 'f', typcategory => 'S',
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 7caff62af7..f4ed9bbff9 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -73,12 +73,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
Re: predefined role(s) for VACUUM and ANALYZE
rebased
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 0643a4dbc9f36d9fd383ef0cfebef13875237718 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 7 Sep 2022 22:25:29 -0700
Subject: [PATCH v10 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 | 4 ++--
src/include/nodes/parsenodes.h | 6 +++---
src/include/utils/acl.h | 28 +-
5 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index f05e72f0dc..8f150e9a2e 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -560,7 +560,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(_cluster);
check_for_isn_and_int8_passing_mismatch(_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(_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..0763dfde39 100644
--- a/src/include/catalog/pg_type.dat
+++ b/src/include/catalog/pg_type.dat
@@ -267,9 +267,9 @@
# 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' },
+ typsend => '-', typalign => 'd' },
{ oid => '1042', array_type_oid => '1014',
descr => 'char(length), blank-padded string, fixed storage length',
typname => 'bpchar', typlen => '-1', typbyval => 'f', typcategory => 'S',
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 7caff62af7..f4ed9bbff9 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -73,12 +73,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
Re: predefined role(s) for VACUUM and ANALYZE
On Wed, Nov 16, 2022 at 03:09:47PM -0500, Andrew Dunstan wrote: > OK, reading the history I think everyone is on board with expanding > AclMode from uint32 to uint64. Is that right? I skimmed through this thread again, and AFAICT folks are okay with this approach. I'm not aware of any remaining concerns. > If so I'm intending to > commit at least the first two of these patches fairly soon. Thanks! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On 2022-11-15 Tu 00:08, Nathan Bossart wrote: > On Mon, Nov 14, 2022 at 03:40:04PM -0800, Nathan Bossart wrote: >> Thanks for taking a look! Here is a rebased version of the patch set. > Oops, apparently object_aclcheck() cannot be used for pg_class. Here is > another version that uses pg_class_aclcheck() instead. I'm not sure how I > missed this earlier. > OK, reading the history I think everyone is on board with expanding AclMode from uint32 to uint64. Is that right? If so I'm intending to commit at least the first two of these patches fairly soon. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: predefined role(s) for VACUUM and ANALYZE
On Mon, Nov 14, 2022 at 03:40:04PM -0800, Nathan Bossart wrote:
> Thanks for taking a look! Here is a rebased version of the patch set.
Oops, apparently object_aclcheck() cannot be used for pg_class. Here is
another version that uses pg_class_aclcheck() instead. I'm not sure how I
missed this earlier.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 20df1ffd941753f20b710b14c1de5c4b52de3eca Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 7 Sep 2022 22:25:29 -0700
Subject: [PATCH v9 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 | 4 ++--
src/include/nodes/parsenodes.h | 6 +++---
src/include/utils/acl.h | 28 +-
5 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index f05e72f0dc..8f150e9a2e 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -560,7 +560,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(_cluster);
check_for_isn_and_int8_passing_mismatch(_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(_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..0763dfde39 100644
--- a/src/include/catalog/pg_type.dat
+++ b/src/include/catalog/pg_type.dat
@@ -267,9 +267,9 @@
# 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' },
+ typsend => '-', typalign => 'd' },
{ oid => '1042', array_type_oid => '1014',
descr => 'char(length), blank-padded string, fixed storage length',
typname => 'bpchar', typlen => '-1', typbyval => 'f', typcategory => 'S',
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 7caff62af7..f4ed9bbff9 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -73,12 +73,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
Re: predefined role(s) for VACUUM and ANALYZE
On Fri, Oct 14, 2022 at 07:37:38PM -0400, Corey Huinker wrote:
> Patch applies.
> Passes make check and make check-world.
> Test coverage seems adequate.
>
> Coding is very clear and very much in the style of the existing code. Any
> quibbles I have with the coding style are ones I have with the overall
> pg-style, and this isn't the forum for that.
>
> I haven't done any benchmarking yet, but it seems that the main question
> will be the impact on ordinary DML statements.
>
> I have no opinion about the design debate earlier in this thread, but I do
> think that this patch is ready and adds something concrete to the ongoing
> discussion.
Thanks for taking a look! Here is a rebased version of the patch set.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From eb6d4f8bb2bb055ec161767bbec741e375314dcb Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 7 Sep 2022 22:25:29 -0700
Subject: [PATCH v8 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 | 4 ++--
src/include/nodes/parsenodes.h | 6 +++---
src/include/utils/acl.h | 28 +-
5 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index f05e72f0dc..8f150e9a2e 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -560,7 +560,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(_cluster);
check_for_isn_and_int8_passing_mismatch(_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(_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..0763dfde39 100644
--- a/src/include/catalog/pg_type.dat
+++ b/src/include/catalog/pg_type.dat
@@ -267,9 +267,9 @@
# 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' },
+ typsend => '-', typalign => 'd' },
{ oid => '1042', array_type_oid => '1014',
descr => 'char(length), blank-padded string, fixed storage length',
typname => 'bpchar', typlen => '-1', typbyval => 'f', typcategory => 'S',
diff --git a/src/include/nodes/parsenodes.h
Re: predefined role(s) for VACUUM and ANALYZE
> > Sounds good. Here's a new patch set with aclitem's typalign fixed. > Patch applies. Passes make check and make check-world. Test coverage seems adequate. Coding is very clear and very much in the style of the existing code. Any quibbles I have with the coding style are ones I have with the overall pg-style, and this isn't the forum for that. I haven't done any benchmarking yet, but it seems that the main question will be the impact on ordinary DML statements. I have no opinion about the design debate earlier in this thread, but I do think that this patch is ready and adds something concrete to the ongoing discussion.
Re: predefined role(s) for VACUUM and ANALYZE
On Fri, Sep 30, 2022 at 07:05:38PM -0400, Tom Lane wrote:
> Nathan Bossart writes:
>> On Fri, Sep 30, 2022 at 06:00:53PM -0400, Tom Lane wrote:
>>> ... and now requires double alignment ... did you fix its typalign?
>
>> Nope, I missed that, thanks for pointing it out. Should we move ai_privs
>> to the beginning of the struct, too?
>
> Don't see any point, there won't be any padding. If we ever change the
> sizeof(Oid), or add more fields, we can consider what to do then.
Sounds good. Here's a new patch set with aclitem's typalign fixed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 6aa726fc323278066f3c1be81ef8a94a0a79ff63 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Wed, 7 Sep 2022 22:25:29 -0700
Subject: [PATCH v7 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 | 4 ++--
src/include/nodes/parsenodes.h | 6 +++---
src/include/utils/acl.h | 28 +-
5 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 64c65f060b..346dc45e4f 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -557,7 +557,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(_cluster);
check_for_isn_and_int8_passing_mismatch(_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(_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..0763dfde39 100644
--- a/src/include/catalog/pg_type.dat
+++ b/src/include/catalog/pg_type.dat
@@ -267,9 +267,9 @@
# 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' },
+ typsend => '-', typalign => 'd' },
{ oid => '1042', array_type_oid => '1014',
descr => 'char(length), blank-padded string, fixed storage length',
typname => 'bpchar', typlen => '-1', typbyval => 'f', typcategory => 'S',
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 633e7671b3..9693c5c889 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -73,12 +73,12 @@ typedef enum SetQuantifier
Re: predefined role(s) for VACUUM and ANALYZE
Nathan Bossart writes: > On Fri, Sep 30, 2022 at 06:00:53PM -0400, Tom Lane wrote: >> ... and now requires double alignment ... did you fix its typalign? > Nope, I missed that, thanks for pointing it out. Should we move ai_privs > to the beginning of the struct, too? Don't see any point, there won't be any padding. If we ever change the sizeof(Oid), or add more fields, we can consider what to do then. regards, tom lane
Re: predefined role(s) for VACUUM and ANALYZE
On Fri, Sep 30, 2022 at 06:00:53PM -0400, Tom Lane wrote: > Nathan Bossart writes: >> The main one I see is AclItem, which increases from 12 bytes to 16 bytes. > > ... and now requires double alignment ... did you fix its typalign? Nope, I missed that, thanks for pointing it out. Should we move ai_privs to the beginning of the struct, too? The only other similar example I see is TimeTzADT, but that only consists of an int64 and an int32, while AclItem starts with 2 uint32s. While it might not be strictly necessary, it seems like there is a small chance it could become necessary in the future. > We could conceivably dodge the alignment increase by splitting the 64-bit > field into two 32-bit fields, one for base privileges and one for grant > options. That'd be rather invasive, so unless it leads to pleasant > improvements in readability (which it might, perhaps) I wouldn't advocate > for it. Yeah, the invasiveness is the main reason I haven't tried this yet, but it does seem like it'd improve readability. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
Nathan Bossart writes: > The main one I see is AclItem, which increases from 12 bytes to 16 bytes. ... and now requires double alignment ... did you fix its typalign? We could conceivably dodge the alignment increase by splitting the 64-bit field into two 32-bit fields, one for base privileges and one for grant options. That'd be rather invasive, so unless it leads to pleasant improvements in readability (which it might, perhaps) I wouldn't advocate for it. regards, tom lane
Re: predefined role(s) for VACUUM and ANALYZE
On Fri, Sep 30, 2022 at 04:15:24PM -0400, Tom Lane wrote: > In view of the recent mess around bigint relfilenodes, it seems to me > that we shouldn't move forward with widening AclMode unless somebody > runs down which structs will get wider (or more aligned) and how much > that'll cost us. Maybe it's not a problem, but it could do with an > explicit look at the point. The main one I see is AclItem, which increases from 12 bytes to 16 bytes. AFAICT all of the catalogs that store aclitem arrays have the aclitem[] column marked extended, so they are compressed or moved out-of-line as needed, too. The only other structs I've spotted that make use of AclMode are InternalGrant and InternalDefaultACL. I haven't identified anything that leads me to believe there are alignment problems or anything else comparable to the issues listed in the relfilenode thread [0], but I could be missing something. Did you have something else in mind you think ought to be checked? I'm not sure my brief analysis here suffices. [0] https://postgr.es/m/CA%2BTgmoaa9Yc9O-FP4vS_xTKf8Wgy8TzHpjnjN56_ShKE%3DjrP-Q%40mail.gmail.com -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
Nathan Bossart writes: > Are there any remaining concerns about this approach? I'm happy to do any > testing that folks deem necessary, or anything else really that might help > move this patch set forward. If we don't want to extend AclMode right > away, we could also keep it in our back pocket for the next time someone > (which may very well be me) wants to add privileges. That is, 0001 is not > fundamentally a prerequisite for 0002-0004, but I recognize that freeing up > some extra bits would be the most courteous. In view of the recent mess around bigint relfilenodes, it seems to me that we shouldn't move forward with widening AclMode unless somebody runs down which structs will get wider (or more aligned) and how much that'll cost us. Maybe it's not a problem, but it could do with an explicit look at the point. I do agree with the position that these features are not where to spend our last remaining privilege bits. regards, tom lane
Re: predefined role(s) for VACUUM and ANALYZE
On Wed, Sep 28, 2022 at 01:12:22PM -0700, Nathan Bossart wrote: > On Wed, Sep 28, 2022 at 03:09:46PM -0400, Stephen Frost wrote: >> The max is the same regardless of the size..? Considering the size is >> capped since pg_class doesn’t (and isn’t likely to..) have a toast table, >> that seems unlikely, so I’m asking for clarification on that. We may be >> able to get consensus that the difference isn’t material since no one is >> likely to have such long lists, but we should at least be aware. > > While pg_class doesn't have a TOAST table, that column is marked as > "extended," so I believe it is still compressed, and the maximum aclitem > array length for pg_class.relacl would depend on how well the array > compresses. Are there any remaining concerns about this approach? I'm happy to do any testing that folks deem necessary, or anything else really that might help move this patch set forward. If we don't want to extend AclMode right away, we could also keep it in our back pocket for the next time someone (which may very well be me) wants to add privileges. That is, 0001 is not fundamentally a prerequisite for 0002-0004, but I recognize that freeing up some extra bits would be the most courteous. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On Wed, Sep 28, 2022 at 03:09:46PM -0400, Stephen Frost wrote: > On Wed, Sep 28, 2022 at 14:50 Nathan Bossart > wrote: >> I've been testing aclmask() with long aclitem arrays (2,000 entries is >> close to the limit for pg_class entries), and I haven't found any >> significant impact from bumping AclMode to 64 bits. > > The max is the same regardless of the size..? Considering the size is > capped since pg_class doesn’t (and isn’t likely to..) have a toast table, > that seems unlikely, so I’m asking for clarification on that. We may be > able to get consensus that the difference isn’t material since no one is > likely to have such long lists, but we should at least be aware. While pg_class doesn't have a TOAST table, that column is marked as "extended," so I believe it is still compressed, and the maximum aclitem array length for pg_class.relacl would depend on how well the array compresses. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
Greetings, On Wed, Sep 28, 2022 at 14:50 Nathan Bossart wrote: > On Tue, Sep 20, 2022 at 09:31:26PM -0700, Nathan Bossart wrote: > > I bet a more pressing concern is the calls to aclmask() since checking > > privileges is probably done more frequently than updating them. That > > appears to use a linear search, too, so maybe sorting the aclitem arrays > is > > actually worth exploring. I still doubt there will be much noticeable > > impact from expanding AclMode outside of the most extreme cases. > > I've been testing aclmask() with long aclitem arrays (2,000 entries is > close to the limit for pg_class entries), and I haven't found any > significant impact from bumping AclMode to 64 bits. The max is the same regardless of the size..? Considering the size is capped since pg_class doesn’t (and isn’t likely to..) have a toast table, that seems unlikely, so I’m asking for clarification on that. We may be able to get consensus that the difference isn’t material since no one is likely to have such long lists, but we should at least be aware. Thanks, Stephen >
Re: predefined role(s) for VACUUM and ANALYZE
On Tue, Sep 20, 2022 at 09:31:26PM -0700, Nathan Bossart wrote: > I bet a more pressing concern is the calls to aclmask() since checking > privileges is probably done more frequently than updating them. That > appears to use a linear search, too, so maybe sorting the aclitem arrays is > actually worth exploring. I still doubt there will be much noticeable > impact from expanding AclMode outside of the most extreme cases. I've been testing aclmask() with long aclitem arrays (2,000 entries is close to the limit for pg_class entries), and I haven't found any significant impact from bumping AclMode to 64 bits. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On Wed, Sep 21, 2022 at 10:31:47AM +0900, Michael Paquier wrote: > Did you just run an aclupdate()? 4% for aclitem[] sounds like quite a > number to me :/ It may be worth looking at if these operations could > be locally optimized more, as well. I'd like to think that we could > live with that to free up enough bits in AclItems for the next 20 > years, anyway. Any opinions? Yes, the test was mostly for aclupdate(). Looking at that function, I bet most of its time is spent in palloc0() and memcpy(). It might be possible to replace the linear search if the array was sorted, but I'm skeptical that will help much. In the end, I'm not it's worth worrying too much about 2,000 calls to aclupdate() with an array of 2,000 ACLs taking 5.3 seconds instead of 5.1 seconds. I bet a more pressing concern is the calls to aclmask() since checking privileges is probably done more frequently than updating them. That appears to use a linear search, too, so maybe sorting the aclitem arrays is actually worth exploring. I still doubt there will be much noticeable impact from expanding AclMode outside of the most extreme cases. > For the column sizes of the catalogs, I was wondering about how > pg_column_size() changes when they hold ACL information. Unoptimized > alignment could cause an unnecessary increase in the structure sizes, > so the addition of new fields or changes in object size could have > unexpected side effects. After a few tests, I haven't discovered any changes to the output of pg_column_size(). -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On Tue, Sep 20, 2022 at 04:50:10PM -0700, Nathan Bossart wrote: > On Tue, Sep 20, 2022 at 04:31:17PM -0700, Nathan Bossart wrote: >> On Tue, Sep 20, 2022 at 11:05:33AM -0700, Nathan Bossart wrote: >>> On Tue, Sep 20, 2022 at 02:45:52PM +0900, Michael Paquier wrote: 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. >> >> Ah, it looks like relacl is compressed. The column is marked "extended," >> but pg_class doesn't appear to have a TOAST table, so presumably no >> out-of-line storage can be used. I found a couple of threads about this >> [0] [1] [2]. Adding a toast table to pg_class has been a sensitive topic over the years. Based on my recollection of the events, there were worries about the potential cross-dependencies with pg_class and pg_attribute that this would create. > I suppose there is some risk that folks with really long aclitem arrays > might be unable to pg_upgrade to a version with uint64 AclModes, but I > suspect that risk is limited to extreme cases (i.e., multiple thousands of > aclitems). I'm not sure whether that's worth worrying about too much. Did you just run an aclupdate()? 4% for aclitem[] sounds like quite a number to me :/ It may be worth looking at if these operations could be locally optimized more, as well. I'd like to think that we could live with that to free up enough bits in AclItems for the next 20 years, anyway. Any opinions? For the column sizes of the catalogs, I was wondering about how pg_column_size() changes when they hold ACL information. Unoptimized alignment could cause an unnecessary increase in the structure sizes, so the addition of new fields or changes in object size could have unexpected side effects. -- Michael signature.asc Description: PGP signature
Re: predefined role(s) for VACUUM and ANALYZE
On Tue, Sep 20, 2022 at 04:31:17PM -0700, Nathan Bossart wrote: > On Tue, Sep 20, 2022 at 11:05:33AM -0700, Nathan Bossart wrote: >> On Tue, Sep 20, 2022 at 02:45:52PM +0900, Michael Paquier wrote: >>> 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. > > Ah, it looks like relacl is compressed. The column is marked "extended," > but pg_class doesn't appear to have a TOAST table, so presumably no > out-of-line storage can be used. I found a couple of threads about this > [0] [1] [2]. I suppose there is some risk that folks with really long aclitem arrays might be unable to pg_upgrade to a version with uint64 AclModes, but I suspect that risk is limited to extreme cases (i.e., multiple thousands of aclitems). I'm not sure whether that's worth worrying about too much. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On Tue, Sep 20, 2022 at 11:05:33AM -0700, Nathan Bossart wrote: > On Tue, Sep 20, 2022 at 02:45:52PM +0900, Michael Paquier wrote: >> 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. Ah, it looks like relacl is compressed. The column is marked "extended," but pg_class doesn't appear to have a TOAST table, so presumably no out-of-line storage can be used. I found a couple of threads about this [0] [1] [2]. [0] https://postgr.es/m/17245.964897719%40sss.pgh.pa.us [1] https://postgr.es/m/200309040531.h845ViP05881%40candle.pha.pa.us [2] https://postgr.es/m/29061.1265327626%40sss.pgh.pa.us -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
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
>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(_cluster);
check_for_isn_and_int8_passing_mismatch(_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(_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
+++
Re: predefined role(s) for VACUUM and ANALYZE
On Mon, Sep 19, 2022 at 08:51:47PM -0700, Nathan Bossart wrote: > Are there any concerns with simply expanding AclMode to 64 bits, as done in > v5 [0]? > > [0] https://postgr.es/m/20220908055035.GA2100193%40nathanxps13 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. Any impact for the column sizes of the catalogs holding ACL information? Just asking while browsing the patch set. Some comments in utils/acl.h need a refresh as the number of lower and upper bits looked at from ai_privs changes. -- Michael signature.asc Description: PGP signature
Re: predefined role(s) for VACUUM and ANALYZE
On Thu, Sep 08, 2022 at 09:41:20AM -0400, Robert Haas wrote: > Now on the other hand, I also do think we need more privilege bits. > You're not alone in making the case that this is a problem which needs > to be solved, and the set of other people who are also making that > argument includes me. At the same time, there is certainly a double > standard here. When Andrew and Tom committed > d11e84ea466b4e3855d7bd5142fb68f51c273567 and > a0ffa885e478f5eeacc4e250e35ce25a4740c487 respectively, we used up 2 of > the remaining 4 bits, bits which other people would have liked to have > used up years ago and they were told "no you can't." I don't believe I > would have been willing to commit those patches without doing > something to solve this problem, because I would have been worried > about getting yelled at by Tom. But now here we are with only 2 bits > left instead of 4, and we're telling the next patch author - who is > not Tom - that he's on the hook to solve the problem. > > Well, we do need to solve the problem. But we're not necessarily being > fair about how the work involved gets distributed. It's a heck of a > lot easier for a committer to get something committed to address this > issue than a non-committer, and it's a heck of a lot easier for a > committer to ignore the fact that the problem hasn't been solved and > press ahead anyway, and yet somehow we're trying to dump a problem > that's a decade in the making on Nathan. I'm not exactly sure what to > propose as an alternative, but that doesn't seem quite fair. Are there any concerns with simply expanding AclMode to 64 bits, as done in v5 [0]? [0] https://postgr.es/m/20220908055035.GA2100193%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On Wed, Sep 07, 2022 at 04:15:23PM -0700, Mark Dilger wrote: > Ok, now I'm a bit lost. If I want to use Nathan's feature to create a role > to vacuum and analyze my database on a regular basis, how does per-relation > granularity help me? If somebody creates a new table and doesn't grant those > privileges to the role, doesn't that break the usage case? To me, > per-relation granularity sounds useful, but orthogonal, to this feature. I think there is room for both per-relation privileges and new predefined roles. My latest patch set [0] introduces both. [0] https://postgr.es/m/20220908055035.GA2100193%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On Wed, Sep 7, 2022 at 7:09 PM Stephen Frost wrote: > Calling this a redesign is over-stating things, imv … and I’d much rather > have the per-relation granularity than predefined roles for this, so there is > that to consider too, perhaps. I also prefer the finer granularity. On the question of whether freeing up more privilege bits is a prerequisite for this patch, I'm a bit on the fence about that. If I look at the amount of extra work that your review comments have caused me to do over, let's say, the last three years, and I compare that to the amount of extra work that the review comments of other people have caused me to do in the same period of time, you win. In fact, you win against all of them added together and doubled. I think that as a general matter you are far too willing to argue vigorously for people to do work that isn't closely related to their original goals, and which is at times even opposed to their original goals, and I think the project would be better off if you tempered that urge. Now on the other hand, I also do think we need more privilege bits. You're not alone in making the case that this is a problem which needs to be solved, and the set of other people who are also making that argument includes me. At the same time, there is certainly a double standard here. When Andrew and Tom committed d11e84ea466b4e3855d7bd5142fb68f51c273567 and a0ffa885e478f5eeacc4e250e35ce25a4740c487 respectively, we used up 2 of the remaining 4 bits, bits which other people would have liked to have used up years ago and they were told "no you can't." I don't believe I would have been willing to commit those patches without doing something to solve this problem, because I would have been worried about getting yelled at by Tom. But now here we are with only 2 bits left instead of 4, and we're telling the next patch author - who is not Tom - that he's on the hook to solve the problem. Well, we do need to solve the problem. But we're not necessarily being fair about how the work involved gets distributed. It's a heck of a lot easier for a committer to get something committed to address this issue than a non-committer, and it's a heck of a lot easier for a committer to ignore the fact that the problem hasn't been solved and press ahead anyway, and yet somehow we're trying to dump a problem that's a decade in the making on Nathan. I'm not exactly sure what to propose as an alternative, but that doesn't seem quite fair. -- Robert Haas EDB: http://www.enterprisedb.com
Re: predefined role(s) for VACUUM and ANALYZE
On Wed, Sep 07, 2022 at 07:09:05PM -0400, Stephen Frost wrote: > Yes, that seems to be the consensus among those involved in this thread > thus far. Basically, I imagine this involves passing around the object > type along with the acl info and then using that to check the bits and > such. I doubt it’s worth inventing a new structure to combine the two … > but that’s just gut feeling and you may find it does make sense to once you > get into it. I've done some preliminary research for this approach, and I've found some interesting challenges. * aclparse() will need to handle ambiguous strings. For example, USAGE is available for most catalogs, so which ACL bit should be chosen? One possible solution would be to make sure the common privilege types always use the same bit. * When comparing ACLs, there probably should be some way to differentiate overloaded privilege bits, else ACLs for different catalogs that have nothing in common could evaluate as equal. Such comparisons may be unlikely, but this still doesn't strike me as acceptable. * aclitemout() needs some way to determine what privilege an ACL bit actually refers to. I can think of a couple of ways to do this: 1) we could create different aclitem types for each catalog (or maybe just one for pg_class and another for everything else), or 2) we could include the type in AclItem, perhaps by adding a uint8 field. I noticed that Tom called out this particular challenge back in 2018 [0]. Am I overlooking an easier way to handle these things? From my admittedly brief analysis thus far, I'm worried this could devolve into something overly complex or magical, especially when simply moving to a uint64 might be a reasonable way to significantly extend AclItem's life span. Robert suggested upthread that Tom might have concerns with adding another 32 bits to AclItem, but the archives indicate he has previously proposed exactly that [1]. Of course, I don't know how everyone feels about the uint64 idea today, but ISTM like it might be the path of least resistance. So, here is a new patch set. 0001 expands AclMode to a uint64. 0002 simplifies some WARNING messages for VACUUM/ANALYZE. 0003 introduces privilege bits for VACUUM and ANALYZE on relations. And 0004 introduces the pg_vacuum/analyze_all_tables predefined roles. [0] https://postgr.es/m/18391.1521419120%40sss.pgh.pa.us [1] https://postgr.es/m/11414.1526422062%40sss.pgh.pa.us -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From d1ec82eeb12a15bb4f39bbf0d88ae32bd554418d Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Wed, 7 Sep 2022 22:25:29 -0700 Subject: [PATCH v5 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 | 24 +++--- 5 files changed, 52 insertions(+), 17 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 f4969bcdad..349f789e8b 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(_cluster); check_for_isn_and_int8_passing_mismatch(_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(_cluster); + /* * PG 14 changed the function signature of encoding conversion functions. * Conversions from older versions cannot be upgraded automatically @@ -1315,6 +1323,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
Re: predefined role(s) for VACUUM and ANALYZE
> On Sep 7, 2022, at 4:09 PM, Stephen Frost wrote: > > Calling this a redesign is over-stating things, imv … and I’d much rather > have the per-relation granularity than predefined roles for this, so there is > that to consider too, perhaps. Ok, now I'm a bit lost. If I want to use Nathan's feature to create a role to vacuum and analyze my database on a regular basis, how does per-relation granularity help me? If somebody creates a new table and doesn't grant those privileges to the role, doesn't that break the usage case? To me, per-relation granularity sounds useful, but orthogonal, to this feature. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: predefined role(s) for VACUUM and ANALYZE
Greetings, On Wed, Sep 7, 2022 at 18:11 Nathan Bossart wrote: > On Wed, Sep 07, 2022 at 05:13:44PM -0400, Stephen Frost wrote: > > I disagree that we should put the onus for addressing this on the next > > person who wants to add bits and just willfully use up the last of them > > right now for what strikes me, at least, as a relatively marginal use > > case. If we had plenty of bits then, sure, let's use a couple of for > > this, but that isn't currently the case. If you want this feature then > > the onus is on you to do the legwork to make it such that we have plenty > > of bits. > > FWIW what I really want is the new predefined roles. I received feedback > upthread that it might also make sense to give people more fine-grained > control, so I implemented that. And now you're telling me that I need to > redesign the ACL system. :) Calling this a redesign is over-stating things, imv … and I’d much rather have the per-relation granularity than predefined roles for this, so there is that to consider too, perhaps. I'm happy to give that project a try given there is agreement on the > direction and general interest in the patches. From the previous > discussion, it sounds like we want to first use a distinct set of bits for > each catalog table. Is that what I should proceed with? Yes, that seems to be the consensus among those involved in this thread thus far. Basically, I imagine this involves passing around the object type along with the acl info and then using that to check the bits and such. I doubt it’s worth inventing a new structure to combine the two … but that’s just gut feeling and you may find it does make sense to once you get into it. Thanks! Stephen >
Re: predefined role(s) for VACUUM and ANALYZE
> On Sep 7, 2022, at 3:21 PM, Nathan Bossart wrote: > > There was some previous discussion around adding a pg_maintenance role that > could perform all of these commands [0]. I didn't intend to draw a line > around VACUUM and ANALYZE. Those are just the commands I started with. > If/when there are many of these roles, it might make sense to create a > pg_maintenance role that is a member of pg_vacuum_all_tables, > pg_analyze_all_tables, etc. Thank you, that sounds fair enough. It seems you've been pushed to make the patch-set more complicated, and now we're debating privilege bits, which seems pretty far off topic. I may be preaching to the choir here, but wouldn't it work to commit new roles pg_vacuum_all_tables and pg_analyze_all_tables with checks like you had in the original patch of this thread? That wouldn't block the later addition of finer grained controls allowing users to grant VACUUM or ANALYZE per-relation, would it? Something like what Stephen is requesting, and what you did with new privilege bits for VACUUM and ANALYZE could still be added, unless I'm missing something. I'd hate to see your patch set get further delayed by things that aren't logically prerequisites. The conversation upthread was useful to determine that they aren't prerequisites, but if anybody wants to explain to me why they are — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: predefined role(s) for VACUUM and ANALYZE
On Wed, Sep 07, 2022 at 02:53:57PM -0700, Mark Dilger wrote: > Assuming for the sake of argument that we should create a role something like > you propose, can you explain why we should draw the line around just VACUUM > and ANALYZE? I am not arguing for including these other commands, but don't > want to regret having drawn the line in the wrong place when later we decide > to add more roles like the one you are proposing. There was some previous discussion around adding a pg_maintenance role that could perform all of these commands [0]. I didn't intend to draw a line around VACUUM and ANALYZE. Those are just the commands I started with. If/when there are many of these roles, it might make sense to create a pg_maintenance role that is a member of pg_vacuum_all_tables, pg_analyze_all_tables, etc. [0] https://postgr.es/m/67a1d667e8ec228b5e07f232184c80348c5d93f4.camel%40j-davis.com -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On Wed, Sep 07, 2022 at 05:13:44PM -0400, Stephen Frost wrote: > I disagree that we should put the onus for addressing this on the next > person who wants to add bits and just willfully use up the last of them > right now for what strikes me, at least, as a relatively marginal use > case. If we had plenty of bits then, sure, let's use a couple of for > this, but that isn't currently the case. If you want this feature then > the onus is on you to do the legwork to make it such that we have plenty > of bits. FWIW what I really want is the new predefined roles. I received feedback upthread that it might also make sense to give people more fine-grained control, so I implemented that. And now you're telling me that I need to redesign the ACL system. :) I'm happy to give that project a try given there is agreement on the direction and general interest in the patches. From the previous discussion, it sounds like we want to first use a distinct set of bits for each catalog table. Is that what I should proceed with? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
> On Jul 22, 2022, at 1:37 PM, Nathan Bossart wrote: > > The primary motivation for this is to continue chipping away at things that > require special privileges or even superuser. VACUUM and ANALYZE typically > require table ownership, database ownership, or superuser. And only > superusers can VACUUM/ANALYZE shared catalogs. A predefined role for these > operations would allow delegating such tasks (e.g., a nightly VACUUM > scheduled with pg_cron) to a role with fewer privileges. > > The attached patch adds a pg_vacuum_analyze role that allows VACUUM and > ANALYZE commands on all relations. Granting membership in a role that can VACUUM and ANALYZE any relation seems to grant a subset of a more general category, the ability to perform modifying administrative operations on a relation without necessarily being able to read or modify the logical contents of that relation. That more general category would seem to also include CLUSTER, REINDEX, REFRESH MATERIALIZED VIEW and more broadly ALTER SUBSCRIPTION ... REFRESH PUBLICATION and ALTER DATABASE ... REFRESH COLLATION VERSION. These latter operations may be less critical to database maintenance than is VACUUM, but arguably ANALYZE isn't as critical as is VACUUM, either. Assuming for the sake of argument that we should create a role something like you propose, can you explain why we should draw the line around just VACUUM and ANALYZE? I am not arguing for including these other commands, but don't want to regret having drawn the line in the wrong place when later we decide to add more roles like the one you are proposing. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: predefined role(s) for VACUUM and ANALYZE
Greetings, * Nathan Bossart (nathandboss...@gmail.com) wrote: > On Tue, Sep 06, 2022 at 11:24:18AM -0400, Robert Haas wrote: > > On Tue, Sep 6, 2022 at 11:11 AM Stephen Frost wrote: > >> If we were to make the specific bits depend on the object type as I'm > >> suggesting, then we'd have 8 bits used for relations (10 with the vacuum > >> and analyze bits), leaving us with 6 remaining inside the existing > >> uint32, or more bits available than we've ever used since the original > >> implementation from what I can tell, or at least 15+ years. That seems > >> like pretty darn good future-proofing without a lot of complication or > >> any change in physical size. We would also be able to get rid of the > >> question of "well, is it more valuable to add the ability to GRANT > >> TRUNCATE on a relation, or GRANT CONNECT on databases" or other rather > >> odd debates between ultimately very different things. > > > > I mostly agree with this. I don't think it's entirely clear how we > > should try to get more bits going forward, but it's clear that we > > cannot just forever hold our breath and refuse to find any more bits. > > And of the possible ways of doing it, this seems like the one with the > > lowest impact, so I think it likely makes sense to do this one first. > > +1. My earlier note wasn't intended to suggest that one approach was > better than the other, merely that there are a couple of options to choose > from once we run out of bits. I don't think this work needs to be tied to > the VACUUM/ANALYZE stuff, but I am interested in it and hope to take it on > at some point. I disagree that we should put the onus for addressing this on the next person who wants to add bits and just willfully use up the last of them right now for what strikes me, at least, as a relatively marginal use case. If we had plenty of bits then, sure, let's use a couple of for this, but that isn't currently the case. If you want this feature then the onus is on you to do the legwork to make it such that we have plenty of bits. My 2c anyway. Thanks, Stephen signature.asc Description: PGP signature
Re: predefined role(s) for VACUUM and ANALYZE
On Mon, Sep 05, 2022 at 11:56:30AM -0700, Nathan Bossart wrote: > Here is a first attempt at allowing users to grant VACUUM or ANALYZE > per-relation. Overall, this seems pretty straightforward. I needed to > adjust the permissions logic for VACUUM/ANALYZE a bit, which causes some > extra WARNING messages for VACUUM (ANALYZE) in some cases, but this didn't > seem particularly worrisome. It may be desirable to allow granting ANALYZE > on specific columns or to allow granting VACUUM/ANALYZE at the schema or > database level, but that is left as a future exercise. Here is a new patch set with some follow-up patches to implement $SUBJECT. 0001 is the same as v3. 0002 simplifies some WARNING messages as suggested upthread [0]. 0003 adds the new pg_vacuum_all_tables and pg_analyze_all_tables predefined roles. Instead of adjusting the permissions logic in vacuum.c, I modified pg_class_aclmask_ext() to return the ACL_VACUUM and/or ACL_ANALYZE bits as appropriate. [0] https://postgr.es/m/20220726.104712.912995710251150228.horikyota.ntt%40gmail.com -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From aa4796c3925fc7675d87df310f8057e62890138f Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Sat, 3 Sep 2022 23:31:38 -0700 Subject: [PATCH v4 1/3] 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:+| +| | |
Re: predefined role(s) for VACUUM and ANALYZE
On Tue, Sep 06, 2022 at 11:24:18AM -0400, Robert Haas wrote: > On Tue, Sep 6, 2022 at 11:11 AM Stephen Frost wrote: >> If we were to make the specific bits depend on the object type as I'm >> suggesting, then we'd have 8 bits used for relations (10 with the vacuum >> and analyze bits), leaving us with 6 remaining inside the existing >> uint32, or more bits available than we've ever used since the original >> implementation from what I can tell, or at least 15+ years. That seems >> like pretty darn good future-proofing without a lot of complication or >> any change in physical size. We would also be able to get rid of the >> question of "well, is it more valuable to add the ability to GRANT >> TRUNCATE on a relation, or GRANT CONNECT on databases" or other rather >> odd debates between ultimately very different things. > > I mostly agree with this. I don't think it's entirely clear how we > should try to get more bits going forward, but it's clear that we > cannot just forever hold our breath and refuse to find any more bits. > And of the possible ways of doing it, this seems like the one with the > lowest impact, so I think it likely makes sense to do this one first. +1. My earlier note wasn't intended to suggest that one approach was better than the other, merely that there are a couple of options to choose from once we run out of bits. I don't think this work needs to be tied to the VACUUM/ANALYZE stuff, but I am interested in it and hope to take it on at some point. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On Tue, Sep 6, 2022 at 11:11 AM Stephen Frost wrote: > Considering our burn rate of ACL bits is really rather slow (2 this > year, but prior to that was TRUNCATE in 2008 and CONNECT in 2006), I'd > argue that moving away from the current one-size-fits-all situation > would kick the can down the road more than just 'a little' and wouldn't > have any performance or space concerns. Once we actually get to the > point where we've burned through all of those after the next few decades > then we can move to a uint64 or something else more complicated, > perhaps. Our burn rate is slow because there's been a lot of pushback - mostly from Tom - about consuming the remaining bits. It's not because people haven't had ideas about how to use them up. > If we were to make the specific bits depend on the object type as I'm > suggesting, then we'd have 8 bits used for relations (10 with the vacuum > and analyze bits), leaving us with 6 remaining inside the existing > uint32, or more bits available than we've ever used since the original > implementation from what I can tell, or at least 15+ years. That seems > like pretty darn good future-proofing without a lot of complication or > any change in physical size. We would also be able to get rid of the > question of "well, is it more valuable to add the ability to GRANT > TRUNCATE on a relation, or GRANT CONNECT on databases" or other rather > odd debates between ultimately very different things. I mostly agree with this. I don't think it's entirely clear how we should try to get more bits going forward, but it's clear that we cannot just forever hold our breath and refuse to find any more bits. And of the possible ways of doing it, this seems like the one with the lowest impact, so I think it likely makes sense to do this one first. -- Robert Haas EDB: http://www.enterprisedb.com
Re: predefined role(s) for VACUUM and ANALYZE
Greetings, * Nathan Bossart (nathandboss...@gmail.com) wrote: > On Tue, Aug 23, 2022 at 07:46:47PM -0400, Stephen Frost wrote: > > I've long felt that we should redefine the way the ACLs work to have a > > distinct set of bits for each object type. We don't need to support a > > CONNECT bit on a table, yet we do today and we expend quite a few bits > > in that way. Having that handled on a per-object-type basis instead > > would allow us to get quite a bit more mileage out of the existing 32bit > > field before having to introduce more complicated storage methods like > > using a bit to tell us to go look up more ACLs somewhere else. > > There are 2 bits remaining at the moment, so I didn't redesign the ACL > system in the attached patch. However, I did some research on a couple > options. Using a distinct set of bits for each catalog table should free > up a handful of bits, which should indeed kick the can down the road a > little. Another easy option is to simply make AclMode a uint64, which > would immediately free up another 16 privilege bits. I was able to get > this approach building and passing tests in a few minutes, but there might > be performance/space concerns. Considering our burn rate of ACL bits is really rather slow (2 this year, but prior to that was TRUNCATE in 2008 and CONNECT in 2006), I'd argue that moving away from the current one-size-fits-all situation would kick the can down the road more than just 'a little' and wouldn't have any performance or space concerns. Once we actually get to the point where we've burned through all of those after the next few decades then we can move to a uint64 or something else more complicated, perhaps. If we were to make the specific bits depend on the object type as I'm suggesting, then we'd have 8 bits used for relations (10 with the vacuum and analyze bits), leaving us with 6 remaining inside the existing uint32, or more bits available than we've ever used since the original implementation from what I can tell, or at least 15+ years. That seems like pretty darn good future-proofing without a lot of complication or any change in physical size. We would also be able to get rid of the question of "well, is it more valuable to add the ability to GRANT TRUNCATE on a relation, or GRANT CONNECT on databases" or other rather odd debates between ultimately very different things. Thanks, Stephen signature.asc Description: PGP signature
Re: predefined role(s) for VACUUM and ANALYZE
On Mon, Sep 5, 2022 at 2:56 PM Nathan Bossart wrote: > There are 2 bits remaining at the moment, so I didn't redesign the ACL > system in the attached patch. However, I did some research on a couple > options. Using a distinct set of bits for each catalog table should free > up a handful of bits, which should indeed kick the can down the road a > little. Another easy option is to simply make AclMode a uint64, which > would immediately free up another 16 privilege bits. I was able to get > this approach building and passing tests in a few minutes, but there might > be performance/space concerns. I believe Tom has expressed such concerns in the past, but it is not clear to me that they are well-founded. I don't think we have much code that manipulates large numbers of aclitems, so I can't quite see where the larger size would be an issue. There may well be some places, so I'm not saying that Tom or anyone else with concerns is wrong, but I'm just having a hard time thinking of where it would be a real issue. -- Robert Haas EDB: http://www.enterprisedb.com
Re: predefined role(s) for VACUUM and ANALYZE
Here is a first attempt at allowing users to grant VACUUM or ANALYZE per-relation. Overall, this seems pretty straightforward. I needed to adjust the permissions logic for VACUUM/ANALYZE a bit, which causes some extra WARNING messages for VACUUM (ANALYZE) in some cases, but this didn't seem particularly worrisome. It may be desirable to allow granting ANALYZE on specific columns or to allow granting VACUUM/ANALYZE at the schema or database level, but that is left as a future exercise. On Tue, Aug 23, 2022 at 07:46:47PM -0400, Stephen Frost wrote: > I've long felt that we should redefine the way the ACLs work to have a > distinct set of bits for each object type. We don't need to support a > CONNECT bit on a table, yet we do today and we expend quite a few bits > in that way. Having that handled on a per-object-type basis instead > would allow us to get quite a bit more mileage out of the existing 32bit > field before having to introduce more complicated storage methods like > using a bit to tell us to go look up more ACLs somewhere else. There are 2 bits remaining at the moment, so I didn't redesign the ACL system in the attached patch. However, I did some research on a couple options. Using a distinct set of bits for each catalog table should free up a handful of bits, which should indeed kick the can down the road a little. Another easy option is to simply make AclMode a uint64, which would immediately free up another 16 privilege bits. I was able to get this approach building and passing tests in a few minutes, but there might be performance/space concerns. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com >From f19eea3f3148916a79d002094ca4eb4aa98af753 Mon Sep 17 00:00:00 2001 From: Nathan Bossart Date: Sat, 3 Sep 2022 23:31:38 -0700 Subject: [PATCH v3 1/1] 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 |
Re: predefined role(s) for VACUUM and ANALYZE
Greetings, * Robert Haas (robertmh...@gmail.com) wrote: > On Tue, Jul 26, 2022 at 1:50 PM David G. Johnston > wrote: > >> Still, it seems somewhat appealing to give > >> people fine-grained control over this, rather than just "on" or "off". > > Appealing enough to consume a couple of permission bits? > > https://www.postgresql.org/message-id/CAKFQuwZ6dhjTFV7Bwmehe1N3%3Dk484y4mM22zuYjVEU2dq9V1aQ%40mail.gmail.com > > I think we're down to 0 remaining now, so it'd be hard to justify > consuming 2 of 0 remaining bits. However, I maintain that the solution > to this is either (1) change the aclitem representation to get another > 32 bits or (2) invent a different system for less-commonly used > permission bits. Checking permissions for SELECT or UPDATE has to be > really fast, because most queries will need to do that sort of thing. > If we represented VACUUM or ANALYZE in some other way in the catalogs > that was more scalable but less efficient, it wouldn't be a big deal > (although there's the issue of code duplication to consider). I've long felt that we should redefine the way the ACLs work to have a distinct set of bits for each object type. We don't need to support a CONNECT bit on a table, yet we do today and we expend quite a few bits in that way. Having that handled on a per-object-type basis instead would allow us to get quite a bit more mileage out of the existing 32bit field before having to introduce more complicated storage methods like using a bit to tell us to go look up more ACLs somewhere else. Thanks, Stephen signature.asc Description: PGP signature
Re: predefined role(s) for VACUUM and ANALYZE
On Tue, Jul 26, 2022 at 01:54:38PM -0400, Robert Haas wrote: > I think we're down to 0 remaining now, so it'd be hard to justify > consuming 2 of 0 remaining bits. AFAICT there are 2 remaining. N_ACL_RIGHTS is only 14. > However, I maintain that the solution > to this is either (1) change the aclitem representation to get another > 32 bits or (2) invent a different system for less-commonly used > permission bits. Checking permissions for SELECT or UPDATE has to be > really fast, because most queries will need to do that sort of thing. > If we represented VACUUM or ANALYZE in some other way in the catalogs > that was more scalable but less efficient, it wouldn't be a big deal > (although there's the issue of code duplication to consider). Perhaps we could add something like a relacl_ext column to affected catalogs with many more than 32 privilege bits. However, if we actually do have 2 bits remaining, we wouldn't need to do that work now unless someone else uses them first. That being said, it's certainly worth thinking about the future of this stuff. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
At Tue, 26 Jul 2022 13:54:38 -0400, Robert Haas wrote in > On Tue, Jul 26, 2022 at 1:50 PM David G. Johnston > wrote: > >> Still, it seems somewhat appealing to give > >> people fine-grained control over this, rather than just "on" or "off". > > Appealing enough to consume a couple of permission bits? > > https://www.postgresql.org/message-id/CAKFQuwZ6dhjTFV7Bwmehe1N3%3Dk484y4mM22zuYjVEU2dq9V1aQ%40mail.gmail.com > > I think we're down to 0 remaining now, so it'd be hard to justify > consuming 2 of 0 remaining bits. However, I maintain that the solution > to this is either (1) change the aclitem representation to get another > 32 bits or (2) invent a different system for less-commonly used > permission bits. Checking permissions for SELECT or UPDATE has to be > really fast, because most queries will need to do that sort of thing. > If we represented VACUUM or ANALYZE in some other way in the catalogs > that was more scalable but less efficient, it wouldn't be a big deal > (although there's the issue of code duplication to consider). I guess that we can use the last bit for ACL_SLOW_PATH or something like. Furthermore we could move some existing ACL modeds to that slow path to vacate some fast-ACL bits. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: predefined role(s) for VACUUM and ANALYZE
On Tue, Jul 26, 2022 at 1:50 PM David G. Johnston wrote: >> Still, it seems somewhat appealing to give >> people fine-grained control over this, rather than just "on" or "off". > Appealing enough to consume a couple of permission bits? > https://www.postgresql.org/message-id/CAKFQuwZ6dhjTFV7Bwmehe1N3%3Dk484y4mM22zuYjVEU2dq9V1aQ%40mail.gmail.com I think we're down to 0 remaining now, so it'd be hard to justify consuming 2 of 0 remaining bits. However, I maintain that the solution to this is either (1) change the aclitem representation to get another 32 bits or (2) invent a different system for less-commonly used permission bits. Checking permissions for SELECT or UPDATE has to be really fast, because most queries will need to do that sort of thing. If we represented VACUUM or ANALYZE in some other way in the catalogs that was more scalable but less efficient, it wouldn't be a big deal (although there's the issue of code duplication to consider). -- Robert Haas EDB: http://www.enterprisedb.com
Re: predefined role(s) for VACUUM and ANALYZE
On Tue, Jul 26, 2022 at 10:37 AM Robert Haas wrote: > On Mon, Jul 25, 2022 at 9:47 PM Kyotaro Horiguchi > wrote: > > One arguable point would be whether we will need to put restriction > > the target relations that Bob can vacuum/analyze. > > But for a command with a target, you really ought to have a > permission on the object, not just a general permission. On the other > hand, we do have things like pg_read_all_tables, so we could have > pg_vacuum_all_tables too. I'm still more likely to create a specific security definer function owned by the relevant table owner to give out ANALYZE (and maybe VACUUM) permission to ETL-performing roles. Still, it seems somewhat appealing to give > people fine-grained control over this, rather than just "on" or "off". > > Appealing enough to consume a couple of permission bits? https://www.postgresql.org/message-id/CAKFQuwZ6dhjTFV7Bwmehe1N3%3Dk484y4mM22zuYjVEU2dq9V1aQ%40mail.gmail.com David J.
Re: predefined role(s) for VACUUM and ANALYZE
On Mon, Jul 25, 2022 at 9:47 PM Kyotaro Horiguchi wrote: > One arguable point would be whether we will need to put restriction > the target relations that Bob can vacuum/analyze. Yeah. pg_checkpoint makes sense because you can either CHECKPOINT or you can't. But for a command with a target, you really ought to have a permission on the object, not just a general permission. On the other hand, we do have things like pg_read_all_tables, so we could have pg_vacuum_all_tables too. Still, it seems somewhat appealing to give people fine-grained control over this, rather than just "on" or "off". -- Robert Haas EDB: http://www.enterprisedb.com
Re: predefined role(s) for VACUUM and ANALYZE
At Tue, 26 Jul 2022 10:47:12 +0900 (JST), Kyotaro Horiguchi wrote in > WARNING: skipping "pg_statistic" --- only superusers, roles with privileges > of pg_vacuum_analyze, or the database owner can vacuum it > WARNING: skipping "pg_type" --- only superusers, roles with privileges of > pg_vacuum_analyze, or the database owner can vacuum it > > WARNING: skipping "user_mappings" --- only table or database owner can > vacuum it By the way, the last error above dissapears by granting pg_vacuum_analyze to the role. Is there a reason the message is left alone? And If I specified the view directly, I would get the following message. postgres=> vacuum information_schema.user_mappings; WARNING: skipping "user_mappings" --- cannot vacuum non-tables or special system tables So, "VACUUM;" does something wrong? Or is it the designed behavior? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: predefined role(s) for VACUUM and ANALYZE
At Mon, 25 Jul 2022 09:40:49 -0700, Nathan Bossart
wrote in
> On Mon, Jul 25, 2022 at 12:58:36PM +0530, Bharath Rupireddy wrote:
> > Thanks. I'm personally happy with more granular levels of control (as
> > we don't have to give full superuser access to just run a few commands
> > or maintenance operations) for various postgres commands. The only
> > concern is that we might eventually end up with many predefined roles
> > (perhaps one predefined role per command), spreading all around the
> > code base and it might be difficult for the users to digest all of the
> > roles in. It will be great if we can have some sort of rules or
> > methods to define a separate role for a command.
>
> Yeah, in the future, I could see this growing to a couple dozen predefined
> roles. Given they are relatively inexpensive and there are already 12 of
> them, I'm personally not too worried about the list becoming too unwieldy.
> Another way to help users might be to create additional aggregate
> predefined roles (like pg_monitor) for common combinations.
I agree to the necessity of that execution control, but I fear a
little how many similar roles will come in future (but it doesn't seem
so much?). I didn't think so when pg_checkpoint was introdueced,
though. That being said, since we're going to control
maintenance'ish-command execution via predefined roles so it is fine
in that criteria.
One arguable point would be whether we will need to put restriction
the target relations that Bob can vacuum/analyze. If we need that, the
new predeefined role is not sufficient then need a new syntax for that.
GRANT EXECUTION COMMAND VACUUM ON TABLE rel1 TO bob.
GRANT EXECUTION COMMAND VACUUM ON TABLES OWNED BY alice TO bob.
GRANT EXECUTION COMMAND VACUUM ON ALL TABLES OWNED BY alice TO bob.
However, one problem of these syntaxes is they cannot do something to
future relations.
So, considering that aspect, I would finally agree to the proposal
here. (In short +1 for what the patch does.)
About the patch, it seems fine as the whole except the change in error
messages.
- (errmsg("skipping \"%s\" --- only superuser can analyze it",
+ (errmsg("skipping \"%s\" --- only superusers and roles with "
+ "privileges of pg_vacuum_analyze can analyze it",
The message looks a bit too verbose or lengty especially when many
relations are rejected.
WARNING: skipping "pg_statistic" --- only superusers, roles with privileges of
pg_vacuum_analyze, or the database owner can vacuum it
WARNING: skipping "pg_type" --- only superusers, roles with privileges of
pg_vacuum_analyze, or the database owner can vacuum it
WARNING: skipping "user_mappings" --- only table or database owner can vacuum
it
VACUUM
Couldn't we simplify the message? For example "skipping \"%s\" ---
insufficient priviledges". We could add a detailed (not a DETAIL:)
message at the end to cover all of the skipped relations, but it may
be too much.
By the way the patch splits an error message into several parts but
that later makes it harder to search for the message in the tree. *I*
would suggest not splitting message strings.
# I refrain from suggesing removing parens surrounding errmsg() :p
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: predefined role(s) for VACUUM and ANALYZE
On Mon, Jul 25, 2022 at 12:58:36PM +0530, Bharath Rupireddy wrote: > Thanks. I'm personally happy with more granular levels of control (as > we don't have to give full superuser access to just run a few commands > or maintenance operations) for various postgres commands. The only > concern is that we might eventually end up with many predefined roles > (perhaps one predefined role per command), spreading all around the > code base and it might be difficult for the users to digest all of the > roles in. It will be great if we can have some sort of rules or > methods to define a separate role for a command. Yeah, in the future, I could see this growing to a couple dozen predefined roles. Given they are relatively inexpensive and there are already 12 of them, I'm personally not too worried about the list becoming too unwieldy. Another way to help users might be to create additional aggregate predefined roles (like pg_monitor) for common combinations. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Re: predefined role(s) for VACUUM and ANALYZE
On Sat, Jul 23, 2022 at 2:07 AM Nathan Bossart wrote: > > Hi hackers, > > The previous attempt to add a predefined role for VACUUM and ANALYZE [0] > resulted in the new pg_checkpoint role in v15. I'd like to try again to > add a new role (or multiple new roles) for VACUUM and ANALYZE. > > The primary motivation for this is to continue chipping away at things that > require special privileges or even superuser. VACUUM and ANALYZE typically > require table ownership, database ownership, or superuser. And only > superusers can VACUUM/ANALYZE shared catalogs. A predefined role for these > operations would allow delegating such tasks (e.g., a nightly VACUUM > scheduled with pg_cron) to a role with fewer privileges. Thanks. I'm personally happy with more granular levels of control (as we don't have to give full superuser access to just run a few commands or maintenance operations) for various postgres commands. The only concern is that we might eventually end up with many predefined roles (perhaps one predefined role per command), spreading all around the code base and it might be difficult for the users to digest all of the roles in. It will be great if we can have some sort of rules or methods to define a separate role for a command. > The attached patch adds a pg_vacuum_analyze role that allows VACUUM and > ANALYZE commands on all relations. I started by trying to introduce > separate pg_vacuum and pg_analyze roles, but that quickly became > complicated because the VACUUM and ANALYZE code is intertwined. To > initiate the discussion, here's the simplest thing I could think of. pg_vacuum_analyze, immediately, makes me think if we need to have a predefined role for CLUSTER command and maybe for other commands as well such as EXECUTE, CALL, ALTER SYSTEM SET, LOAD, COPY and so on. > An alternate approach might be to allow using GRANT to manage these > privileges, as suggested in the previous thread [1]. > > Thoughts? > > [0] > https://postgr.es/m/67a1d667e8ec228b5e07f232184c80348c5d93f4.camel%40j-davis.com > [1] https://postgr.es/m/20211104224636.5qg6cfyjkw52r...@alap3.anarazel.de I think GRANT approach [1] is worth considering or at least discussing its pros and cons might give us a better idea as to why we need separate predefined roles. Regards, Bharath Rupireddy.
Re: predefined role(s) for VACUUM and ANALYZE
On Fri, Jul 22, 2022 at 01:37:35PM -0700, Nathan Bossart wrote:
> The attached patch adds a pg_vacuum_analyze role that allows VACUUM and
> ANALYZE commands on all relations. I started by trying to introduce
> separate pg_vacuum and pg_analyze roles, but that quickly became
> complicated because the VACUUM and ANALYZE code is intertwined. To
> initiate the discussion, here's the simplest thing I could think of.
And here's the same patch, but with docs that actually build.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index b968f740cb..203b713a4e 100644
--- a/doc/src/sgml/ref/analyze.sgml
+++ b/doc/src/sgml/ref/analyze.sgml
@@ -148,11 +148,14 @@ ANALYZE [ VERBOSE ] [ table_and_columnsNotes
- To analyze a table, one must ordinarily be the table's owner or a
- superuser. However, database owners are allowed to
+ To analyze a table, one must ordinarily be the table's owner, a superuser, or
+ a role with privileges of the
+ pg_vacuum_analyze
+ 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_vacuum_analyze.)
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 c582021d29..12d7b96fee 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -356,11 +356,14 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ pg_vacuum_analyze
+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_analyze.)
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 6e36b8..6052bd0c4f 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -588,6 +588,13 @@ DROP ROLE doomed_role;
the CHECKPOINT
command.
+
+ pg_vacuum_analyze
+ Allow executing the
+ VACUUM and
+ ANALYZE
+ commands on all tables.
+
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 8df25f59d8..b3eb41a8cc 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -36,6 +36,7 @@
#include "access/xact.h"
#include "catalog/namespace.h"
#include "catalog/index.h"
+#include "catalog/pg_authid.h"
#include "catalog/pg_database.h"
#include "catalog/pg_inherits.h"
#include "catalog/pg_namespace.h"
@@ -574,7 +575,8 @@ vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, bits32 options)
* trying to vacuum or analyze the rest of the DB --- is this appropriate?
*/
if (pg_class_ownercheck(relid, GetUserId()) ||
- (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !reltuple->relisshared))
+ (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !reltuple->relisshared) ||
+ has_privs_of_role(GetUserId(), ROLE_PG_VACUUM_ANALYZE))
return true;
relname = NameStr(reltuple->relname);
@@ -583,11 +585,14 @@ vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, bits32 options)
{
if (reltuple->relisshared)
ereport(WARNING,
- (errmsg("skipping \"%s\" --- only superuser can vacuum it",
+ (errmsg("skipping \"%s\" --- only superusers and roles with "
+ "privileges of pg_vacuum_analyze can vacuum it",
relname)));
else if (reltuple->relnamespace == PG_CATALOG_NAMESPACE)
ereport(WARNING,
- (errmsg("skipping \"%s\" --- only superuser or database owner can vacuum it",
+ (errmsg("skipping \"%s\" --- only superusers, roles with "
+ "privileges of pg_vacuum_analyze, or the database "
+ "owner can vacuum it",
relname)));
else
ereport(WARNING,
@@ -606,11 +611,14 @@ vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, bits32 options)
{
if (reltuple->relisshared)
ereport(WARNING,
- (errmsg("skipping \"%s\" --- only superuser can analyze it",
+ (errmsg("skipping \"%s\" --- only superusers and roles with "
+ "privileges of pg_vacuum_analyze can analyze it",
relname)));
else if (reltuple->relnamespace == PG_CATALOG_NAMESPACE)
ereport(WARNING,
- (errmsg("skipping \"%s\" --- only superuser or database owner can analyze it",
+ (errmsg("skipping \"%s\" --- only superusers, roles with "
+ "privileges of pg_vacuum_analyze, or the database "
+
predefined role(s) for VACUUM and ANALYZE
Hi hackers,
The previous attempt to add a predefined role for VACUUM and ANALYZE [0]
resulted in the new pg_checkpoint role in v15. I'd like to try again to
add a new role (or multiple new roles) for VACUUM and ANALYZE.
The primary motivation for this is to continue chipping away at things that
require special privileges or even superuser. VACUUM and ANALYZE typically
require table ownership, database ownership, or superuser. And only
superusers can VACUUM/ANALYZE shared catalogs. A predefined role for these
operations would allow delegating such tasks (e.g., a nightly VACUUM
scheduled with pg_cron) to a role with fewer privileges.
The attached patch adds a pg_vacuum_analyze role that allows VACUUM and
ANALYZE commands on all relations. I started by trying to introduce
separate pg_vacuum and pg_analyze roles, but that quickly became
complicated because the VACUUM and ANALYZE code is intertwined. To
initiate the discussion, here's the simplest thing I could think of.
An alternate approach might be to allow using GRANT to manage these
privileges, as suggested in the previous thread [1].
Thoughts?
[0]
https://postgr.es/m/67a1d667e8ec228b5e07f232184c80348c5d93f4.camel%40j-davis.com
[1] https://postgr.es/m/20211104224636.5qg6cfyjkw52r...@alap3.anarazel.de
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
commit 0b1397397c0f490525d3a15f5e9d5eb8f6023aa9
Author: Nathan Bossart
Date: Fri Jul 22 12:21:16 2022 -0700
introduce pg_vacuum_analyze
diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index b968f740cb..203b713a4e 100644
--- a/doc/src/sgml/ref/analyze.sgml
+++ b/doc/src/sgml/ref/analyze.sgml
@@ -148,11 +148,14 @@ ANALYZE [ VERBOSE ] [ table_and_columnsNotes
- To analyze a table, one must ordinarily be the table's owner or a
- superuser. However, database owners are allowed to
+ To analyze a table, one must ordinarily be the table's owner, a superuser, or
+ a role with privileges of the
+ pg_vacuum_analyze
+ 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_vacuum_analyze.)
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 c582021d29..12d7b96fee 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -356,11 +356,14 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ pg_vacuum_analyze
+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_analyze.)
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 6e36b8..b09aa7aed9 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -588,6 +588,13 @@ DROP ROLE doomed_role;
the CHECKPOINT
command.
+
+ pg_vacuum_analyze
+ Allow executing the
+ VACUUM and
+ ANALYZE
+ commands on all tables.
+
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 8df25f59d8..b3eb41a8cc 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -36,6 +36,7 @@
#include "access/xact.h"
#include "catalog/namespace.h"
#include "catalog/index.h"
+#include "catalog/pg_authid.h"
#include "catalog/pg_database.h"
#include "catalog/pg_inherits.h"
#include "catalog/pg_namespace.h"
@@ -574,7 +575,8 @@ vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, bits32 options)
* trying to vacuum or analyze the rest of the DB --- is this appropriate?
*/
if (pg_class_ownercheck(relid, GetUserId()) ||
- (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !reltuple->relisshared))
+ (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !reltuple->relisshared) ||
+ has_privs_of_role(GetUserId(), ROLE_PG_VACUUM_ANALYZE))
return true;
relname = NameStr(reltuple->relname);
@@ -583,11 +585,14 @@ vacuum_is_relation_owner(Oid relid, Form_pg_class reltuple, bits32 options)
{
if (reltuple->relisshared)
ereport(WARNING,
- (errmsg("skipping \"%s\" --- only superuser can vacuum it",
+ (errmsg("skipping \"%s\" --- only superusers and roles with "
+ "privileges of pg_vacuum_analyze can vacuum it",
relname)));
else if (reltuple->relnamespace == PG_CATALOG_NAMESPACE)
ereport(WARNING,
-
