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.

> I have another question, although it's unrelated to this bug.
> The logic in remove_rel_from_query() makes me a bit uneasy. It
> currently has to handle both self-join and outer-join cases,
> and it seems possible that inner-join-specific handling could end up
> being added there in the future as well.
> The function appears to be taking on multiple distinct
> responsibilities, which makes the overall design feel somewhat
> awkward.

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.

- Richard

Attachment: v2-0001-Strip-removed-relation-references-from-PlaceHolde.patch
Description: Binary data

Reply via email to