A problem about ParamPathInfo for an AppendPath

Started by Richard Guoover 3 years ago4 messageshackers
Jump to latest
#1Richard Guo
guofenglinux@gmail.com

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

#2Richard Guo
guofenglinux@gmail.com
In reply to: Richard Guo (#1)
Re: A problem about ParamPathInfo for an AppendPath

On Tue, Dec 6, 2022 at 5:00 PM Richard Guo <guofenglinux@gmail.com> 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

Attachments:

v1-0001-Fix-ParamPathInfo-for-AppendPath.patchapplication/octet-stream; name=v1-0001-Fix-ParamPathInfo-for-AppendPath.patchDownload+41-12
#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Richard Guo (#2)
Re: A problem about ParamPathInfo for an AppendPath

Richard Guo <guofenglinux@gmail.com> 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

#4Richard Guo
guofenglinux@gmail.com
In reply to: Tom Lane (#3)
Re: A problem about ParamPathInfo for an AppendPath

On Fri, Mar 17, 2023 at 6:15 AM Tom Lane <tgl@sss.pgh.pa.us> 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