Thread: Potential Issue with Redundant Restriction Clauses in get_parameterized_baserel_size for PARTITIONED_REL
Potential Issue with Redundant Restriction Clauses in get_parameterized_baserel_size for PARTITIONED_REL
From
huyajun
Date:
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
Re: Potential Issue with Redundant Restriction Clauses in get_parameterized_baserel_size for PARTITIONED_REL
From
Richard Guo
Date:
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 ownrestriction clauses. However, for PARTITIONED_REL, rel->tuples is computed in set_append_rel_size which comes from thesum of lived childrel->rows. It is important to note that childrel->rows is the estimated number of result tuples, meaningit 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]. [1] https://postgr.es/m/CAMbWs4-ocromEKMtVDH3RBMuAJQaQDK0qi4k6zOuvpOnMWZauw@mail.gmail.com Thanks Richard
Re: Potential Issue with Redundant Restriction Clauses in get_parameterized_baserel_size for PARTITIONED_REL
From
Tom Lane
Date:
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