Potential Issue with Redundant Restriction Clauses in get_parameterized_baserel_size for PARTITIONED_REL
Hi,
The get_parameterized_baserel_size function does not differentiate for PARTITIONED_REL and always appends the rel's own restriction clauses. However, for PARTITIONED_REL, rel->tuples is computed in set_append_rel_size which comes from the sum of lived childrel->rows. It is important to note that childrel->rows is the estimated number of result tuples, meaning it already includes the effect of the rel's own restriction clauses.
Therefore, get_parameterized_baserel_size should not append the rel's own restriction clauses again for PARTITIONED_REL. It seems there might be a mistake here. Although I have not found any SQL that causes issues, this looks like a potential problem. The correct code should handle this situation appropriately like this.
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -5365,7 +5365,10 @@ get_parameterized_baserel_size(PlannerInfo *root, RelOptInfo *rel,
* restriction clauses. Note that we force the clauses to be treated as
* non-join clauses during selectivity estimation.
*/
- allclauses = list_concat_copy(param_clauses, rel->baserestrictinfo);
+ if (rel->reloptkind == RELOPT_BASEREL && IS_PARTITIONED_REL(rel))
+ allclauses = list_copy(param_clauses);
+ else
+ allclauses = list_concat_copy(param_clauses, rel->baserestrictinfo);
nrows = rel->tuples *
clauselist_selectivity(root,
allclauses,Regards,
Yajun Hu
On Tue, Nov 26, 2024 at 4:57 PM huyajun <hu_yajun@qq.com> wrote:
The get_parameterized_baserel_size function does not differentiate for PARTITIONED_REL and always appends the rel's own restriction clauses. However, for PARTITIONED_REL, rel->tuples is computed in set_append_rel_size which comes from the sum of lived childrel->rows. It is important to note that childrel->rows is the estimated number of result tuples, meaning it already includes the effect of the rel's own restriction clauses.
Generally speaking, rel->tuples is the number of 'raw tuples' on disk,
while rel->rows is the estimated number of tuples after applying
restriction clauses.
In this regard, it seems that get_parameterized_baserel_size() does
not do anything wrong.
I think it might be better to modify set_append_rel_size() to set an
appendrel's tuples to the sum of the tuples from each live child,
rather than to its rows. This would also help us adjust the estimate
for the number of distinct values in estimate_num_groups() for
appendrels using the new formula introduced in 84f9a35e3. There were
discussions as well as a patch for this about one year ago. Please
see [1]/messages/by-id/CAMbWs4-ocromEKMtVDH3RBMuAJQaQDK0qi4k6zOuvpOnMWZauw@mail.gmail.com.
[1]: /messages/by-id/CAMbWs4-ocromEKMtVDH3RBMuAJQaQDK0qi4k6zOuvpOnMWZauw@mail.gmail.com
Thanks
Richard
On Wed, 27 Nov 2024 at 02:20, Richard Guo <guofenglinux@gmail.com> wrote:
In this regard, it seems that get_parameterized_baserel_size() does
not do anything wrong.I think it might be better to modify set_append_rel_size() to set an
appendrel's tuples to the sum of the tuples from each live child,
rather than to its rows. This would also help us adjust the estimate
for the number of distinct values in estimate_num_groups() for
appendrels using the new formula introduced in 84f9a35e3. There were
discussions as well as a patch for this about one year ago. Please
see [1].
It would be good to understand why get_parameterized_baserel_size()
bothers accounting for the baserestrictinfo quals and does not just do
clauselist_selectivity() on param_clauses alone and multiply by
rel->rows (which should already account for the baserestrictinfo). The
varRelId being 0 shouldn't matter as the baserestrictinfo obviously
won't contain join quals. The "set_baserel_size_estimates must have
been applied already." comment does not seem to be true, so it kinda
does look like something is wrong here. If it was possible to do it
this way, it would be a bit more efficient too as it saves making
another List and saves rechecking the selectivity of the
baserestictinfo quals.
I tried the attached quick hack to see if anything in the regression
tests came up with a different answer when the function was coded as
described above. I didn't spend the time to see which answer is
correct, but it does both tests that raise a notice are UNION ALL
subqueries. e.g:
explain (costs off)
select * from
(select 0 as z) as t1
left join
(select true as a) as t2
on true,
lateral (select true as b
union all
select a as b) as t3
where b;
which gives:
NOTICE: nrows = 1, nrows2 = 2
I didn't develop an opinion on what set_append_rel_size() sets rel->tuples to.
David
Attachments:
get_parameterized_baserel_size.patchapplication/octet-stream; name=get_parameterized_baserel_size.patchDownload
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 2bb6db1df7..0e22ac4903 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -5358,6 +5358,7 @@ get_parameterized_baserel_size(PlannerInfo *root, RelOptInfo *rel,
{
List *allclauses;
double nrows;
+ double nrows2;
/*
* Estimate the number of rows returned by the parameterized scan, knowing
@@ -5372,7 +5373,19 @@ get_parameterized_baserel_size(PlannerInfo *root, RelOptInfo *rel,
rel->relid, /* do not use 0! */
JOIN_INNER,
NULL);
+
+ nrows2 = rel->rows *
+ clauselist_selectivity(root,
+ param_clauses,
+ rel->relid, /* do not use 0! */
+ JOIN_INNER,
+ NULL);
nrows = clamp_row_est(nrows);
+ nrows2 = clamp_row_est(nrows2);
+
+ if (nrows != nrows2)
+ elog(NOTICE, "nrows = %g, nrows2 = %g", nrows, nrows2);
+
/* For safety, make sure result is not more than the base estimate */
if (nrows > rel->rows)
nrows = rel->rows;
David Rowley <dgrowleyml@gmail.com> writes:
It would be good to understand why get_parameterized_baserel_size()
bothers accounting for the baserestrictinfo quals and does not just do
clauselist_selectivity() on param_clauses alone and multiply by
rel->rows (which should already account for the baserestrictinfo).
One reason is that clauselist_selectivity does not necessarily
treat all the clauses independently. For example, if "x > 1"
is in one list and "x < 4" is in the other, you'll get very
different (and worse) results if you keep the lists separate
and just multiply their selectivities together.
We do have per-RestrictInfo selectivity caching that eliminates
most of the apparent inefficiency in this.
regards, tom lane