A problem about ParamPathInfo for an AppendPath

2022-12-06 Thread Richard Guo
While trying the codes of MemoizePath I noticed that MemoizePath cannot
be generated if inner path is a union-all AppendPath, even if it is
parameterized.  And I found the problem is that for a union-all
AppendPath the ParamPathInfo is not created in the usual way.  Instead
it is created with get_appendrel_parampathinfo, which just creates a
struct with no ppi_clauses.  As a result, MemoizePath cannot find a
proper cache key to use.

This problem does not exist for AppendPath generated for a partitioned
table though, because in this case we always create a normal param_info
with ppi_clauses, for use in run-time pruning.

As an illustration, we can use the table 'prt' in sql/memoize.sql with
the settings below

set enable_hashjoin to off;
set enable_mergejoin to off;
set enable_seqscan to off;
set enable_material to off;

explain (costs off) select * from prt_p1 t1 join prt t2 on t1.a = t2.a;
QUERY PLAN
--
 Nested Loop
   ->  Index Only Scan using iprt_p1_a on prt_p1 t1
   ->  Memoize
 Cache Key: t1.a
 Cache Mode: logical
 ->  Append
   ->  Index Only Scan using iprt_p1_a on prt_p1 t2_1
 Index Cond: (a = t1.a)
   ->  Index Only Scan using iprt_p2_a on prt_p2 t2_2
 Index Cond: (a = t1.a)
(10 rows)

explain (costs off) select * from prt_p1 t1 join (select * from prt_p1
union all select * from prt_p2) t2 on t1.a = t2.a;
  QUERY PLAN
---
 Nested Loop
   ->  Index Only Scan using iprt_p1_a on prt_p1 t1
   ->  Append
 ->  Index Only Scan using iprt_p1_a on prt_p1
   Index Cond: (a = t1.a)
 ->  Index Only Scan using iprt_p2_a on prt_p2
   Index Cond: (a = t1.a)
(7 rows)

As we can see, MemoizePath can be generated for partitioned AppendPath
but not for union-all AppendPath.

For the fix I think we can relax the check in create_append_path and
always use get_baserel_parampathinfo if the parent is a baserel.

--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1266,7 +1266,7 @@ create_append_path(PlannerInfo *root,
 * partition, and it's not necessary anyway in that case.  Must skip it
if
 * we don't have "root", too.)
 */
-   if (root && rel->reloptkind == RELOPT_BASEREL &&
IS_PARTITIONED_REL(rel))
+   if (root && rel->reloptkind == RELOPT_BASEREL)
pathnode->path.param_info = get_baserel_parampathinfo(root,
  rel,

required_outer);

Thanks
Richard


Re: A problem about ParamPathInfo for an AppendPath

2022-12-12 Thread Richard Guo
On Tue, Dec 6, 2022 at 5:00 PM Richard Guo  wrote:

> As we can see, MemoizePath can be generated for partitioned AppendPath
> but not for union-all AppendPath.
>
> For the fix I think we can relax the check in create_append_path and
> always use get_baserel_parampathinfo if the parent is a baserel.
>

BTW, IIUC currently we don't generate any parameterized MergeAppend
paths, as explained in generate_orderedappend_paths.  So the codes that
gather information from a MergeAppend path's param_info for run-time
partition pruning in create_merge_append_plan seem unnecessary.

Attached is a patch for this change and the changes described upthread.

Thanks
Richard


v1-0001-Fix-ParamPathInfo-for-AppendPath.patch
Description: Binary data


Re: A problem about ParamPathInfo for an AppendPath

2023-03-16 Thread Tom Lane
Richard Guo  writes:
> Attached is a patch for this change and the changes described upthread.

Pushed.  I thought the comment needed to be completely rewritten not just
tweaked, and I felt it was probably reasonable to continue to exclude
dummy paths from getting the more expensive treatment.

regards, tom lane




Re: A problem about ParamPathInfo for an AppendPath

2023-03-16 Thread Richard Guo
On Fri, Mar 17, 2023 at 6:15 AM Tom Lane  wrote:

> Pushed.  I thought the comment needed to be completely rewritten not just
> tweaked, and I felt it was probably reasonable to continue to exclude
> dummy paths from getting the more expensive treatment.


Yes agreed.  Thanks for the changes and pushing.

Thanks
Richard