public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v6 1/2] fix MAINTAIN privs for partitions
6+ messages / 4 participants
[nested] [flat]
* [PATCH v6 1/2] fix MAINTAIN privs for partitions
@ 2023-01-09 22:29 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Nathan Bossart @ 2023-01-09 22:29 UTC (permalink / raw)
---
doc/src/sgml/ref/analyze.sgml | 5 ++-
doc/src/sgml/ref/cluster.sgml | 17 ++++++---
doc/src/sgml/ref/lock.sgml | 5 ++-
doc/src/sgml/ref/reindex.sgml | 6 +++-
doc/src/sgml/ref/vacuum.sgml | 5 ++-
src/backend/commands/cluster.c | 35 +++++++++++++------
src/backend/commands/indexcmds.c | 22 ++++++------
src/backend/commands/lockcmds.c | 8 +++++
src/backend/commands/tablecmds.c | 30 ++++++++++++++--
src/backend/commands/vacuum.c | 9 +++--
src/include/commands/tablecmds.h | 1 +
.../expected/cluster-conflict-partition.out | 6 ++--
.../specs/cluster-conflict-partition.spec | 5 +--
src/test/regress/expected/cluster.out | 4 ++-
src/test/regress/expected/vacuum.out | 18 ----------
src/test/regress/sql/cluster.sql | 2 ++
16 files changed, 119 insertions(+), 59 deletions(-)
diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index a26834da4f..2f94e89cb0 100644
--- a/doc/src/sgml/ref/analyze.sgml
+++ b/doc/src/sgml/ref/analyze.sgml
@@ -156,7 +156,10 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea
analyze all tables in their databases, except shared catalogs.
(The restriction for shared catalogs means that a true database-wide
<command>ANALYZE</command> can only be performed by superusers and roles
- with privileges of <literal>pg_maintain</literal>.)
+ with privileges of <literal>pg_maintain</literal>.) If a role has
+ permission to <command>ANALYZE</command> a partitioned table, it is also
+ permitted to <command>ANALYZE</command> each of its partitions, regardless
+ of whether the role has the aforementioned privileges on the partition.
<command>ANALYZE</command> will skip over any tables that the calling user
does not have permission to analyze.
</para>
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 145101e6a5..b9f2acb1de 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -69,10 +69,7 @@ CLUSTER [VERBOSE]
<para>
<command>CLUSTER</command> without any parameter reclusters all the
previously-clustered tables in the current database that the calling user
- owns or has the <literal>MAINTAIN</literal> privilege for, or all such tables
- if called by a superuser or a role with privileges of the
- <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link>
- role. This form of <command>CLUSTER</command> cannot be
+ has privileges for. This form of <command>CLUSTER</command> cannot be
executed inside a transaction block.
</para>
@@ -134,6 +131,18 @@ CLUSTER [VERBOSE]
<refsect1>
<title>Notes</title>
+ <para>
+ To cluster a table, one must have the <literal>MAINTAIN</literal> privilege
+ on the table or be the table's owner, a superuser, or a role with
+ privileges of the
+ <link linkend="predefined-roles-table"><literal>pg_maintain</literal></link>
+ role. If a role has permission to <command>CLUSTER</command> a partitioned
+ table, it is also permitted to <command>CLUSTER</command> each of its
+ partitions, regardless of whether the role has the aforementioned
+ privileges on the partition. <command>CLUSTER</command> will skip over any
+ tables that the calling user does not have permission to cluster.
+ </para>
+
<para>
In cases where you are accessing single rows randomly
within a table, the actual order of the data in the
diff --git a/doc/src/sgml/ref/lock.sgml b/doc/src/sgml/ref/lock.sgml
index 8524182211..5b3b2b793a 100644
--- a/doc/src/sgml/ref/lock.sgml
+++ b/doc/src/sgml/ref/lock.sgml
@@ -177,7 +177,10 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ]
MODE</literal> (or a less-conflicting mode as described in <xref
linkend="explicit-locking"/>) is permitted. If a user has
<literal>SELECT</literal> privileges on the table, <literal>ACCESS SHARE
- MODE</literal> is permitted.
+ MODE</literal> is permitted. If a role has permission to lock a
+ partitioned table, it is also permitted to lock each of its partitions,
+ regardless of whether the role has the aforementioned privileges on the
+ partition.
</para>
<para>
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 192513f34e..c6ad2546f9 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -306,7 +306,11 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DA
indexes on shared catalogs will be skipped unless the user owns the
catalog (which typically won't be the case), has privileges of the
<literal>pg_maintain</literal> role, or has the <literal>MAINTAIN</literal>
- privilege on the catalog. Of course, superusers can always reindex anything.
+ privilege on the catalog. If a role has permission to
+ <command>REINDEX</command> a partitioned table, it is also permitted to
+ <command>REINDEX</command> each of its partitions, regardless of whether the
+ role has the aforementioned privileges on the partition. Of course,
+ superusers can always reindex anything.
</para>
<para>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 8fa8421847..545b23b54f 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -401,7 +401,10 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
vacuum all tables in their databases, except shared catalogs.
(The restriction for shared catalogs means that a true database-wide
<command>VACUUM</command> can only be performed by superusers and roles
- with privileges of <literal>pg_maintain</literal>.)
+ with privileges of <literal>pg_maintain</literal>.) If a role has
+ permission to <command>VACUUM</command> a partitioned table, it is also
+ permitted to <command>VACUUM</command> each of its partitions, regardless
+ of whether the role has the aforementioned privileges on the partition.
<command>VACUUM</command> will skip over any tables that the calling user
does not have permission to vacuum.
</para>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index f11691aff7..369fea7c04 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -80,6 +80,7 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
static List *get_tables_to_cluster(MemoryContext cluster_context);
static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
Oid indexOid);
+static bool cluster_is_permitted_for_relation(Oid relid, Oid userid);
/*---------------------------------------------------------------------------
@@ -366,8 +367,7 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
if (recheck)
{
/* Check that the user still has privileges for the relation */
- if (!object_ownercheck(RelationRelationId, tableOid, save_userid) &&
- pg_class_aclcheck(tableOid, save_userid, ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(tableOid, save_userid))
{
relation_close(OldHeap, AccessExclusiveLock);
goto out;
@@ -1645,8 +1645,7 @@ get_tables_to_cluster(MemoryContext cluster_context)
index = (Form_pg_index) GETSTRUCT(indexTuple);
- if (!object_ownercheck(RelationRelationId, index->indrelid, GetUserId()) &&
- pg_class_aclcheck(index->indrelid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (!cluster_is_permitted_for_relation(index->indrelid, GetUserId()))
continue;
/* Use a permanent memory context for the result list */
@@ -1694,12 +1693,11 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
if (get_rel_relkind(indexrelid) != RELKIND_INDEX)
continue;
- /* Silently skip partitions which the user has no access to. */
- if (!object_ownercheck(RelationRelationId, relid, GetUserId()) &&
- pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
- (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) ||
- IsSharedRelation(relid)))
- continue;
+ /*
+ * We already checked that the user has privileges to CLUSTER the
+ * partitioned table when we locked it earlier, so there's no need to
+ * check the privileges again here.
+ */
/* Use a permanent memory context for the result list */
old_context = MemoryContextSwitchTo(cluster_context);
@@ -1714,3 +1712,20 @@ get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
return rtcs;
}
+
+/*
+ * Return whether userid has privileges to CLUSTER relid. If not, this
+ * function emits a WARNING.
+ */
+static bool
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+ if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK ||
+ has_partition_ancestor_privs(relid, userid, ACL_MAINTAIN))
+ return true;
+
+ ereport(WARNING,
+ (errmsg("permission denied to cluster \"%s\", skipping it",
+ get_rel_name(relid))));
+ return false;
+}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8afc006f89..16ec0b114e 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2796,9 +2796,9 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
/* Check permissions */
table_oid = IndexGetRelation(relId, true);
- if (!object_ownercheck(RelationRelationId, relId, GetUserId()) &&
- OidIsValid(table_oid) &&
- pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (OidIsValid(table_oid) &&
+ pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
+ !has_partition_ancestor_privs(table_oid, GetUserId(), ACL_MAINTAIN))
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_INDEX,
relation->relname);
@@ -3008,16 +3008,16 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
/*
* The table can be reindexed if the user has been granted MAINTAIN on
- * the table or the user is a superuser, the table owner, or the
- * database/schema owner (but in the latter case, only if it's not a
- * shared relation). object_ownercheck includes the superuser case,
- * and depending on objectKind we already know that the user has
- * permission to run REINDEX on this database or schema per the
- * permission checks at the beginning of this routine.
+ * the table or one of its partition ancestors or the user is a
+ * superuser, the table owner, or the database/schema owner (but in the
+ * latter case, only if it's not a shared relation). pg_class_aclcheck
+ * includes the superuser case, and depending on objectKind we already
+ * know that the user has permission to run REINDEX on this database or
+ * schema per the permission checks at the beginning of this routine.
*/
if (classtuple->relisshared &&
- !object_ownercheck(RelationRelationId, relid, GetUserId()) &&
- pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
+ !has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN))
continue;
/*
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 410d78b040..6bf1b815f0 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -19,6 +19,7 @@
#include "catalog/namespace.h"
#include "catalog/pg_inherits.h"
#include "commands/lockcmds.h"
+#include "commands/tablecmds.h"
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
#include "parser/parse_clause.h"
@@ -305,5 +306,12 @@ LockTableAclCheck(Oid reloid, LOCKMODE lockmode, Oid userid)
aclresult = pg_class_aclcheck(reloid, userid, aclmask);
+ /*
+ * If this is a partition, check permissions of its ancestors if needed.
+ */
+ if (aclresult != ACLCHECK_OK &&
+ has_partition_ancestor_privs(reloid, userid, ACL_MAINTAIN))
+ aclresult = ACLCHECK_OK;
+
return aclresult;
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1fbdad4b64..7c697a285b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16886,12 +16886,38 @@ RangeVarCallbackMaintainsTable(const RangeVar *relation,
errmsg("\"%s\" is not a table or materialized view", relation->relname)));
/* Check permissions */
- if (!object_ownercheck(RelationRelationId, relId, GetUserId()) &&
- pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK)
+ if (pg_class_aclcheck(relId, GetUserId(), ACL_MAINTAIN) != ACLCHECK_OK &&
+ !has_partition_ancestor_privs(relId, GetUserId(), ACL_MAINTAIN))
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_TABLE,
relation->relname);
}
+/*
+ * If relid is a partition, returns whether userid has any of the privileges
+ * specified in acl on any of its ancestors. Otherwise, returns false.
+ */
+bool
+has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl)
+{
+ List *ancestors;
+ ListCell *lc;
+
+ if (!get_rel_relispartition(relid))
+ return false;
+
+ ancestors = get_partition_ancestors(relid);
+ foreach(lc, ancestors)
+ {
+ Oid ancestor = lfirst_oid(lc);
+
+ if (OidIsValid(ancestor) &&
+ pg_class_aclcheck(ancestor, userid, acl) == ACLCHECK_OK)
+ return true;
+ }
+
+ return false;
+}
+
/*
* Callback to RangeVarGetRelidExtended() for TRUNCATE processing.
*/
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index ea1428dc8c..076b1e54eb 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -41,6 +41,7 @@
#include "catalog/pg_namespace.h"
#include "commands/cluster.h"
#include "commands/defrem.h"
+#include "commands/tablecmds.h"
#include "commands/vacuum.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
@@ -598,11 +599,13 @@ vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple,
* - the role owns the relation
* - the role owns the current database and the relation is not shared
* - the role has been granted the MAINTAIN privilege on the relation
+ * - the role has privileges to vacuum/analyze any of the relation's
+ * partition ancestors
*----------
*/
- if (object_ownercheck(RelationRelationId, relid, GetUserId()) ||
- (object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) ||
- pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK)
+ if ((object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()) && !reltuple->relisshared) ||
+ pg_class_aclcheck(relid, GetUserId(), ACL_MAINTAIN) == ACLCHECK_OK ||
+ has_partition_ancestor_privs(relid, GetUserId(), ACL_MAINTAIN))
return true;
relname = NameStr(reltuple->relname);
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 2e717fa815..e7c2b91a58 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -98,6 +98,7 @@ extern void AtEOSubXact_on_commit_actions(bool isCommit,
extern void RangeVarCallbackMaintainsTable(const RangeVar *relation,
Oid relId, Oid oldRelId,
void *arg);
+extern bool has_partition_ancestor_privs(Oid relid, Oid userid, AclMode acl);
extern void RangeVarCallbackOwnsRelation(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out
index 7acb675c97..8d21276996 100644
--- a/src/test/isolation/expected/cluster-conflict-partition.out
+++ b/src/test/isolation/expected/cluster-conflict-partition.out
@@ -22,14 +22,16 @@ starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_res
step s1_begin: BEGIN;
step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE;
step s2_auth: SET ROLE regress_cluster_part;
-step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind;
+step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...>
step s1_commit: COMMIT;
+step s2_cluster: <... completed>
step s2_reset: RESET ROLE;
starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset
step s1_begin: BEGIN;
step s2_auth: SET ROLE regress_cluster_part;
step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE;
-step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind;
+step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...>
step s1_commit: COMMIT;
+step s2_cluster: <... completed>
step s2_reset: RESET ROLE;
diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec
index 5091f684a9..ae38cb4ee3 100644
--- a/src/test/isolation/specs/cluster-conflict-partition.spec
+++ b/src/test/isolation/specs/cluster-conflict-partition.spec
@@ -27,11 +27,8 @@ step s2_auth { SET ROLE regress_cluster_part; }
step s2_cluster { CLUSTER cluster_part_tab USING cluster_part_ind; }
step s2_reset { RESET ROLE; }
-# CLUSTER on the parent waits if locked, passes for all cases.
+# CLUSTER waits if locked, passes for all cases.
permutation s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset
permutation s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset
-
-# When taking a lock on a partition leaf, CLUSTER on the parent skips
-# the leaf, passes for all cases.
permutation s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset
permutation s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 542c2e098c..2eec483eaa 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -352,7 +352,9 @@ INSERT INTO clstr_3 VALUES (1);
-- this user can only cluster clstr_1 and clstr_3, but the latter
-- has not been clustered
SET SESSION AUTHORIZATION regress_clstr_user;
+SET client_min_messages = ERROR; -- order of "skipping" warnings may vary
CLUSTER;
+RESET client_min_messages;
SELECT * FROM clstr_1 UNION ALL
SELECT * FROM clstr_2 UNION ALL
SELECT * FROM clstr_3;
@@ -513,7 +515,7 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a
-----------+----------
ptnowner | t
ptnowner1 | f
- ptnowner2 | t
+ ptnowner2 | f
(3 rows)
DROP TABLE ptnowner;
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index d860be0e20..458adee7f8 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -353,20 +353,14 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum;
ALTER TABLE vacowned_part1 OWNER TO regress_vacuum;
SET ROLE regress_vacuum;
VACUUM vacowned_parted;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
VACUUM vacowned_part1;
VACUUM vacowned_part2;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
ANALYZE vacowned_parted;
-WARNING: permission denied to analyze "vacowned_part2", skipping it
ANALYZE vacowned_part1;
ANALYZE vacowned_part2;
-WARNING: permission denied to analyze "vacowned_part2", skipping it
VACUUM (ANALYZE) vacowned_parted;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
VACUUM (ANALYZE) vacowned_part1;
VACUUM (ANALYZE) vacowned_part2;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
RESET ROLE;
-- Only one partition owned by other user.
ALTER TABLE vacowned_parted OWNER TO CURRENT_USER;
@@ -395,26 +389,14 @@ ALTER TABLE vacowned_parted OWNER TO regress_vacuum;
ALTER TABLE vacowned_part1 OWNER TO CURRENT_USER;
SET ROLE regress_vacuum;
VACUUM vacowned_parted;
-WARNING: permission denied to vacuum "vacowned_part1", skipping it
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
VACUUM vacowned_part1;
-WARNING: permission denied to vacuum "vacowned_part1", skipping it
VACUUM vacowned_part2;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
ANALYZE vacowned_parted;
-WARNING: permission denied to analyze "vacowned_part1", skipping it
-WARNING: permission denied to analyze "vacowned_part2", skipping it
ANALYZE vacowned_part1;
-WARNING: permission denied to analyze "vacowned_part1", skipping it
ANALYZE vacowned_part2;
-WARNING: permission denied to analyze "vacowned_part2", skipping it
VACUUM (ANALYZE) vacowned_parted;
-WARNING: permission denied to vacuum "vacowned_part1", skipping it
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
VACUUM (ANALYZE) vacowned_part1;
-WARNING: permission denied to vacuum "vacowned_part1", skipping it
VACUUM (ANALYZE) vacowned_part2;
-WARNING: permission denied to vacuum "vacowned_part2", skipping it
RESET ROLE;
DROP TABLE vacowned;
DROP TABLE vacowned_parted;
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 6cb9c926c0..a4cfaae807 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -145,7 +145,9 @@ INSERT INTO clstr_3 VALUES (1);
-- this user can only cluster clstr_1 and clstr_3, but the latter
-- has not been clustered
SET SESSION AUTHORIZATION regress_clstr_user;
+SET client_min_messages = ERROR; -- order of "skipping" warnings may vary
CLUSTER;
+RESET client_min_messages;
SELECT * FROM clstr_1 UNION ALL
SELECT * FROM clstr_2 UNION ALL
SELECT * FROM clstr_3;
--
2.25.1
--sdtB3X0nJg68CQEu
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0002-fix-MAINTAIN-privs-for-TOAST-tables.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: PSQL error: total cell count of XXX exceeded
@ 2023-09-12 04:19 Tom Lane <[email protected]>
2023-09-13 02:22 ` Re: PSQL error: total cell count of XXX exceeded Hongxu Ma <[email protected]>
2023-11-20 20:48 ` Re: PSQL error: total cell count of XXX exceeded Alvaro Herrera <[email protected]>
0 siblings, 2 replies; 6+ messages in thread
From: Tom Lane @ 2023-09-12 04:19 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Hongxu Ma <[email protected]>; Jelte Fennema <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
Michael Paquier <[email protected]> writes:
> On Tue, Sep 12, 2023 at 02:39:55AM +0000, Hongxu Ma wrote:
> + long total_cells;
> long is 4 bytes on Windows, and 8 bytes basically elsewhere. So you
> would still have the same problem on Windows, no?
More to the point: what about the multiplication in printTableInit?
The cat's been out of the bag for quite some time before we get to
printTableAddCell.
I'm more than a bit skeptical about trying to do something about this,
simply because this range of query result sizes is far past what is
practical. The OP clearly hasn't tested his patch on actually
overflowing query results, and I don't care to either.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: PSQL error: total cell count of XXX exceeded
2023-09-12 04:19 Re: PSQL error: total cell count of XXX exceeded Tom Lane <[email protected]>
@ 2023-09-13 02:22 ` Hongxu Ma <[email protected]>
2023-09-13 07:31 ` Re: PSQL error: total cell count of XXX exceeded Hongxu Ma <[email protected]>
1 sibling, 1 reply; 6+ messages in thread
From: Hongxu Ma @ 2023-09-13 02:22 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Michael Paquier <[email protected]>; +Cc: pgsql-hackers
Thanks for pointing that, I did miss some other "ncols * nrows" places. Uploaded v3 patch to fix them.
As for the Windows, I didn't test it before but I think it should also have the issue (and happens more possible since `cellsadded` is also a long type).
My fix idea is simple: define a common long64 type for it.
I referred MSDN: only `LONGLONG` and `LONG64` are 64 bytes. And I assume Postgres should already have a similar type, but only found `typedef long int int64` in src/include/c.h, looks it's not a proper choose.
@Michael Paquier<mailto:[email protected]>, could you help to give some advices here (which type should be used? or should define a new one?). Thank you very much.
________________________________
From: Tom Lane <[email protected]>
Sent: Tuesday, September 12, 2023 12:19
To: Michael Paquier <[email protected]>
Cc: Hongxu Ma <[email protected]>; Jelte Fennema <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
Subject: Re: PSQL error: total cell count of XXX exceeded
Michael Paquier <[email protected]> writes:
> On Tue, Sep 12, 2023 at 02:39:55AM +0000, Hongxu Ma wrote:
> + long total_cells;
> long is 4 bytes on Windows, and 8 bytes basically elsewhere. So you
> would still have the same problem on Windows, no?
More to the point: what about the multiplication in printTableInit?
The cat's been out of the bag for quite some time before we get to
printTableAddCell.
I'm more than a bit skeptical about trying to do something about this,
simply because this range of query result sizes is far past what is
practical. The OP clearly hasn't tested his patch on actually
overflowing query results, and I don't care to either.
regards, tom lane
Attachments:
[application/octet-stream] v3-0001-Using-long-type-in-printTableAddCell.patch (2.7K, ../../TYBP286MB0351C97FB2A217D927E9C332B4F0A@TYBP286MB0351.JPNP286.PROD.OUTLOOK.COM/3-v3-0001-Using-long-type-in-printTableAddCell.patch)
download | inline diff:
From 19c8c53949959a0bac2408268d8709c8930e042d Mon Sep 17 00:00:00 2001
From: interma <[email protected]>
Date: Mon, 11 Sep 2023 14:42:14 +0800
Subject: [PATCH] Using long type in printTableAddCell() to prevent int
overflow
---
src/fe_utils/print.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index 7af1ccb6b5..5c0748df10 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -3172,6 +3172,8 @@ void
printTableInit(printTableContent *const content, const printTableOpt *opt,
const char *title, const int ncolumns, const int nrows)
{
+ long total_cells;
+
content->opt = opt;
content->title = title;
content->ncolumns = ncolumns;
@@ -3179,7 +3181,8 @@ printTableInit(printTableContent *const content, const printTableOpt *opt,
content->headers = pg_malloc0((ncolumns + 1) * sizeof(*content->headers));
- content->cells = pg_malloc0((ncolumns * nrows + 1) * sizeof(*content->cells));
+ total_cells = (long)ncolumns * (long)nrows;
+ content->cells = pg_malloc0((total_cells + 1) * sizeof(*content->cells));
content->cellmustfree = NULL;
content->footers = NULL;
@@ -3249,15 +3252,21 @@ void
printTableAddCell(printTableContent *const content, char *cell,
const bool translate, const bool mustfree)
{
+ long total_cells;
#ifndef ENABLE_NLS
(void) translate; /* unused parameter */
#endif
- if (content->cellsadded >= content->ncolumns * content->nrows)
+ /*
+ * total_cells is the product of ncolumns and nrows
+ * Using long type here to prevent int overflow
+ */
+ total_cells = (long)content->ncolumns * (long)content->nrows;
+ if (content->cellsadded >= total_cells)
{
fprintf(stderr, _("Cannot add cell to table content: "
- "total cell count of %d exceeded.\n"),
- content->ncolumns * content->nrows);
+ "total cell count of %ld exceeded, cells added: %ld.\n"),
+ total_cells, content->cellsadded);
exit(EXIT_FAILURE);
}
@@ -3273,7 +3282,7 @@ printTableAddCell(printTableContent *const content, char *cell,
{
if (content->cellmustfree == NULL)
content->cellmustfree =
- pg_malloc0((content->ncolumns * content->nrows + 1) * sizeof(bool));
+ pg_malloc0((total_cells + 1) * sizeof(bool));
content->cellmustfree[content->cellsadded] = true;
}
@@ -3341,9 +3350,10 @@ printTableCleanup(printTableContent *const content)
{
if (content->cellmustfree)
{
- int i;
-
- for (i = 0; i < content->nrows * content->ncolumns; i++)
+ long i;
+ long total_cells;
+ total_cells = (long)content->ncolumns * (long)content->nrows;
+ for (i = 0; i < total_cells; i++)
{
if (content->cellmustfree[i])
free(unconstify(char *, content->cells[i]));
--
2.39.2 (Apple Git-143)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: PSQL error: total cell count of XXX exceeded
2023-09-12 04:19 Re: PSQL error: total cell count of XXX exceeded Tom Lane <[email protected]>
2023-09-13 02:22 ` Re: PSQL error: total cell count of XXX exceeded Hongxu Ma <[email protected]>
@ 2023-09-13 07:31 ` Hongxu Ma <[email protected]>
2023-11-21 14:33 ` Re: PSQL error: total cell count of XXX exceeded Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Hongxu Ma @ 2023-09-13 07:31 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers
After double check, looks `int64` of src/include/c.h is the proper type for it.
Uploaded the v4 patch to fix it.
Thanks.
________________________________
From: Hongxu Ma <[email protected]>
Sent: Wednesday, September 13, 2023 10:22
To: Tom Lane <[email protected]>; Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: PSQL error: total cell count of XXX exceeded
Thanks for pointing that, I did miss some other "ncols * nrows" places. Uploaded v3 patch to fix them.
As for the Windows, I didn't test it before but I think it should also have the issue (and happens more possible since `cellsadded` is also a long type).
My fix idea is simple: define a common long64 type for it.
I referred MSDN: only `LONGLONG` and `LONG64` are 64 bytes. And I assume Postgres should already have a similar type, but only found `typedef long int int64` in src/include/c.h, looks it's not a proper choose.
@Michael Paquier<mailto:[email protected]>, could you help to give some advices here (which type should be used? or should define a new one?). Thank you very much.
________________________________
From: Tom Lane <[email protected]>
Sent: Tuesday, September 12, 2023 12:19
To: Michael Paquier <[email protected]>
Cc: Hongxu Ma <[email protected]>; Jelte Fennema <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
Subject: Re: PSQL error: total cell count of XXX exceeded
Michael Paquier <[email protected]> writes:
> On Tue, Sep 12, 2023 at 02:39:55AM +0000, Hongxu Ma wrote:
> + long total_cells;
> long is 4 bytes on Windows, and 8 bytes basically elsewhere. So you
> would still have the same problem on Windows, no?
More to the point: what about the multiplication in printTableInit?
The cat's been out of the bag for quite some time before we get to
printTableAddCell.
I'm more than a bit skeptical about trying to do something about this,
simply because this range of query result sizes is far past what is
practical. The OP clearly hasn't tested his patch on actually
overflowing query results, and I don't care to either.
regards, tom lane
Attachments:
[application/octet-stream] v4-0001-Using-int64-type-for-the-number-of-total-cells.patch (3.4K, ../../TYBP286MB0351CA77974AFE5A689E3369B4F0A@TYBP286MB0351.JPNP286.PROD.OUTLOOK.COM/3-v4-0001-Using-int64-type-for-the-number-of-total-cells.patch)
download | inline diff:
From f322951005315f55beaae332b72359615342340b Mon Sep 17 00:00:00 2001
From: interma <[email protected]>
Date: Mon, 11 Sep 2023 14:42:14 +0800
Subject: [PATCH] Using int64 type for the number of total cells in
printTableAddCell() to prevent int overflow
---
src/fe_utils/print.c | 23 +++++++++++++++--------
src/include/fe_utils/print.h | 2 +-
2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index 7af1ccb6b5..d2ceafcdc4 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -3172,6 +3172,8 @@ void
printTableInit(printTableContent *const content, const printTableOpt *opt,
const char *title, const int ncolumns, const int nrows)
{
+ int64 total_cells;
+
content->opt = opt;
content->title = title;
content->ncolumns = ncolumns;
@@ -3179,7 +3181,8 @@ printTableInit(printTableContent *const content, const printTableOpt *opt,
content->headers = pg_malloc0((ncolumns + 1) * sizeof(*content->headers));
- content->cells = pg_malloc0((ncolumns * nrows + 1) * sizeof(*content->cells));
+ total_cells = (int64)ncolumns * (int64)nrows;
+ content->cells = pg_malloc0((total_cells + 1) * sizeof(*content->cells));
content->cellmustfree = NULL;
content->footers = NULL;
@@ -3249,15 +3252,18 @@ void
printTableAddCell(printTableContent *const content, char *cell,
const bool translate, const bool mustfree)
{
+ int64 total_cells;
#ifndef ENABLE_NLS
(void) translate; /* unused parameter */
#endif
- if (content->cellsadded >= content->ncolumns * content->nrows)
+ total_cells = (int64)content->ncolumns * (int64)content->nrows;
+ if (content->cellsadded >= total_cells)
{
fprintf(stderr, _("Cannot add cell to table content: "
- "total cell count of %d exceeded.\n"),
- content->ncolumns * content->nrows);
+ "total cell count of " INT64_FORMAT " "
+ "exceeded, cells added: " INT64_FORMAT ".\n"),
+ total_cells, content->cellsadded);
exit(EXIT_FAILURE);
}
@@ -3273,7 +3279,7 @@ printTableAddCell(printTableContent *const content, char *cell,
{
if (content->cellmustfree == NULL)
content->cellmustfree =
- pg_malloc0((content->ncolumns * content->nrows + 1) * sizeof(bool));
+ pg_malloc0((total_cells + 1) * sizeof(bool));
content->cellmustfree[content->cellsadded] = true;
}
@@ -3341,9 +3347,10 @@ printTableCleanup(printTableContent *const content)
{
if (content->cellmustfree)
{
- int i;
-
- for (i = 0; i < content->nrows * content->ncolumns; i++)
+ int64 i;
+ int64 total_cells;
+ total_cells = (int64)content->ncolumns * (int64)content->nrows;
+ for (i = 0; i < total_cells; i++)
{
if (content->cellmustfree[i])
free(unconstify(char *, content->cells[i]));
diff --git a/src/include/fe_utils/print.h b/src/include/fe_utils/print.h
index cc6652def9..6e96f1c26e 100644
--- a/src/include/fe_utils/print.h
+++ b/src/include/fe_utils/print.h
@@ -171,7 +171,7 @@ typedef struct printTableContent
const char **cells; /* NULL-terminated array of cell content
* strings */
const char **cell; /* Pointer to the last added cell */
- long cellsadded; /* Number of cells added this far */
+ int64 cellsadded; /* Number of cells added this far */
bool *cellmustfree; /* true for cells that need to be free()d */
printTableFooter *footers; /* Pointer to the first footer */
printTableFooter *footer; /* Pointer to the last added footer */
--
2.39.2 (Apple Git-143)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: PSQL error: total cell count of XXX exceeded
2023-09-12 04:19 Re: PSQL error: total cell count of XXX exceeded Tom Lane <[email protected]>
2023-09-13 02:22 ` Re: PSQL error: total cell count of XXX exceeded Hongxu Ma <[email protected]>
2023-09-13 07:31 ` Re: PSQL error: total cell count of XXX exceeded Hongxu Ma <[email protected]>
@ 2023-11-21 14:33 ` Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Alvaro Herrera @ 2023-11-21 14:33 UTC (permalink / raw)
To: Hongxu Ma <[email protected]>; +Cc: Michael Paquier <[email protected]>; pgsql-hackers
On 2023-Sep-13, Hongxu Ma wrote:
> After double check, looks `int64` of src/include/c.h is the proper type for it.
> Uploaded the v4 patch to fix it.
Right. I made a few more adjustments, including the additional overflow
check in printTableInit that Tom Lane suggested, and pushed this.
It's a bit annoying that the error recovery decision of this code is to
exit the process with an error. If somebody were to be interested in a
fun improvement exercise, it may be worth redoing the print.c API so
that it returns errors that psql can report and recover from, instead of
just closing the process.
TBH though, I've never hit that code in real usage.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: PSQL error: total cell count of XXX exceeded
2023-09-12 04:19 Re: PSQL error: total cell count of XXX exceeded Tom Lane <[email protected]>
@ 2023-11-20 20:48 ` Alvaro Herrera <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: Alvaro Herrera @ 2023-11-20 20:48 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Michael Paquier <[email protected]>; Hongxu Ma <[email protected]>; Jelte Fennema <[email protected]>; David G. Johnston <[email protected]>; pgsql-hackers
On 2023-Sep-12, Tom Lane wrote:
> I'm more than a bit skeptical about trying to do something about this,
> simply because this range of query result sizes is far past what is
> practical. The OP clearly hasn't tested his patch on actually
> overflowing query results, and I don't care to either.
I think we're bound to hit this limit at some point in the future, and
it seems easy enough to solve. I propose the attached, which is pretty
much what Hongxu last submitted, with some minor changes.
Having this make a difference requires some 128GB of RAM, so it's not a
piece of cake, but it's an amount that can be reasonably expected to be
physically installed in real machines nowadays.
(I first thought we could just use pg_mul_s32_overflow during
printTableInit and raise an error if that returns true, but that just
postpones the problem.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Subversion to GIT: the shortest path to happiness I've ever heard of
(Alexey Klyukin)
Attachments:
[text/x-diff] v5-0001-Avoid-overflow-in-fe_utils-printTable-API.patch (3.7K, ../../[email protected]/2-v5-0001-Avoid-overflow-in-fe_utils-printTable-API.patch)
download | inline diff:
From 75abe5a485532cbc03db1eade2e1a6099914f98f Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email protected]>
Date: Mon, 20 Nov 2023 17:35:18 +0100
Subject: [PATCH v5] Avoid overflow in fe_utils' printTable API
We were using 32-bit integer arithmetic to compute the total number of
cells, which can overflow when used with large resultsets. We still
don't allow more than 4 billion rows, but now the total number of cells
can exceed that.
Author: Hongxu Ma <[email protected]>
Discussion: https://postgr.es/m/TYBP286MB0351B057B101C90D7C1239E6B4E2A@TYBP286MB0351.JPNP286.PROD.OUTLOOK.COM
---
src/fe_utils/print.c | 22 ++++++++++++++--------
src/include/fe_utils/print.h | 2 +-
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index 7af1ccb6b5..565cbc6d13 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -3172,6 +3172,8 @@ void
printTableInit(printTableContent *const content, const printTableOpt *opt,
const char *title, const int ncolumns, const int nrows)
{
+ int64 total_cells;
+
content->opt = opt;
content->title = title;
content->ncolumns = ncolumns;
@@ -3179,7 +3181,8 @@ printTableInit(printTableContent *const content, const printTableOpt *opt,
content->headers = pg_malloc0((ncolumns + 1) * sizeof(*content->headers));
- content->cells = pg_malloc0((ncolumns * nrows + 1) * sizeof(*content->cells));
+ total_cells = (int64) ncolumns * nrows;
+ content->cells = pg_malloc0((total_cells + 1) * sizeof(*content->cells));
content->cellmustfree = NULL;
content->footers = NULL;
@@ -3249,15 +3252,17 @@ void
printTableAddCell(printTableContent *const content, char *cell,
const bool translate, const bool mustfree)
{
+ int64 total_cells;
+
#ifndef ENABLE_NLS
(void) translate; /* unused parameter */
#endif
- if (content->cellsadded >= content->ncolumns * content->nrows)
+ total_cells = (int64) content->ncolumns * content->nrows;
+ if (content->cellsadded >= total_cells)
{
- fprintf(stderr, _("Cannot add cell to table content: "
- "total cell count of %d exceeded.\n"),
- content->ncolumns * content->nrows);
+ fprintf(stderr, _("Cannot add cell to table content: total cell count of %lld exceeded.\n"),
+ (long long int) total_cells);
exit(EXIT_FAILURE);
}
@@ -3273,7 +3278,7 @@ printTableAddCell(printTableContent *const content, char *cell,
{
if (content->cellmustfree == NULL)
content->cellmustfree =
- pg_malloc0((content->ncolumns * content->nrows + 1) * sizeof(bool));
+ pg_malloc0((total_cells + 1) * sizeof(bool));
content->cellmustfree[content->cellsadded] = true;
}
@@ -3341,9 +3346,10 @@ printTableCleanup(printTableContent *const content)
{
if (content->cellmustfree)
{
- int i;
+ int64 total_cells;
- for (i = 0; i < content->nrows * content->ncolumns; i++)
+ total_cells = (int64) content->ncolumns * content->nrows;
+ for (int64 i = 0; i < total_cells; i++)
{
if (content->cellmustfree[i])
free(unconstify(char *, content->cells[i]));
diff --git a/src/include/fe_utils/print.h b/src/include/fe_utils/print.h
index 13ebb00362..a697d0ba8d 100644
--- a/src/include/fe_utils/print.h
+++ b/src/include/fe_utils/print.h
@@ -171,7 +171,7 @@ typedef struct printTableContent
const char **cells; /* NULL-terminated array of cell content
* strings */
const char **cell; /* Pointer to the last added cell */
- long cellsadded; /* Number of cells added this far */
+ int64 cellsadded; /* Number of cells added this far */
bool *cellmustfree; /* true for cells that need to be free()d */
printTableFooter *footers; /* Pointer to the first footer */
printTableFooter *footer; /* Pointer to the last added footer */
--
2.39.2
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-11-21 14:33 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-01-09 22:29 [PATCH v6 1/2] fix MAINTAIN privs for partitions Nathan Bossart <[email protected]>
2023-09-12 04:19 Re: PSQL error: total cell count of XXX exceeded Tom Lane <[email protected]>
2023-09-13 02:22 ` Re: PSQL error: total cell count of XXX exceeded Hongxu Ma <[email protected]>
2023-09-13 07:31 ` Re: PSQL error: total cell count of XXX exceeded Hongxu Ma <[email protected]>
2023-11-21 14:33 ` Re: PSQL error: total cell count of XXX exceeded Alvaro Herrera <[email protected]>
2023-11-20 20:48 ` Re: PSQL error: total cell count of XXX exceeded Alvaro Herrera <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox