public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v5 1/2] fix MAINTAIN privs for partitions 6+ messages / 4 participants [nested] [flat]
* [PATCH v5 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 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] 6+ messages in thread
* [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: Reduce useless changes before reassembly during logical replication @ 2024-01-18 06:42 Bharath Rupireddy <[email protected]> 0 siblings, 1 reply; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ messages in thread
* Re: Expand applicability of aggregate's sortop optimization @ 2024-07-18 05:18 Andrei Lepikhov <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Andrei Lepikhov @ 2024-07-18 05:18 UTC (permalink / raw) To: Matthias van de Meent <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; David Rowley <[email protected]>; [email protected] On 17/7/2024 16:33, Matthias van de Meent wrote: > On Wed, 17 Jul 2024 at 05:29, Andrei Lepikhov <[email protected]> wrote: >> As I see, the code: >> aggsortop = fetch_agg_sort_op(aggref->aggfnoid); >> if (!OidIsValid(aggsortop)) >> return false; /* not a MIN/MAX aggregate */ >> >> used twice and can be evaluated earlier to avoid duplicated code. > > The code is structured like this to make sure we only start accessing > catalogs once we know that all other reasons to bail out from this > optimization indicate we can apply the opimization. You'll notice that > I've tried to put the cheapest checks that only use caller-supplied > information first, and catalog accesses only after that. After additional research I think I get the key misunderstanding why you did so: As I see, the checks: if (list_length(aggref->aggorder) > 1) return false; if (orderClause->tleSortGroupRef != curTarget->ressortgroupref) return false; not needed at all. You already have check: if (list_length(aggref->args) != 1) and this tells us, that if we have ordering like MIN(x ORDER BY <smth>), this <smth> ordering contains only aggregate argument x. Because if it contained some expression, the transformAggregateCall() would add this expression to agg->args by calling the transformSortClause() routine. The tleSortGroupRef is just exactly ressortgroupref - no need to recheck it one more time. Of course, it is suitable only for MIN/MAX aggregates, but we discuss only them right now. Am I wrong? If you want, you can place it as assertions (see the diff in attachment). -- regards, Andrei Lepikhov diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c index afb5445b77..e0cbe5c923 100644 --- a/src/backend/optimizer/plan/planagg.c +++ b/src/backend/optimizer/plan/planagg.c @@ -253,6 +253,20 @@ can_minmax_aggs(PlannerInfo *root, List **context) if (list_length(aggref->args) != 1) return false; /* it couldn't be MIN/MAX */ + /* + * We might implement the optimization when a FILTER clause is present + * by adding the filter to the quals of the generated subquery. For + * now, just punt. + */ + if (aggref->aggfilter != NULL) + return false; + + curTarget = (TargetEntry *) linitial(aggref->args); + + aggsortop = fetch_agg_sort_op(aggref->aggfnoid); + if (!OidIsValid(aggsortop)) + return false; /* not a MIN/MAX aggregate */ + /* * ORDER BY is usually irrelevant for MIN/MAX, but it can change the * outcome if the aggsortop's operator class recognizes non-identical @@ -267,22 +281,35 @@ can_minmax_aggs(PlannerInfo *root, List **context) * quickly. */ if (aggref->aggorder != NIL) - return false; - /* note: we do not care if DISTINCT is mentioned ... */ + { + SortGroupClause *orderClause; - /* - * We might implement the optimization when a FILTER clause is present - * by adding the filter to the quals of the generated subquery. For - * now, just punt. - */ - if (aggref->aggfilter != NULL) - return false; + /* + * If the order clause is the same column as the one we're + * aggregating, we can still use the index: It is undefined which + * value is MIN() or MAX(), as well as which value is first or + * last when sorted. So, we can still use the index IFF the + * aggregated expression equals the expression used in the + * ordering operation. + */ - aggsortop = fetch_agg_sort_op(aggref->aggfnoid); - if (!OidIsValid(aggsortop)) - return false; /* not a MIN/MAX aggregate */ + /* + * We only accept a single argument to min/max aggregates, + * orderings that have more clauses won't provide correct results. + */ + Assert(list_length(aggref->aggorder) == 1); + + orderClause = castNode(SortGroupClause, linitial(aggref->aggorder)); + + Assert(orderClause->tleSortGroupRef == curTarget->ressortgroupref); + + if (orderClause->sortop != aggsortop && + orderClause->sortop != get_commutator(aggsortop)) + return false; + } + + /* note: we do not care if DISTINCT is mentioned ... */ - curTarget = (TargetEntry *) linitial(aggref->args); if (contain_mutable_functions((Node *) curTarget->expr)) return false; /* not potentially indexable */ diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index a5596ab210..5d37510d64 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -1003,6 +1003,71 @@ select max(unique1) from tenk1 where unique1 > 42; 9999 (1 row) +-- When sorting on the column that's being aggregated, indexes can also be +-- used, but only when the aggregate's operator has the same ordering behavior +-- as the ORDER BY-clause, i.e. if it is in the same btree opclass as the one +-- chosen for the ORDER BY clause. +explain (costs off) + select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1; + QUERY PLAN +------------------------------------------------------------ + Result + InitPlan 1 + -> Limit + -> Index Only Scan using tenk1_unique1 on tenk1 + Index Cond: (unique1 IS NOT NULL) +(5 rows) + +select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1; + min +----- + 0 +(1 row) + +explain (costs off) + select max(unique1 ORDER BY unique1 USING <) from tenk1; + QUERY PLAN +--------------------------------------------------------------------- + Result + InitPlan 1 + -> Limit + -> Index Only Scan Backward using tenk1_unique1 on tenk1 + Index Cond: (unique1 IS NOT NULL) +(5 rows) + +select max(unique1 ORDER BY unique1 USING <) from tenk1; + max +------ + 9999 +(1 row) + +explain (costs off) + select max(unique1 ORDER BY tenthous) from tenk1; + QUERY PLAN +------------------------------- + Aggregate + -> Sort + Sort Key: tenthous + -> Seq Scan on tenk1 +(4 rows) + +select max(unique1 ORDER BY tenthous) from tenk1; + max +------ + 9999 +(1 row) + +-- But even then, the index can't be used if we order by multiple columns. +explain (costs off) + select max(unique1 ORDER BY unique1, tenthous) from tenk1; + QUERY PLAN +------------------------------------- + Aggregate + -> Sort + Sort Key: unique1, tenthous + -> Seq Scan on tenk1 +(4 rows) + -- the planner may choose a generic aggregate here if parallel query is -- enabled, since that plan will be parallel safe and the "optimized" -- plan, which has almost identical cost, will not be. we want to test diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index ca6d1bcfb7..5cdf336fc0 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -368,6 +368,24 @@ explain (costs off) select max(unique1) from tenk1 where unique1 > 42; select max(unique1) from tenk1 where unique1 > 42; +-- When sorting on the column that's being aggregated, indexes can also be +-- used, but only when the aggregate's operator has the same ordering behavior +-- as the ORDER BY-clause, i.e. if it is in the same btree opclass as the one +-- chosen for the ORDER BY clause. +explain (costs off) + select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1; +select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1; +explain (costs off) + select max(unique1 ORDER BY unique1 USING <) from tenk1; +select max(unique1 ORDER BY unique1 USING <) from tenk1; +explain (costs off) + select max(unique1 ORDER BY tenthous) from tenk1; +select max(unique1 ORDER BY tenthous) from tenk1; + +-- But even then, the index can't be used if we order by multiple columns. +explain (costs off) + select max(unique1 ORDER BY unique1, tenthous) from tenk1; + -- the planner may choose a generic aggregate here if parallel query is -- enabled, since that plan will be parallel safe and the "optimized" -- plan, which has almost identical cost, will not be. we want to test Attachments: [text/plain] rewritten-patch-v2.diff (6.3K, ../../[email protected]/2-rewritten-patch-v2.diff) download | inline diff: diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c index afb5445b77..e0cbe5c923 100644 --- a/src/backend/optimizer/plan/planagg.c +++ b/src/backend/optimizer/plan/planagg.c @@ -253,6 +253,20 @@ can_minmax_aggs(PlannerInfo *root, List **context) if (list_length(aggref->args) != 1) return false; /* it couldn't be MIN/MAX */ + /* + * We might implement the optimization when a FILTER clause is present + * by adding the filter to the quals of the generated subquery. For + * now, just punt. + */ + if (aggref->aggfilter != NULL) + return false; + + curTarget = (TargetEntry *) linitial(aggref->args); + + aggsortop = fetch_agg_sort_op(aggref->aggfnoid); + if (!OidIsValid(aggsortop)) + return false; /* not a MIN/MAX aggregate */ + /* * ORDER BY is usually irrelevant for MIN/MAX, but it can change the * outcome if the aggsortop's operator class recognizes non-identical @@ -267,22 +281,35 @@ can_minmax_aggs(PlannerInfo *root, List **context) * quickly. */ if (aggref->aggorder != NIL) - return false; - /* note: we do not care if DISTINCT is mentioned ... */ + { + SortGroupClause *orderClause; - /* - * We might implement the optimization when a FILTER clause is present - * by adding the filter to the quals of the generated subquery. For - * now, just punt. - */ - if (aggref->aggfilter != NULL) - return false; + /* + * If the order clause is the same column as the one we're + * aggregating, we can still use the index: It is undefined which + * value is MIN() or MAX(), as well as which value is first or + * last when sorted. So, we can still use the index IFF the + * aggregated expression equals the expression used in the + * ordering operation. + */ - aggsortop = fetch_agg_sort_op(aggref->aggfnoid); - if (!OidIsValid(aggsortop)) - return false; /* not a MIN/MAX aggregate */ + /* + * We only accept a single argument to min/max aggregates, + * orderings that have more clauses won't provide correct results. + */ + Assert(list_length(aggref->aggorder) == 1); + + orderClause = castNode(SortGroupClause, linitial(aggref->aggorder)); + + Assert(orderClause->tleSortGroupRef == curTarget->ressortgroupref); + + if (orderClause->sortop != aggsortop && + orderClause->sortop != get_commutator(aggsortop)) + return false; + } + + /* note: we do not care if DISTINCT is mentioned ... */ - curTarget = (TargetEntry *) linitial(aggref->args); if (contain_mutable_functions((Node *) curTarget->expr)) return false; /* not potentially indexable */ diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index a5596ab210..5d37510d64 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -1003,6 +1003,71 @@ select max(unique1) from tenk1 where unique1 > 42; 9999 (1 row) +-- When sorting on the column that's being aggregated, indexes can also be +-- used, but only when the aggregate's operator has the same ordering behavior +-- as the ORDER BY-clause, i.e. if it is in the same btree opclass as the one +-- chosen for the ORDER BY clause. +explain (costs off) + select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1; + QUERY PLAN +------------------------------------------------------------ + Result + InitPlan 1 + -> Limit + -> Index Only Scan using tenk1_unique1 on tenk1 + Index Cond: (unique1 IS NOT NULL) +(5 rows) + +select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1; + min +----- + 0 +(1 row) + +explain (costs off) + select max(unique1 ORDER BY unique1 USING <) from tenk1; + QUERY PLAN +--------------------------------------------------------------------- + Result + InitPlan 1 + -> Limit + -> Index Only Scan Backward using tenk1_unique1 on tenk1 + Index Cond: (unique1 IS NOT NULL) +(5 rows) + +select max(unique1 ORDER BY unique1 USING <) from tenk1; + max +------ + 9999 +(1 row) + +explain (costs off) + select max(unique1 ORDER BY tenthous) from tenk1; + QUERY PLAN +------------------------------- + Aggregate + -> Sort + Sort Key: tenthous + -> Seq Scan on tenk1 +(4 rows) + +select max(unique1 ORDER BY tenthous) from tenk1; + max +------ + 9999 +(1 row) + +-- But even then, the index can't be used if we order by multiple columns. +explain (costs off) + select max(unique1 ORDER BY unique1, tenthous) from tenk1; + QUERY PLAN +------------------------------------- + Aggregate + -> Sort + Sort Key: unique1, tenthous + -> Seq Scan on tenk1 +(4 rows) + -- the planner may choose a generic aggregate here if parallel query is -- enabled, since that plan will be parallel safe and the "optimized" -- plan, which has almost identical cost, will not be. we want to test diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index ca6d1bcfb7..5cdf336fc0 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -368,6 +368,24 @@ explain (costs off) select max(unique1) from tenk1 where unique1 > 42; select max(unique1) from tenk1 where unique1 > 42; +-- When sorting on the column that's being aggregated, indexes can also be +-- used, but only when the aggregate's operator has the same ordering behavior +-- as the ORDER BY-clause, i.e. if it is in the same btree opclass as the one +-- chosen for the ORDER BY clause. +explain (costs off) + select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1; +select min(unique1 ORDER BY unique1 ASC NULLS LAST) from tenk1; +explain (costs off) + select max(unique1 ORDER BY unique1 USING <) from tenk1; +select max(unique1 ORDER BY unique1 USING <) from tenk1; +explain (costs off) + select max(unique1 ORDER BY tenthous) from tenk1; +select max(unique1 ORDER BY tenthous) from tenk1; + +-- But even then, the index can't be used if we order by multiple columns. +explain (costs off) + select max(unique1 ORDER BY unique1, tenthous) from tenk1; + -- the planner may choose a generic aggregate here if parallel query is -- enabled, since that plan will be parallel safe and the "optimized" -- plan, which has almost identical cost, will not be. we want to test ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-07-18 05:18 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 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]> 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]> 2024-07-18 05:18 Re: Expand applicability of aggregate's sortop optimization Andrei Lepikhov <[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