Trivial revise for the check of parameterized partial paths
While working on the invalid parameterized join path issue [1]/messages/by-id/CAJKUy5g2uZRrUDZJ8p-=giwcSHVUn0c9nmdxPSY0jF0Ov8VoEA@mail.gmail.com, I
noticed that we can simplify the codes for checking parameterized
partial paths in try_partial_hashjoin/mergejoin_path, with the help of
macro PATH_REQ_OUTER.
- if (inner_path->param_info != NULL)
- {
- Relids inner_paramrels =
inner_path->param_info->ppi_req_outer;
-
- if (!bms_is_empty(inner_paramrels))
- return;
- }
+ if (!bms_is_empty(PATH_REQ_OUTER(inner_path)))
+ return;
Also there is a comment there that is not correct.
* If the inner path is parameterized, the parameterization must be fully
* satisfied by the proposed outer path.
This is true for nestloop but not for hashjoin/mergejoin.
Besides, I wonder if it'd be better that we verify that the outer input
path for a partial join path should not have any parameterization
dependency.
Attached is a patch for all these changes.
[1]: /messages/by-id/CAJKUy5g2uZRrUDZJ8p-=giwcSHVUn0c9nmdxPSY0jF0Ov8VoEA@mail.gmail.com
/messages/by-id/CAJKUy5g2uZRrUDZJ8p-=giwcSHVUn0c9nmdxPSY0jF0Ov8VoEA@mail.gmail.com
Thanks
Richard
Attachments:
v1-0001-Trivial-revise-for-the-check-of-parameterized-partial-paths.patchapplication/octet-stream; name=v1-0001-Trivial-revise-for-the-check-of-parameterized-partial-paths.patchDownload
From 6a36b7925f6324cdf3a4e84b2af02b7c522dfca3 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, 29 Jun 2023 10:52:59 +0800
Subject: [PATCH v1] Trivial revise for the check of parameterized partial
paths
---
src/backend/optimizer/path/joinpath.c | 27 +++++++++------------------
1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index c2f211a60d..e4200049b8 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -813,6 +813,7 @@ try_partial_nestloop_path(PlannerInfo *root,
* rels are required here.
*/
Assert(bms_is_empty(joinrel->lateral_relids));
+ Assert(bms_is_empty(PATH_REQ_OUTER(outer_path)));
if (inner_path->param_info != NULL)
{
Relids inner_paramrels = inner_path->param_info->ppi_req_outer;
@@ -990,13 +991,9 @@ try_partial_mergejoin_path(PlannerInfo *root,
* See comments in try_partial_hashjoin_path().
*/
Assert(bms_is_empty(joinrel->lateral_relids));
- if (inner_path->param_info != NULL)
- {
- Relids inner_paramrels = inner_path->param_info->ppi_req_outer;
-
- if (!bms_is_empty(inner_paramrels))
- return;
- }
+ Assert(bms_is_empty(PATH_REQ_OUTER(outer_path)));
+ if (!bms_is_empty(PATH_REQ_OUTER(inner_path)))
+ return;
/*
* If the given paths are already well enough ordered, we can skip doing
@@ -1121,19 +1118,13 @@ try_partial_hashjoin_path(PlannerInfo *root,
JoinCostWorkspace workspace;
/*
- * If the inner path is parameterized, the parameterization must be fully
- * satisfied by the proposed outer path. Parameterized partial paths are
- * not supported. The caller should already have verified that no lateral
- * rels are required here.
+ * Parameterized partial paths are not supported. The caller should
+ * already have verified that no lateral rels are required here.
*/
Assert(bms_is_empty(joinrel->lateral_relids));
- if (inner_path->param_info != NULL)
- {
- Relids inner_paramrels = inner_path->param_info->ppi_req_outer;
-
- if (!bms_is_empty(inner_paramrels))
- return;
- }
+ Assert(bms_is_empty(PATH_REQ_OUTER(outer_path)));
+ if (!bms_is_empty(PATH_REQ_OUTER(inner_path)))
+ return;
/*
* Before creating a path, get a quick lower bound on what it is likely to
--
2.31.0
On Thu, 29 Jun 2023 at 08:53, Richard Guo <guofenglinux@gmail.com> wrote:
While working on the invalid parameterized join path issue [1], I
noticed that we can simplify the codes for checking parameterized
partial paths in try_partial_hashjoin/mergejoin_path, with the help of
macro PATH_REQ_OUTER.- if (inner_path->param_info != NULL) - { - Relids inner_paramrels = inner_path->param_info->ppi_req_outer; - - if (!bms_is_empty(inner_paramrels)) - return; - } + if (!bms_is_empty(PATH_REQ_OUTER(inner_path))) + return;Also there is a comment there that is not correct.
* If the inner path is parameterized, the parameterization must be fully
* satisfied by the proposed outer path.This is true for nestloop but not for hashjoin/mergejoin.
Besides, I wonder if it'd be better that we verify that the outer input
path for a partial join path should not have any parameterization
dependency.Attached is a patch for all these changes.
I'm seeing that there has been no activity in this thread for nearly 7
months, I'm planning to close this in the current commitfest unless
someone is planning to take it forward.
Regards,
Vignesh
On Sun, Jan 21, 2024 at 8:36 PM vignesh C <vignesh21@gmail.com> wrote:
I'm seeing that there has been no activity in this thread for nearly 7
months, I'm planning to close this in the current commitfest unless
someone is planning to take it forward.
This patch fixes the wrong comments in try_partial_hashjoin_path, and
also simplifies and enhances the checks for parameterized partial paths.
I think it's worth to be moved forward.
I've rebased the patch over current master, added a commit message
describing what it does, and updated the comment a little bit in the v2
patch.
Thanks
Richard
Attachments:
v2-0001-Revisions-to-the-checks-for-parameterized-partial-paths.patchapplication/octet-stream; name=v2-0001-Revisions-to-the-checks-for-parameterized-partial-paths.patchDownload
From e32ec05dfe3533eff0932fd2fcbf916cf6912b17 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, 29 Jun 2023 10:52:59 +0800
Subject: [PATCH v2] Revisions to the checks for parameterized partial paths
Parameterized partial paths are not supported, and we have several
checks in try_partial_xxx_path functions to enforce this. For a partial
nestloop join path, we need to ensure that if the inner path is
parameterized, the parameterization is fully satisfied by the proposed
outer path. For a partial merge/hashjoin join path, we need to ensure
that the inner path is not parameterized.
However, the comment in try_partial_hashjoin_path does not describe this
correctly. This commit fixes that.
In addtion, this commit simplifies the checks peformed in
try_partial_hashjoin_path and try_partial_mergejoin_path with the help
of macro PATH_REQ_OUTER.
In passing, this commit also asserts that the outer path is not
parameterized in try_partial_xxx_path functions.
---
src/backend/optimizer/path/joinpath.c | 28 ++++++++++-----------------
1 file changed, 10 insertions(+), 18 deletions(-)
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index c0ba087b40..f61e6b57b9 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -847,6 +847,7 @@ try_partial_nestloop_path(PlannerInfo *root,
* rels are required here.
*/
Assert(bms_is_empty(joinrel->lateral_relids));
+ Assert(bms_is_empty(PATH_REQ_OUTER(outer_path)));
if (inner_path->param_info != NULL)
{
Relids inner_paramrels = inner_path->param_info->ppi_req_outer;
@@ -1035,13 +1036,9 @@ try_partial_mergejoin_path(PlannerInfo *root,
* See comments in try_partial_hashjoin_path().
*/
Assert(bms_is_empty(joinrel->lateral_relids));
- if (inner_path->param_info != NULL)
- {
- Relids inner_paramrels = inner_path->param_info->ppi_req_outer;
-
- if (!bms_is_empty(inner_paramrels))
- return;
- }
+ Assert(bms_is_empty(PATH_REQ_OUTER(outer_path)));
+ if (!bms_is_empty(PATH_REQ_OUTER(inner_path)))
+ return;
/*
* If the given paths are already well enough ordered, we can skip doing
@@ -1177,19 +1174,14 @@ try_partial_hashjoin_path(PlannerInfo *root,
JoinCostWorkspace workspace;
/*
- * If the inner path is parameterized, the parameterization must be fully
- * satisfied by the proposed outer path. Parameterized partial paths are
- * not supported. The caller should already have verified that no lateral
- * rels are required here.
+ * If the inner path is parameterized, we can't use a partial hashjoin.
+ * Parameterized partial paths are not supported. The caller should
+ * already have verified that no lateral rels are required here.
*/
Assert(bms_is_empty(joinrel->lateral_relids));
- if (inner_path->param_info != NULL)
- {
- Relids inner_paramrels = inner_path->param_info->ppi_req_outer;
-
- if (!bms_is_empty(inner_paramrels))
- return;
- }
+ Assert(bms_is_empty(PATH_REQ_OUTER(outer_path)));
+ if (!bms_is_empty(PATH_REQ_OUTER(inner_path)))
+ return;
/*
* Before creating a path, get a quick lower bound on what it is likely to
--
2.31.0
On Thu, Jan 25, 2024 at 3:21 PM Richard Guo <guofenglinux@gmail.com> wrote:
On Sun, Jan 21, 2024 at 8:36 PM vignesh C <vignesh21@gmail.com> wrote:
I'm seeing that there has been no activity in this thread for nearly 7
months, I'm planning to close this in the current commitfest unless
someone is planning to take it forward.This patch fixes the wrong comments in try_partial_hashjoin_path, and
also simplifies and enhances the checks for parameterized partial paths.
I think it's worth to be moved forward.I've rebased the patch over current master, added a commit message
describing what it does, and updated the comment a little bit in the v2
patch.
I've pushed this patch.
Thanks
Richard