Re: Oversight in reparameterize_path_by_child leading to executor crash - Mailing list pgsql-hackers
| From | Richard Guo |
|---|---|
| Subject | Re: Oversight in reparameterize_path_by_child leading to executor crash |
| Date | |
| Msg-id | CAMbWs4-xNYnDDM6J-6Ty0+3LkbQaxibnv9YZt3ewLrGW5hfuog@mail.gmail.com Whole thread Raw |
| In response to | Re: Oversight in reparameterize_path_by_child leading to executor crash (Tom Lane <tgl@sss.pgh.pa.us>) |
| Responses |
Re: Oversight in reparameterize_path_by_child leading to executor crash
|
| List | pgsql-hackers |
On Tue, Aug 22, 2023 at 4:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I spent some time reviewing the v4 patch. I noted that
path_is_reparameterizable_by_child still wasn't modeling the pass/fail
behavior of reparameterize_path_by_child very well, because that
function checks this at every recursion level:
/*
* If the path is not parameterized by the parent of the given relation,
* it doesn't need reparameterization.
*/
if (!path->param_info ||
!bms_overlap(PATH_REQ_OUTER(path), child_rel->top_parent_relids))
return path;
So we might have a sub-path (eg a join input rel) that is not one of
the supported kinds, and yet we can succeed because parameterization
appears only in other sub-paths. The patch as written will not crash
in such a case, but it might refuse to use a path that we previously
would have allowed. So I think we need to put the same test into
path_is_reparameterizable_by_child, which requires adding child_rel
to its param list but is otherwise simple enough.
sigh.. I overlooked this check. You're right. We have to have this
same test in path_is_reparameterizable_by_child.
same test in path_is_reparameterizable_by_child.
So that led me to the attached v5, which seemed committable to me so I
set about back-patching it ... and it fell over immediately in v15, as
shown in the attached regression diffs from v15. It looks to me like
we are now failing to recognize that reparameterized quals are
redundant with not-reparameterized ones, so this patch is somehow
dependent on restructuring that happened during the v16 cycle.
I don't have time to dig deeper than that, and I'm not sure that that
is an area we'd want to mess with in a back-patched bug fix.
I looked at this and I think the culprit is that in create_nestloop_path
we are failing to recognize those restrict_clauses that have been moved
into the inner path. In v16, we have the new serial number stuff to
help detect such clauses and that works very well. In v15, we are still
using join_clause_is_movable_into() for that purpose. It does not work
well with the patch because now in create_nestloop_path the inner path
is still parameterized by top-level parents, while the restrict_clauses
already have been adjusted to refer to the child rels. So the check
performed by join_clause_is_movable_into() would always fail.
I'm wondering if we can instead adjust the 'inner_req_outer' in
create_nestloop_path before we perform the check to work around this
issue for the back branches, something like
@@ -2418,6 +2419,15 @@ create_nestloop_path(PlannerInfo *root,
NestPath *pathnode = makeNode(NestPath);
Relids inner_req_outer = PATH_REQ_OUTER(inner_path);
+ /*
+ * Adjust the parameterization information, which refers to the topmost
+ * parent.
+ */
+ inner_req_outer =
+ adjust_child_relids_multilevel(root, inner_req_outer,
+ outer_path->parent->relids,
+ outer_path->parent->top_parent_relids);
+
And with it we do not need the changes as in the patch for
create_nestloop_path any more.
Thanks
Richard
we are failing to recognize those restrict_clauses that have been moved
into the inner path. In v16, we have the new serial number stuff to
help detect such clauses and that works very well. In v15, we are still
using join_clause_is_movable_into() for that purpose. It does not work
well with the patch because now in create_nestloop_path the inner path
is still parameterized by top-level parents, while the restrict_clauses
already have been adjusted to refer to the child rels. So the check
performed by join_clause_is_movable_into() would always fail.
I'm wondering if we can instead adjust the 'inner_req_outer' in
create_nestloop_path before we perform the check to work around this
issue for the back branches, something like
@@ -2418,6 +2419,15 @@ create_nestloop_path(PlannerInfo *root,
NestPath *pathnode = makeNode(NestPath);
Relids inner_req_outer = PATH_REQ_OUTER(inner_path);
+ /*
+ * Adjust the parameterization information, which refers to the topmost
+ * parent.
+ */
+ inner_req_outer =
+ adjust_child_relids_multilevel(root, inner_req_outer,
+ outer_path->parent->relids,
+ outer_path->parent->top_parent_relids);
+
And with it we do not need the changes as in the patch for
create_nestloop_path any more.
Thanks
Richard
pgsql-hackers by date: