On Thu, Apr 23, 2026 at 12:11 PM David Rowley <dgrowleyml@gmail.com> wrote:
> Have you followed through on what happens
> for CTEs that do DML and RETURNING? I assume that fails on the nearby
> rte->relkind == RELKIND_RELATION, but I didn't debug to check.
Right. The CTE's reference in the outer query is RTE_CTE, and the
rte->rtekind == RTE_RELATION check ensures SJE never considers it as a
candidate. The CTE itself is planned as a separate Query, where the
varno != root->parse->resultRelation check rules out its target
relation.
> The
> only place I see all_result_relids being added to, aside from the
> initial setting with bms_make_singleton() is for the inheritance
> expansion in expand_single_inheritance_child(), which happens after
> join removals. For leaf_result_relids, it's similar.
Right. And this makes the comment and commit message not accurate, as
inheritance children have not been added yet, as that happens later in
add_other_rels_to_query(). Fixed in v2.
> I think the Asserts should go at the top of the function next to the
> other Asserts. Putting them near the top makes it clearer when reading
> code. The Asserts will be very close to the function's header comment,
> so it's easier to get a picture about what the function supports and
> does, plus, it helps ensure we still get the Asserts before any early
> returns are taken.
Hmm, I considered that, but I chose the current placement because the
Asserts are documenting a specific non-action: "we don't touch these
two sets, and here is why." That reads more naturally adjacent to the
cleanup of all the other structures, rather than at the top where it
would turn into a precondition claim. The existing Asserts at the top
check input-parameter validity, which is a different kind of check.
On the early-returns argument: remove_self_join_rel() has no early
returns today, and adding one would mean forgetting to clear some
field, so I don't expect that to change.
That said, either location is OK, so happy to move them if you feel
strongly.
> It may also be useful to decorate adjust_relid_set() with pg_nodiscard.
Good suggestion. Done in v2.
- Richard
