public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v6 1/2] fix MAINTAIN privs for partitions 10+ messages / 6 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; 10+ 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] 10+ messages in thread
* [PATCH v5 1/2] fix MAINTAIN privs for partitions @ 2023-01-09 22:29 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 10+ 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 d9c5bf9a1d..21a9f88c70 100644 --- a/doc/src/sgml/ref/lock.sgml +++ b/doc/src/sgml/ref/lock.sgml @@ -176,7 +176,10 @@ LOCK [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [ * ] or <literal>TRUNCATE</literal> privileges on the target table. All other forms of <command>LOCK</command> are allowed with table-level <literal>UPDATE</literal>, <literal>DELETE</literal>, - or <literal>TRUNCATE</literal> privileges. + or <literal>TRUNCATE</literal> privileges. 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 99e68bff85..de2a975882 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 c4ed7efce3..5908733347 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 --AqsLC8rIMeq19msA Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v5-0002-fix-MAINTAIN-privs-for-TOAST-tables.patch" ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: PSQL error: total cell count of XXX exceeded @ 2023-09-12 04:19 Tom Lane <[email protected]> 0 siblings, 2 replies; 10+ 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] 10+ messages in thread
* Re: PSQL error: total cell count of XXX exceeded @ 2023-09-13 02:22 Hongxu Ma <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 10+ 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] 10+ messages in thread
* Re: PSQL error: total cell count of XXX exceeded @ 2023-09-13 07:31 Hongxu Ma <[email protected]> parent: Hongxu Ma <[email protected]> 0 siblings, 1 reply; 10+ 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] 10+ messages in thread
* Re: PSQL error: total cell count of XXX exceeded @ 2023-11-20 20:48 Alvaro Herrera <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 10+ 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] 10+ messages in thread
* Re: PSQL error: total cell count of XXX exceeded @ 2023-11-21 14:33 Alvaro Herrera <[email protected]> parent: Hongxu Ma <[email protected]> 0 siblings, 0 replies; 10+ 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] 10+ messages in thread
* Re: Reduce useless changes before reassembly during logical replication @ 2024-01-18 06:42 Bharath Rupireddy <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Bharath Rupireddy @ 2024-01-18 06:42 UTC (permalink / raw) To: li jie <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Amit Kapila <[email protected]> On Wed, Jan 17, 2024 at 11:45 AM li jie <[email protected]> wrote: > > Hi hackers, > > During logical replication, if there is a large write transaction, some > spill files will be written to disk, depending on the setting of > logical_decoding_work_mem. > > This behavior can effectively avoid OOM, but if the transaction > generates a lot of change before commit, a large number of files may > fill the disk. For example, you can update a TB-level table. > > However, I found an inelegant phenomenon. If the modified large table is not > published, its changes will also be written with a large number of spill files. > Look at an example below: Thanks. I agree that decoding and queuing the changes of unpublished tables' data into reorder buffer is an unnecessary task for walsender. It takes processing efforts (CPU overhead), consumes disk space and uses memory configured via logical_decoding_work_mem for a replication connection inefficiently. > Later you will see a large number of spill files in the > > We can see that table tbl_t1 is not published in mypub. It also won't be sent > downstream because it's not subscribed. > After the transaction is reorganized, the pgoutput decoding plugin filters out > changes to these unpublished relationships when sending logical changes. > See function pgoutput_change. Right. Here's my testing [1]. > Most importantly, if we filter out unpublished relationship-related > changes after constructing the changes but before queuing the changes > into a transaction, will it reduce the workload of logical decoding > and avoid disk > or memory growth as much as possible? Right. It can. > The patch in the attachment is a prototype, which can effectively reduce the > memory and disk space usage during logical replication. > > Design: > 1. Added a callback LogicalDecodeFilterByRelCB for the output plugin. > > 2. Added this callback function pgoutput_table_filter for the pgoutput plugin. > Its main implementation is based on the table filter in the > pgoutput_change function. > Its main function is to determine whether the change needs to be published based > on the parameters of the publication, and if not, filter it. > > 3. After constructing a change and before Queue a change into a transaction, > use RelidByRelfilenumber to obtain the relation associated with the change, > just like obtaining the relation in the ReorderBufferProcessTXN function. > > 4. Relation may be a toast, and there is no good way to get its real > table relation based on toast relation. Here, I get the real table oid > through toast relname, and then get the real table relation. > > 5. This filtering takes into account INSERT/UPDATE/INSERT. Other > changes have not been considered yet and can be expanded in the future. Design of this patch is based on the principle of logical decoding filtering things out early on and looks very similar to filter_prepare_cb_wrapper/pg_decode_filter_prepare and filter_by_origin_cb/pgoutput_origin_filter. Per my understanding this design looks okay unless I'm missing anything. > Test: > 1. Added a test case 034_table_filter.pl > 2. Like the case above, create two tables, the published table tbl_pub and > the non-published table tbl_t1 > 3. Insert 10,000 rows of toast data into tbl_t1 on the publisher, and use > pg_ls_replslotdir to record the total size of the slot directory > every second. > 4. Compare the size of the slot directory at the beginning of the > transaction(size1), > the size at the end of the transaction (size2), and the average > size of the entire process(size3). > 5. Assert(size1==size2==size3) I bet that the above test with 10K rows is going to take a noticeable time on some buildfarm members (it took 6 seconds on my dev system which is an AWS EC2 instance). And, the above test can get flaky. Therefore, IMO, the concrete way of testing this feature is by looking at the server logs for the following message using PostgreSQL::Test::Cluster log_contains(). +filter_done: + + if (result && RelationIsValid(relation)) + elog(DEBUG1, "logical filter change by table %s", RelationGetRelationName(relation)); + Here are some comments on the v1 patch: 1. @@ -1415,9 +1419,6 @@ pgoutput_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, TupleTableSlot *old_slot = NULL; TupleTableSlot *new_slot = NULL; - if (!is_publishable_relation(relation)) - return; - Instead of removing is_publishable_relation from pgoutput_change, I think it can just be turned into an assertion Assert(is_publishable_relation(relation));, no? 2. + switch (change->action) + { + /* intentionally fall through */ Perhaps, it must use /* FALLTHROUGH */ just like elsewhere in the code, otherwise a warning is thrown. 3. From commit message: Most of the code in the FilterByTable function is transplanted from the ReorderBufferProcessTXN function, which can be called before the ReorderBufferQueueChange function.It is I think the above note can just be above the FilterByTable function for better understanding. +static bool +FilterByTable(LogicalDecodingContext *ctx, ReorderBufferChange *change) +{ 4. Why is FilterByTable(ctx, change) call placed after DecodeXLogTuple in DecodeInsert, DecodeUpdate and DecodeDelete? Is there a use for decoded tuples done by DecodeXLogTuple in the new callback filter_by_table_cb? If not, can we move FilterByTable call before DecodeXLogTuple to avoid some more extra processing? 5. Why is ReorderBufferChange needed as a parameter to FilterByTable and filter_by_table_cb? Can't just the LogicalDecodingContext and relation name, the change action be enough to decide if the table is publishable or not? If done this way, it can avoid some more processing, no? 6. Please run pgindent and pgperltidy on the new source code and new TAP test file respectively. [1] HEAD: postgres=# BEGIN; BEGIN Time: 0.110 ms postgres=*# insert into tbl_t1 select i,repeat('xyzzy', i),repeat('abcba', i),repeat('dfds', i) from generate_series(0,99999) i; INSERT 0 100000 Time: 379488.265 ms (06:19.488) postgres=*# ubuntu:~/postgres/pg17/bin$ du -sh /home/ubuntu/postgres/pg17/bin/db17/pg_replslot/mysub 837M /home/ubuntu/postgres/pg17/bin/db17/pg_replslot/mysub ubuntu:~/postgres/pg17/bin$ du -sh /home/ubuntu/postgres/pg17/bin/db17 2.6G /home/ubuntu/postgres/pg17/bin/db17 PATCHED: postgres=# BEGIN; BEGIN Time: 0.105 ms postgres=*# insert into tbl_t1 select i,repeat('xyzzy', i),repeat('abcba', i),repeat('dfds', i) from generate_series(0,99999) i; INSERT 0 100000 Time: 380044.554 ms (06:20.045) ubuntu:~/postgres$ du -sh /home/ubuntu/postgres/pg17/bin/db17/pg_replslot/mysub 8.0K /home/ubuntu/postgres/pg17/bin/db17/pg_replslot/mysub ubuntu:~/postgres$ du -sh /home/ubuntu/postgres/pg17/bin/db17 1.8G /home/ubuntu/postgres/pg17/bin/db17 -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Reduce useless changes before reassembly during logical replication @ 2024-01-18 09:17 Amit Kapila <[email protected]> parent: Bharath Rupireddy <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Amit Kapila @ 2024-01-18 09:17 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: li jie <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Jan 18, 2024 at 12:12 PM Bharath Rupireddy <[email protected]> wrote: > > On Wed, Jan 17, 2024 at 11:45 AM li jie <[email protected]> wrote: > > > > Hi hackers, > > > > During logical replication, if there is a large write transaction, some > > spill files will be written to disk, depending on the setting of > > logical_decoding_work_mem. > > > > This behavior can effectively avoid OOM, but if the transaction > > generates a lot of change before commit, a large number of files may > > fill the disk. For example, you can update a TB-level table. > > > > However, I found an inelegant phenomenon. If the modified large table is not > > published, its changes will also be written with a large number of spill files. > > Look at an example below: > > Thanks. I agree that decoding and queuing the changes of unpublished > tables' data into reorder buffer is an unnecessary task for walsender. > It takes processing efforts (CPU overhead), consumes disk space and > uses memory configured via logical_decoding_work_mem for a replication > connection inefficiently. > This is all true but note that in successful cases (where the table is published) all the work done by FilterByTable(accessing caches, transaction-related stuff) can add noticeable overhead as anyway we do that later in pgoutput_change(). I think I gave the same comment earlier as well but didn't see any satisfactory answer or performance data for successful cases to back this proposal. Note, users can configure to stream_in_progress transactions in which case they shouldn't see such a big problem. However, I agree that if we can find some solution where there is no noticeable overhead then that would be worth considering. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Reduce useless changes before reassembly during logical replication @ 2024-01-18 11:14 Bharath Rupireddy <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Bharath Rupireddy @ 2024-01-18 11:14 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: li jie <[email protected]>; PostgreSQL Hackers <[email protected]> On Thu, Jan 18, 2024 at 2:47 PM Amit Kapila <[email protected]> wrote: > > On Thu, Jan 18, 2024 at 12:12 PM Bharath Rupireddy > <[email protected]> wrote: > > > > On Wed, Jan 17, 2024 at 11:45 AM li jie <[email protected]> wrote: > > > > > > Hi hackers, > > > > > > During logical replication, if there is a large write transaction, some > > > spill files will be written to disk, depending on the setting of > > > logical_decoding_work_mem. > > > > > > This behavior can effectively avoid OOM, but if the transaction > > > generates a lot of change before commit, a large number of files may > > > fill the disk. For example, you can update a TB-level table. > > > > > > However, I found an inelegant phenomenon. If the modified large table is not > > > published, its changes will also be written with a large number of spill files. > > > Look at an example below: > > > > Thanks. I agree that decoding and queuing the changes of unpublished > > tables' data into reorder buffer is an unnecessary task for walsender. > > It takes processing efforts (CPU overhead), consumes disk space and > > uses memory configured via logical_decoding_work_mem for a replication > > connection inefficiently. > > > > This is all true but note that in successful cases (where the table is > published) all the work done by FilterByTable(accessing caches, > transaction-related stuff) can add noticeable overhead as anyway we do > that later in pgoutput_change(). Right. Overhead for published tables need to be studied. A possible way is to mark the checks performed in FilterByTable/filter_by_table_cb and skip the same checks in pgoutput_change. I'm not sure if this works without any issues though. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-01-18 11:14 UTC | newest] Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-01-09 22:29 [PATCH v5 1/2] fix MAINTAIN privs for partitions Nathan Bossart <[email protected]> 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]> 2024-01-18 06:42 Re: Reduce useless changes before reassembly during logical replication Bharath Rupireddy <[email protected]> 2024-01-18 09:17 ` Re: Reduce useless changes before reassembly during logical replication Amit Kapila <[email protected]> 2024-01-18 11:14 ` Re: Reduce useless changes before reassembly during logical replication Bharath Rupireddy <[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