Richard Guo <[email protected]> 于2026年6月17日周三 13:51写道：
>
> On Wed, Jun 17, 2026 at 11:00 AM Tender Wang <[email protected]> wrote:
> > In remove_rel_from_eclass(), we have
> > ...
> > if (!IsA(em->em_expr, Var))
> > em->em_expr = (Expr *)
> > remove_rel_from_phvs((Node *) em->em_expr, relid,
> > ojrelid);
> > ...
> > "where s.id = 1" in the test case, the Const node seems to be able to skip,
> > too.
>
> Good point. Patch updated.
In remove_rel_from_phvs_mutator(), we have:
else if (IsA(node, Query))
{
Query *newnode;
...
}
I want to find what kind of SQL can enter the above else-if block.
So I added "assert(0)" to the else-if block. But no regression test crashed.
We test rel->baserestrictinfo here. Is it possible that the clause
includes a Query structure?
I know some xxx_mutator() functions in the planner have the same logic
processing pattern.
Is there a need to add a test case to cover the above code?
> Yeah. remove_rel_from_query() was originally written for left-join
> removal only. The self-join elimination (SJE) commit grafted
> self-join removal onto it, which made it do more than one thing within
> one function. Commit 20efbdffe cleaned it up, clarifying the
> separation between the left-join removal and self-join elimination
> code paths within it. That cleanup was also meant to make it better
> structured for adding new types of join removal, such as inner-join
> removal, in the future.
>
> That said, I agree that the current shape is not ideal. A better
> structure might be to make remove_rel_from_query() handle only the
> bookkeeping that is common to all removal types, and push the
> type-specific work into the per-type callers. I think that is worth
> pursuing as separate future work, kept apart from the current bug fix.
Agree.
--
Thanks,
Tender Wang
