public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v8 2/8] Implement CLUSTER of partitioned table.. 6+ messages / 3 participants [nested] [flat]
* [PATCH v8 2/8] Implement CLUSTER of partitioned table.. @ 2020-06-07 21:58 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Justin Pryzby @ 2020-06-07 21:58 UTC (permalink / raw) This requires either specification of a partitioned index on which to cluster, or that an partitioned index was previously set clustered. --- doc/src/sgml/ref/cluster.sgml | 6 + src/backend/commands/cluster.c | 174 +++++++++++++++++++------- src/bin/psql/tab-complete.c | 1 + src/include/commands/cluster.h | 1 + src/test/regress/expected/cluster.out | 58 ++++++++- src/test/regress/sql/cluster.sql | 24 +++- 6 files changed, 209 insertions(+), 55 deletions(-) diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml index 5dd21a0189..fb5deddb35 100644 --- a/doc/src/sgml/ref/cluster.sgml +++ b/doc/src/sgml/ref/cluster.sgml @@ -192,6 +192,12 @@ CLUSTER [VERBOSE] are periodically reclustered. </para> + <para> + Clustering a partitioned table clusters each of its partitions using the + index partition of the given partitioned index or (if not specified) the + partitioned index marked as clustered. + </para> + </refsect1> <refsect1> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 096a06f7b3..9b6673867c 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -32,7 +32,9 @@ #include "catalog/index.h" #include "catalog/namespace.h" #include "catalog/objectaccess.h" +#include "catalog/partition.h" #include "catalog/pg_am.h" +#include "catalog/pg_inherits.h" #include "catalog/toasting.h" #include "commands/cluster.h" #include "commands/defrem.h" @@ -73,6 +75,9 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose, bool *pSwapToastByContent, TransactionId *pFreezeXid, MultiXactId *pCutoffMulti); static List *get_tables_to_cluster(MemoryContext cluster_context); +static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context, + Oid indexOid); +static void cluster_multiple_rels(List *rvs, int options); /*--------------------------------------------------------------------------- @@ -135,7 +140,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) AccessExclusiveLock, 0, RangeVarCallbackOwnsTable, NULL); - rel = table_open(tableOid, NoLock); + rel = table_open(tableOid, ShareUpdateExclusiveLock); /* * Reject clustering a remote temp table ... their local buffer @@ -146,14 +151,6 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot cluster temporary tables of other sessions"))); - /* - * Reject clustering a partitioned table. - */ - if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot cluster a partitioned table"))); - if (stmt->indexname == NULL) { ListCell *index; @@ -189,10 +186,34 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) } /* close relation, keep lock till commit */ - table_close(rel, NoLock); + table_close(rel, ShareUpdateExclusiveLock); - /* Do the job. */ - cluster_rel(tableOid, indexOid, ¶ms); + if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + { + /* Do the job. */ + cluster_rel(tableOid, indexOid, ¶ms); + } + else + { + List *rvs; + MemoryContext cluster_context; + + /* Refuse to hold strong locks in a user transaction */ + PreventInTransactionBlock(isTopLevel, "CLUSTER"); + + cluster_context = AllocSetContextCreate(PortalContext, + "Cluster", + ALLOCSET_DEFAULT_SIZES); + + rvs = get_tables_to_cluster_partitioned(cluster_context, indexOid); + cluster_multiple_rels(rvs, params.options); + + /* Start a new transaction for the cleanup work. */ + StartTransactionCommand(); + + /* Clean up working storage */ + MemoryContextDelete(cluster_context); + } } else { @@ -202,7 +223,6 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) */ MemoryContext cluster_context; List *rvs; - ListCell *rv; /* * We cannot run this form of CLUSTER inside a user transaction block; @@ -225,28 +245,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel) * cluster_context. */ rvs = get_tables_to_cluster(cluster_context); - - /* Commit to get out of starting transaction */ - PopActiveSnapshot(); - CommitTransactionCommand(); - - /* Ok, now that we've got them all, cluster them one by one */ - foreach(rv, rvs) - { - RelToCluster *rvtc = (RelToCluster *) lfirst(rv); - ClusterParams cluster_params = params; - - /* Start a new transaction for each relation. */ - StartTransactionCommand(); - /* functions in indexes may want a snapshot set */ - PushActiveSnapshot(GetTransactionSnapshot()); - /* Do the job. */ - cluster_params.options |= CLUOPT_RECHECK; - cluster_rel(rvtc->tableOid, rvtc->indexOid, - &cluster_params); - PopActiveSnapshot(); - CommitTransactionCommand(); - } + cluster_multiple_rels(rvs, params.options | CLUOPT_RECHECK_ISCLUSTERED); /* Start a new transaction for the cleanup work. */ StartTransactionCommand(); @@ -352,9 +351,10 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params) } /* - * Check that the index is still the one with indisclustered set. + * Check that the index is still the one with indisclustered set, if needed. */ - if (!get_index_isclustered(indexOid)) + if ((params->options & CLUOPT_RECHECK_ISCLUSTERED) != 0 && + !get_index_isclustered(indexOid)) { relation_close(OldHeap, AccessExclusiveLock); pgstat_progress_end_command(); @@ -398,8 +398,13 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params) /* Check heap and index are valid to cluster on */ if (OidIsValid(indexOid)) + { check_index_is_clusterable(OldHeap, indexOid, recheck, AccessExclusiveLock); + /* Mark the index as clustered */ + mark_index_clustered(OldHeap, indexOid, true); + } + /* * Quietly ignore the request if this is a materialized view which has not * been populated from its query. No harm is done because there is no data @@ -415,6 +420,14 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params) return; } + /* For a partitioned rel, we're done. */ + if (!RELKIND_HAS_STORAGE(get_rel_relkind(tableOid))) + { + relation_close(OldHeap, AccessExclusiveLock); + pgstat_progress_end_command(); + return; + } + /* * All predicate locks on the tuples or pages are about to be made * invalid, because we move tuples around. Promote them to relation @@ -483,6 +496,9 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD * the worst consequence of following broken HOT chains would be that we * might put recently-dead tuples out-of-order in the new table, and there * is little harm in that.) + * + * This also refuses to cluster on an "incomplete" partitioned index + * created with "ON ONLY". */ if (!OldIndex->rd_index->indisvalid) ereport(ERROR, @@ -507,12 +523,6 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal) Relation pg_index; ListCell *index; - /* Disallow applying to a partitioned table */ - if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot mark index clustered in partitioned table"))); - /* * If the index is already marked clustered, no need to do anything. */ @@ -584,10 +594,6 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose) TransactionId frozenXid; MultiXactId cutoffMulti; - /* Mark the correct index as clustered */ - if (OidIsValid(indexOid)) - mark_index_clustered(OldHeap, indexOid, true); - /* Remember info about rel before closing OldHeap */ relpersistence = OldHeap->rd_rel->relpersistence; is_system_catalog = IsSystemRelation(OldHeap); @@ -1582,3 +1588,77 @@ get_tables_to_cluster(MemoryContext cluster_context) return rvs; } + +/* + * Return a List of tables and associated index, where each index is a + * partition of the given index + */ +static List * +get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid) +{ + List *inhoids; + ListCell *lc; + List *rvs = NIL; + MemoryContext old_context; + + inhoids = find_all_inheritors(indexOid, NoLock, NULL); + + foreach(lc, inhoids) + { + Oid indexrelid = lfirst_oid(lc); + Oid relid = IndexGetRelation(indexrelid, false); + RelToCluster *rvtc; + + /* + * Partitioned rels are also processed by cluster_rel, to + * call check_index_is_clusterable() and mark_index_clustered(). + */ + + /* + * We have to build the list in a different memory context so it will + * survive the cross-transaction processing + */ + old_context = MemoryContextSwitchTo(cluster_context); + + rvtc = (RelToCluster *) palloc(sizeof(RelToCluster)); + rvtc->tableOid = relid; + rvtc->indexOid = indexrelid; + rvs = lappend(rvs, rvtc); + + MemoryContextSwitchTo(old_context); + } + + return rvs; +} + +/* Cluster each relation in a separate transaction */ +static void +cluster_multiple_rels(List *rvs, int options) +{ + ListCell *lc; + + /* Commit to get out of starting transaction */ + PopActiveSnapshot(); + CommitTransactionCommand(); + + /* Ok, now that we've got them all, cluster them one by one */ + foreach(lc, rvs) + { + RelToCluster *rvtc = (RelToCluster *) lfirst(lc); + ClusterParams cluster_params = { .options = options, }; + + /* Start a new transaction for each relation. */ + StartTransactionCommand(); + + /* functions in indexes may want a snapshot set */ + PushActiveSnapshot(GetTransactionSnapshot()); + + /* Do the job. */ + cluster_params.options |= CLUOPT_RECHECK; + cluster_rel(rvtc->tableOid, rvtc->indexOid, + &cluster_params); + + PopActiveSnapshot(); + CommitTransactionCommand(); + } +} diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 1e1c315bae..ba301a6b08 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -588,6 +588,7 @@ static const SchemaQuery Query_for_list_of_clusterables = { .catname = "pg_catalog.pg_class c", .selcondition = "c.relkind IN (" CppAsString2(RELKIND_RELATION) ", " + CppAsString2(RELKIND_PARTITIONED_TABLE) ", " CppAsString2(RELKIND_MATVIEW) ")", .viscondition = "pg_catalog.pg_table_is_visible(c.oid)", .namespace = "c.relnamespace", diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h index a941f2accd..c30ca01726 100644 --- a/src/include/commands/cluster.h +++ b/src/include/commands/cluster.h @@ -22,6 +22,7 @@ /* flag bits for ClusterParams->flags */ #define CLUOPT_RECHECK 0x01 /* recheck relation state */ #define CLUOPT_VERBOSE 0x02 /* print progress info */ +#define CLUOPT_RECHECK_ISCLUSTERED 0x04 /* recheck relation state for indisclustered */ /* options for CLUSTER */ typedef struct ClusterParams diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out index bdae8fe00c..c74cfa88cc 100644 --- a/src/test/regress/expected/cluster.out +++ b/src/test/regress/expected/cluster.out @@ -439,14 +439,62 @@ select * from clstr_temp; drop table clstr_temp; RESET SESSION AUTHORIZATION; --- Check that partitioned tables cannot be clustered +-- Check that partitioned tables can be clustered CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a); +CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a); +CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10); +CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a); +CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30); +CREATE TABLE clstrpart3 PARTITION OF clstrpart DEFAULT PARTITION BY RANGE(a); +CREATE TABLE clstrpart33 PARTITION OF clstrpart3 DEFAULT; +ALTER TABLE clstrpart SET WITHOUT CLUSTER; +CREATE INDEX clstrpart_only_idx ON ONLY clstrpart (a); +CLUSTER clstrpart USING clstrpart_only_idx; -- fails +ERROR: cannot cluster on invalid index "clstrpart_only_idx" +DROP INDEX clstrpart_only_idx; CREATE INDEX clstrpart_idx ON clstrpart (a); -ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; -ERROR: cannot mark index clustered in partitioned table +-- Check that clustering sets new relfilenodes: +CREATE TEMP TABLE old_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ; CLUSTER clstrpart USING clstrpart_idx; -ERROR: cannot cluster a partitioned table -DROP TABLE clstrpart; +CREATE TEMP TABLE new_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ; +SELECT relname, old.level, old.relkind, old.relfilenode = new.relfilenode FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname) ORDER BY relname COLLATE "C"; + relname | level | relkind | ?column? +-------------+-------+---------+---------- + clstrpart | 0 | p | t + clstrpart1 | 1 | p | t + clstrpart11 | 2 | r | f + clstrpart12 | 2 | p | t + clstrpart2 | 1 | r | f + clstrpart3 | 1 | p | t + clstrpart33 | 2 | r | f +(7 rows) + +-- Check that clustering sets new indisclustered: +SELECT relname, relkind, indisclustered FROM pg_partition_tree('clstrpart_idx'::regclass) AS tree JOIN pg_index i ON i.indexrelid=tree.relid JOIN pg_class c ON c.oid=indexrelid ORDER BY relname COLLATE "C"; + relname | relkind | indisclustered +-------------------+---------+---------------- + clstrpart11_a_idx | i | t + clstrpart12_a_idx | I | t + clstrpart1_a_idx | I | t + clstrpart2_a_idx | i | t + clstrpart33_a_idx | i | t + clstrpart3_a_idx | I | t + clstrpart_idx | I | t +(7 rows) + +CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned +CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs +CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf +\d clstrpart + Partitioned table "public.clstrpart" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition key: RANGE (a) +Indexes: + "clstrpart_idx" btree (a) CLUSTER +Number of partitions: 3 (Use \d+ to list them.) + -- Test CLUSTER with external tuplesorting create table clstr_4 as select * from tenk1; create index cluster_sort on clstr_4 (hundred, thousand, tenthous); diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql index 188183647c..9bcc77695c 100644 --- a/src/test/regress/sql/cluster.sql +++ b/src/test/regress/sql/cluster.sql @@ -196,12 +196,30 @@ drop table clstr_temp; RESET SESSION AUTHORIZATION; --- Check that partitioned tables cannot be clustered +-- Check that partitioned tables can be clustered CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a); +CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a); +CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10); +CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a); +CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30); +CREATE TABLE clstrpart3 PARTITION OF clstrpart DEFAULT PARTITION BY RANGE(a); +CREATE TABLE clstrpart33 PARTITION OF clstrpart3 DEFAULT; +ALTER TABLE clstrpart SET WITHOUT CLUSTER; +CREATE INDEX clstrpart_only_idx ON ONLY clstrpart (a); +CLUSTER clstrpart USING clstrpart_only_idx; -- fails +DROP INDEX clstrpart_only_idx; CREATE INDEX clstrpart_idx ON clstrpart (a); -ALTER TABLE clstrpart CLUSTER ON clstrpart_idx; +-- Check that clustering sets new relfilenodes: +CREATE TEMP TABLE old_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ; CLUSTER clstrpart USING clstrpart_idx; -DROP TABLE clstrpart; +CREATE TEMP TABLE new_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ; +SELECT relname, old.level, old.relkind, old.relfilenode = new.relfilenode FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname) ORDER BY relname COLLATE "C"; +-- Check that clustering sets new indisclustered: +SELECT relname, relkind, indisclustered FROM pg_partition_tree('clstrpart_idx'::regclass) AS tree JOIN pg_index i ON i.indexrelid=tree.relid JOIN pg_class c ON c.oid=indexrelid ORDER BY relname COLLATE "C"; +CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned +CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs +CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf +\d clstrpart -- Test CLUSTER with external tuplesorting -- 2.17.0 --O5XBE6gyVG5Rl6Rj Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0003-f-progress-reporting.patch" ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: ERROR: corrupt MVNDistinct entry @ 2024-12-25 08:14 Richard Guo <[email protected]> 2024-12-25 09:36 ` Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Richard Guo @ 2024-12-25 08:14 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: pgsql-hackers On Wed, Dec 25, 2024 at 11:34 AM Andrei Lepikhov <[email protected]> wrote: > I have a couple of notes. > 1. The nulling_relids provides us sensible information about possible > nulls inside the input. We are not using it to estimate the number of > such nulls for now. Does Your idea consist of obtaining 'clear' > statistics and reusing nulling_relids knowledge somewhere later? Are you referring to the nullfrac estimates? A RelOptInfo's nulling_relids records all above outer joins that can null this rel. However, I cannot see how it helps with nullfrac. > 2. It is ok for Vars. But what about expressions? We use equal() in > distinct, MCV and dependencies modules. Do we need to remove nulls > before using extended statistics as a general rule? AFAIU, the expressions in extended statistics are not decorated with any nullingrels bits, are they? Thanks Richard ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: ERROR: corrupt MVNDistinct entry 2024-12-25 08:14 Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> @ 2024-12-25 09:36 ` Richard Guo <[email protected]> 2024-12-25 16:14 ` Re: ERROR: corrupt MVNDistinct entry Andrei Lepikhov <[email protected]> 2024-12-27 04:16 ` Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> 0 siblings, 2 replies; 6+ messages in thread From: Richard Guo @ 2024-12-25 09:36 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: pgsql-hackers On Wed, Dec 25, 2024 at 5:14 PM Richard Guo <[email protected]> wrote: > On Wed, Dec 25, 2024 at 11:34 AM Andrei Lepikhov <[email protected]> wrote: > > 2. It is ok for Vars. But what about expressions? We use equal() in > > distinct, MCV and dependencies modules. Do we need to remove nulls > > before using extended statistics as a general rule? > > AFAIU, the expressions in extended statistics are not decorated with > any nullingrels bits, are they? I've just realized that there may be other places with similar issues, not just in estimate_num_groups. For instance, -- after v16 explain (costs on) select * from t t1 left join t t2 on true where (t2.a+t2.b) is null; QUERY PLAN -------------------------------------------------------------------- Nested Loop Left Join (cost=0.00..15032.50 rows=5000 width=16) Filter: ((t2.a + t2.b) IS NULL) -> Seq Scan on t t1 (cost=0.00..15.00 rows=1000 width=8) -> Materialize (cost=0.00..20.00 rows=1000 width=8) -> Seq Scan on t t2 (cost=0.00..15.00 rows=1000 width=8) (5 rows) -- before v16 explain (costs on) select * from t t1 left join t t2 on true where (t2.a+t2.b) is null; QUERY PLAN -------------------------------------------------------------------- Nested Loop Left Join (cost=0.00..15032.50 rows=1 width=16) Filter: ((t2.a + t2.b) IS NULL) -> Seq Scan on t t1 (cost=0.00..15.00 rows=1000 width=8) -> Materialize (cost=0.00..20.00 rows=1000 width=8) -> Seq Scan on t t2 (cost=0.00..15.00 rows=1000 width=8) (5 rows) In v16 and later, the nullingrels within the expression "t2.a + t2.b" prevent it from being matched to the corresponding expression in extended statistics, forcing us to use DEFAULT_UNK_SEL(0.005). It seems that we need to strip out the nullingrels bits from expressions before matching them to extended statistics or expressional index columns in more places. Thanks Richard ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: ERROR: corrupt MVNDistinct entry 2024-12-25 08:14 Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> 2024-12-25 09:36 ` Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> @ 2024-12-25 16:14 ` Andrei Lepikhov <[email protected]> 1 sibling, 0 replies; 6+ messages in thread From: Andrei Lepikhov @ 2024-12-25 16:14 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: pgsql-hackers; Tomas Vondra <[email protected]> On 25/12/2024 16:36, Richard Guo wrote: > On Wed, Dec 25, 2024 at 5:14 PM Richard Guo <[email protected]> wrote: >> On Wed, Dec 25, 2024 at 11:34 AM Andrei Lepikhov <[email protected]> wrote: >>> 2. It is ok for Vars. But what about expressions? We use equal() in >>> distinct, MCV and dependencies modules. Do we need to remove nulls >>> before using extended statistics as a general rule? >> >> AFAIU, the expressions in extended statistics are not decorated with >> any nullingrels bits, are they? > > I've just realized that there may be other places with similar issues, > not just in estimate_num_groups. For instance, I'm pleased to see that you've grasped my initially unclear idea. Yeah, it seems that all types of statistics may be lost because of varnullingrels. > In v16 and later, the nullingrels within the expression "t2.a + t2.b" > prevent it from being matched to the corresponding expression in > extended statistics, forcing us to use DEFAULT_UNK_SEL(0.005). > > It seems that we need to strip out the nullingrels bits from > expressions before matching them to extended statistics or > expressional index columns in more places. I think Tomas Vondra may have a decisive opinion in this place: we have already discussed some approaches to calculate NULLs generated by RHS of Left Join shortly. Maybe we can commit a quick cure like the one provided in your patch, but we should remember this example - it is not apparent to me how to estimate a group of clauses in the case when part of Vars has varnullingrels and part of them - doesn't. Also, I think this is a good example that an explain analyse summary could have some sort of extended statistics usage report. It can help to clearly identify cases when extended statistics don't work, but should. - something like already implemented in SQL Server. -- regards, Andrei Lepikhov ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: ERROR: corrupt MVNDistinct entry 2024-12-25 08:14 Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> 2024-12-25 09:36 ` Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> @ 2024-12-27 04:16 ` Richard Guo <[email protected]> 2024-12-31 08:40 ` Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> 1 sibling, 1 reply; 6+ messages in thread From: Richard Guo @ 2024-12-27 04:16 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: pgsql-hackers On Wed, Dec 25, 2024 at 6:36 PM Richard Guo <[email protected]> wrote: > In v16 and later, the nullingrels within the expression "t2.a + t2.b" > prevent it from being matched to the corresponding expression in > extended statistics, forcing us to use DEFAULT_UNK_SEL(0.005). Furthermore, even without extended statistics or expressional index columns, the nullingrels can still cause issues with selectivity estimates. They may cause examine_variable to fail in identifying a valid RelOptInfo for an expression, making the relation size not available, which is required in many cases. For instance, create table t (a int, b int); insert into t select i, i from generate_series(1,10)i; analyze t; -- in v16 and later explain (costs on) select * from t t1 left join t t2 on true left join t t3 on t1.a = t3.a and t3.a < 8 where t1.a = coalesce(t2.a); QUERY PLAN ----------------------------------------------------------------------- Nested Loop Left Join (cost=0.00..4.94 rows=1 width=24) Join Filter: (t1.a = t3.a) -> Nested Loop Left Join (cost=0.00..3.73 rows=1 width=16) Filter: (t1.a = COALESCE(t2.a)) -> Seq Scan on t t1 (cost=0.00..1.10 rows=10 width=8) -> Materialize (cost=0.00..1.15 rows=10 width=8) -> Seq Scan on t t2 (cost=0.00..1.10 rows=10 width=8) -> Seq Scan on t t3 (cost=0.00..1.12 rows=7 width=8) Filter: (a < 8) (9 rows) -- in v15 and before explain (costs on) select * from t t1 left join t t2 on true left join t t3 on t1.a = t3.a and t3.a < 8 where t1.a = coalesce(t2.a); QUERY PLAN ----------------------------------------------------------------------- Hash Left Join (cost=1.21..5.04 rows=10 width=24) Hash Cond: (t1.a = t3.a) -> Nested Loop Left Join (cost=0.00..3.73 rows=10 width=16) Filter: (t1.a = COALESCE(t2.a)) -> Seq Scan on t t1 (cost=0.00..1.10 rows=10 width=8) -> Materialize (cost=0.00..1.15 rows=10 width=8) -> Seq Scan on t t2 (cost=0.00..1.10 rows=10 width=8) -> Hash (cost=1.12..1.12 rows=7 width=8) -> Seq Scan on t t3 (cost=0.00..1.12 rows=7 width=8) Filter: (a < 8) (10 rows) In v16 and later, when calculating the join selectivity for "t1.a = coalesce(t2.a)", eqjoinsel sets nd2 — the number of distinct values of coalesce(t2.a) — to DEFAULT_NUM_DISTINCT (200) because the corresponding RelOptInfo is not identifiable. This results in very inaccurate join selectivity. I'm wondering if we also need to strip out the nullingrels from the expression in examine_variable(). I tried doing so and noticed a plan diff in regression test join.sql. @@ -2573,10 +2573,11 @@ -> Materialize -> Seq Scan on int4_tbl t2 Filter: (f1 > 1) - -> Seq Scan on int4_tbl t3 + -> Materialize + -> Seq Scan on int4_tbl t3 -> Materialize -> Seq Scan on int4_tbl t4 -(13 rows) +(14 rows) Thanks Richard ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: ERROR: corrupt MVNDistinct entry 2024-12-25 08:14 Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> 2024-12-25 09:36 ` Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> 2024-12-27 04:16 ` Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> @ 2024-12-31 08:40 ` Richard Guo <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Richard Guo @ 2024-12-31 08:40 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: pgsql-hackers On Fri, Dec 27, 2024 at 1:16 PM Richard Guo <[email protected]> wrote: > I'm wondering if we also need to strip out the nullingrels from the > expression in examine_variable(). I tried doing so and noticed a plan > diff in regression test join.sql. Here is an updated patch that implements this change, and also moves the removal of the nullingrels within grouping expressions from estimate_num_groups to add_unique_group_var. I'm not quite sure how this change affects the exprs_known_equal call in add_unique_group_var, possibly making it even less precise (it is alreay somewhat fuzzy, as we do not provide a valid opfamily). However, I think it should be fine for estimation purposes. Moreover, in many cases, such as with groupClause, distinctClause, and WindowClause's PARTITION BY clauses, we have already removed any items that can be proven redundant. Regarding the back-patch, this patch is a bug fix, which suggests it should be back-patched. However, it also changes some plans by fixing the cost estimation. Does anyone know what our usual approach is in this situation? Thanks Richard Attachments: [application/octet-stream] v2-0001-Ignore-nullingrels-when-looking-up-statistics.patch (5.8K, ../../CAMbWs48iYx=pxBP8O1NLz8wnbrQew3QJcMJOnmnNEFJzs+EAww@mail.gmail.com/2-v2-0001-Ignore-nullingrels-when-looking-up-statistics.patch) download | inline diff: From 1a8786a75666094d015b33abca4dbb95a0ad72a8 Mon Sep 17 00:00:00 2001 From: Richard Guo <[email protected]> Date: Tue, 31 Dec 2024 12:29:36 +0900 Subject: [PATCH v2] Ignore nullingrels when looking up statistics When looking up statistical data about an expression, we do not need to concern ourselves with the outer joins that could null the Vars/PHVs contained in the expression. Accounting for nullingrels in the expression could cause estimate_num_groups to count the same Var multiple times if it's marked with different nullingrels. This is incorrect, and could lead to "ERROR: corrupt MVNDistinct entry" when searching for multivariate n-distinct. Furthermore, the nullingrels could prevent us from matching an expression to expressional index columns or to the expressions in extended statistics, leading to inaccurate estimates. To fix, strip out all the nullingrels from the expression before we look up statistical data about it. --- src/backend/utils/adt/selfuncs.c | 30 +++++++++++++++++++++++++++--- src/test/regress/expected/join.out | 26 ++++++++++++++++++++++++-- src/test/regress/sql/join.sql | 13 +++++++++++++ 3 files changed, 64 insertions(+), 5 deletions(-) diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index 08fa6774d9..aa38b60da7 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -121,6 +121,7 @@ #include "parser/parse_clause.h" #include "parser/parse_relation.h" #include "parser/parsetree.h" +#include "rewrite/rewriteManip.h" #include "statistics/statistics.h" #include "storage/bufmgr.h" #include "utils/acl.h" @@ -3306,6 +3307,15 @@ add_unique_group_var(PlannerInfo *root, List *varinfos, ndistinct = get_variable_numdistinct(vardata, &isdefault); + /* + * The nullingrels bits within the var could cause the same var to be + * counted multiple times if it is marked with different nullingrels. + * They could also prevent us from matching the var to the expressions in + * extended statistics (see estimate_multivariate_ndistinct). So strip + * them out first. + */ + var = remove_nulling_relids(var, root->outer_join_rels, NULL); + foreach(lc, varinfos) { varinfo = (GroupVarInfo *) lfirst(lc); @@ -5077,7 +5087,13 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid, { int relid; - if (bms_get_singleton_member(varnos, &relid)) + /* + * Check if the expression is in vars of a single base relation. We + * need to exclude the OJs that might be present in the expression. + */ + if (bms_get_singleton_member(bms_difference(varnos, + root->outer_join_rels), + &relid)) { if (varRelid == 0 || varRelid == relid) { @@ -5107,8 +5123,6 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid, } } - bms_free(varnos); - vardata->var = node; vardata->atttype = exprType(node); vardata->atttypmod = exprTypmod(node); @@ -5132,6 +5146,14 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid, ListCell *slist; Oid userid; + /* + * The nullingrels bits within the expression could prevent us from + * matching it to expressional index columns or to the expressions in + * extended statistics. So strip them out first. + */ + if (bms_overlap(varnos, root->outer_join_rels)) + node = remove_nulling_relids(node, root->outer_join_rels, NULL); + /* * Determine the user ID to use for privilege checks: either * onerel->userid if it's set (e.g., in case we're accessing the table @@ -5402,6 +5424,8 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid, } } } + + bms_free(varnos); } /* diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 0c9b312eaf..079fcf46f0 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -2573,10 +2573,11 @@ where t1.f1 = coalesce(t2.f1, 1); -> Materialize -> Seq Scan on int4_tbl t2 Filter: (f1 > 1) - -> Seq Scan on int4_tbl t3 + -> Materialize + -> Seq Scan on int4_tbl t3 -> Materialize -> Seq Scan on int4_tbl t4 -(13 rows) +(14 rows) explain (costs off) select * from int4_tbl t1 @@ -8217,3 +8218,24 @@ SELECT * FROM rescan_bhs t1 LEFT JOIN rescan_bhs t2 ON t1.a IN RESET enable_seqscan; RESET enable_indexscan; +-- Test that we do not account for nullingrels when looking up statistics +CREATE TABLE group_tbl (a INT, b INT); +INSERT INTO group_tbl SELECT 1, 1; +CREATE STATISTICS group_tbl_stat (ndistinct) ON a, b FROM group_tbl; +ANALYZE group_tbl; +EXPLAIN (COSTS OFF) +SELECT 1 FROM group_tbl t1 + LEFT JOIN (SELECT a c1, COALESCE(a) c2 FROM group_tbl t2) s ON TRUE +GROUP BY s.c1, s.c2; + QUERY PLAN +-------------------------------------------- + Group + Group Key: t2.a, (COALESCE(t2.a)) + -> Sort + Sort Key: t2.a, (COALESCE(t2.a)) + -> Nested Loop Left Join + -> Seq Scan on group_tbl t1 + -> Seq Scan on group_tbl t2 +(7 rows) + +DROP TABLE group_tbl; diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 8cfc1053cb..779d56cb30 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -3033,3 +3033,16 @@ SELECT * FROM rescan_bhs t1 LEFT JOIN rescan_bhs t2 ON t1.a IN RESET enable_seqscan; RESET enable_indexscan; + +-- Test that we do not account for nullingrels when looking up statistics +CREATE TABLE group_tbl (a INT, b INT); +INSERT INTO group_tbl SELECT 1, 1; +CREATE STATISTICS group_tbl_stat (ndistinct) ON a, b FROM group_tbl; +ANALYZE group_tbl; + +EXPLAIN (COSTS OFF) +SELECT 1 FROM group_tbl t1 + LEFT JOIN (SELECT a c1, COALESCE(a) c2 FROM group_tbl t2) s ON TRUE +GROUP BY s.c1, s.c2; + +DROP TABLE group_tbl; -- 2.43.0 ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-12-31 08:40 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-06-07 21:58 [PATCH v8 2/8] Implement CLUSTER of partitioned table.. Justin Pryzby <[email protected]> 2024-12-25 08:14 Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> 2024-12-25 09:36 ` Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> 2024-12-25 16:14 ` Re: ERROR: corrupt MVNDistinct entry Andrei Lepikhov <[email protected]> 2024-12-27 04:16 ` Re: ERROR: corrupt MVNDistinct entry Richard Guo <[email protected]> 2024-12-31 08:40 ` Re: ERROR: corrupt MVNDistinct entry Richard Guo <[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