Re: Oversight in reparameterize_path_by_child leading to executor crash

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> On Tue, Aug 22, 2023 at 4:48 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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.

Ah.

> 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
> + /*
> + * 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);

Mmm ... at the very least you'd need to not do that when top_parent_relids
is unset. But I think the risk/reward ratio for messing with this in the
back branches is unattractive in any case: to fix a corner case that
apparently nobody uses in the field, we risk breaking any number of
mainstream parameterized-path cases. I'm content to commit the v5 patch
(or a successor) into HEAD, but at this point I'm not sure I even want
to risk it in v16, let alone perform delicate surgery to get it to work
in older branches. I think we ought to go with the "tablesample scans
can't be reparameterized" approach in v16 and before.

regards, tom lane