Re: Oversight in reparameterize_path_by_child leading to executor crash

On Tue, Aug 22, 2023 at 4:48 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.

> 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