MIME-Version: 1.0
References: 
 <CAMbWs49fYQcqJfJ_Gtn8r1GFNoYtb1=2AUab4ieuqY4Zid9ocQ@mail.gmail.com>
 <CAApHDvr7c0XP6Hc2P0nqV8iR0mDWUniXXzjiTsgMvpnzePXevg@mail.gmail.com>
In-Reply-To: 
 <CAApHDvr7c0XP6Hc2P0nqV8iR0mDWUniXXzjiTsgMvpnzePXevg@mail.gmail.com>
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, 23 Apr 2026 16:58:46 +0900
Message-ID: 
 <CAMbWs48=du2DSGPR_b6AkJ=ecoQ+GcXqzN6XSvn32CHn0x6itA@mail.gmail.com>
Subject: Re: The bogus calls in remove_self_join_rel()
To: David Rowley <dgrowleyml@gmail.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="0000000000004eb47e06501c05b7"
Archived-At: 
 <https://www.postgresql.org/message-id/CAMbWs48%3Ddu2DSGPR_b6AkJ%3DecoQ%2BGcXqzN6XSvn32CHn0x6itA%40mail.gmail.com>
Precedence: bulk

--0000000000004eb47e06501c05b7
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 23, 2026 at 12:11=E2=80=AFPM 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 =3D=3D 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 =3D=3D RTE_RELATION check ensures SJE never considers it as a
candidate.  The CTE itself is planned as a separate Query, where the
varno !=3D 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

--0000000000004eb47e06501c05b7
Content-Type: application/octet-stream;
	name="v2-0001-Fix-bogus-calls-in-remove_self_join_rel.patch"
Content-Disposition: attachment;
	filename="v2-0001-Fix-bogus-calls-in-remove_self_join_rel.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_mob6wgfw0>
X-Attachment-Id: f_mob6wgfw0

RnJvbSA1YWNiNTA2NmQ3M2I3MzhiMDg2MTU5M2JiYmNmODUwMTRmMzVmOGZhIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBSaWNoYXJkIEd1byA8Z3VvZmVuZ2xpbnV4QGdtYWlsLmNvbT4K
RGF0ZTogVGh1LCAyMyBBcHIgMjAyNiAxMToxOToyNSArMDkwMApTdWJqZWN0OiBbUEFUQ0ggdjJd
IEZpeCBib2d1cyBjYWxscyBpbiByZW1vdmVfc2VsZl9qb2luX3JlbCgpCgpyZW1vdmVfc2VsZl9q
b2luX3JlbCgpIGNhbGxlZCBhZGp1c3RfcmVsaWRfc2V0KCkgb24gYWxsX3Jlc3VsdF9yZWxpZHMK
YW5kIGxlYWZfcmVzdWx0X3JlbGlkcyBidXQgdGhyZXcgYXdheSB0aGUgcmV0dXJuIHZhbHVlLiAg
U2luY2UKYWRqdXN0X3JlbGlkX3NldCgpIHJldHVybnMgYSBmcmVzaGx5LWJ1aWx0IFJlbGlkcyBh
bmQgZG9lcyBub3QgbW9kaWZ5CnRoZSBpbnB1dCBpbiBwbGFjZSwgdGhlIGNhbGxzIGRpZCBub3Ro
aW5nLiAgVGhpcyBoYXMgYmVlbiB0aGUgY2FzZQpzaW5jZSB0aGUgU0pFIGZlYXR1cmUgd2VudCBp
biAoY29tbWl0IGZjMDY5YTNhNikuCgpUaGVyZSBoYXMgYmVlbiBubyBvYnNlcnZhYmxlIG1pc2Jl
aGF2aW9yLCBiZWNhdXNlIHRoZSByZWxpZCBiZWluZwpwYXNzZWQgaXMgZ3VhcmFudGVlZCBub3Qg
dG8gYmUgYSBtZW1iZXIgb2YgZWl0aGVyIHNldC4gIEF0IHRoZSBwb2ludApyZW1vdmVfc2VsZl9q
b2luX3JlbCgpIHJ1bnMsIHRob3NlIHNldHMgY29udGFpbiBvbmx5IHJlc3VsdFJlbGF0aW9uOwpp
bmhlcml0YW5jZSBjaGlsZHJlbiBoYXZlIG5vdCBiZWVuIGFkZGVkIHlldCwgYXMgdGhhdCBoYXBw
ZW5zIGxhdGVyIGluCnF1ZXJ5X3BsYW5uZXIoKSwgaW4gZXhwYW5kX3NpbmdsZV9pbmhlcml0YW5j
ZV9jaGlsZCgpIGNhbGxlZCBmcm9tCmFkZF9vdGhlcl9yZWxzX3RvX3F1ZXJ5KCkuICBBbmQgcmVt
b3ZlX3NlbGZfam9pbnNfcmVjdXJzZSgpIHJlamVjdHMKcGFyc2UtPnJlc3VsdFJlbGF0aW9uIGFz
IGFuIFNKRSBjYW5kaWRhdGUgdG8gcHJlc2VydmUgdGhlIEV2YWxQbGFuUXVhbAptZWNoYW5pc20u
ICBFdmVuIHdpdGggdGhlIHJlc3VsdCBhc3NpZ25lZCwgdGhlIGNhbGxzIHdvdWxkIGJlIG5vLW9w
cwppbiBwcmFjdGljZS4KClJhdGhlciB0aGFuIG1ha2UgdGhlIGNhbGxzIGRvIHRoZSBjbGVhbnVw
IHRoZXkgcHJldGVuZCB0byBkbywgcmVwbGFjZQp0aGVtIHdpdGggYXNzZXJ0aW9ucyBvZiB0aGUg
aW52YXJpYW50LiAgQW55IGZ1dHVyZSBsb29zZW5pbmcgb2YgdGhlClNKRSBjYW5kaWRhdGUgZmls
dGVyIC0tIGZvciBpbnN0YW5jZSB0byBhbGxvdyBlbGltaW5hdGluZyBhIHJlc3VsdApyZWxhdGlv
biB1bmRlciBwcm92YWJsZSBjb25kaXRpb25zIC0tIHdpbGwgdHJpcCB0aGUgYXNzZXJ0aW9uIGFu
ZApmb3JjZSB3aG9ldmVyIGRvZXMgaXQgdG8gcmV2aXNpdCB0aGlzIGNvZGUuCgpBZGRpdGlvbmFs
bHksIGRlY29yYXRlIGFkanVzdF9yZWxpZF9zZXQoKSB3aXRoIHBnX25vZGlzY2FyZCBzbyB0aGF0
CmFueSBmdXR1cmUgYWNjaWRlbnRhbCBkaXNjYXJkIG9mIGl0cyByZXR1cm4gdmFsdWUgaXMgY2F1
Z2h0IGF0IGNvbXBpbGUKdGltZS4KLS0tCiBzcmMvYmFja2VuZC9vcHRpbWl6ZXIvcGxhbi9hbmFs
eXplam9pbnMuYyB8IDEyICsrKysrKysrKystLQogc3JjL2luY2x1ZGUvcmV3cml0ZS9yZXdyaXRl
TWFuaXAuaCAgICAgICAgfCAgMiArLQogMiBmaWxlcyBjaGFuZ2VkLCAxMSBpbnNlcnRpb25zKCsp
LCAzIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL29wdGltaXplci9wbGFu
L2FuYWx5emVqb2lucy5jIGIvc3JjL2JhY2tlbmQvb3B0aW1pemVyL3BsYW4vYW5hbHl6ZWpvaW5z
LmMKaW5kZXggMDMwNTZiZGYzZTAuLjBmODJhYjlmYWNiIDEwMDY0NAotLS0gYS9zcmMvYmFja2Vu
ZC9vcHRpbWl6ZXIvcGxhbi9hbmFseXplam9pbnMuYworKysgYi9zcmMvYmFja2VuZC9vcHRpbWl6
ZXIvcGxhbi9hbmFseXplam9pbnMuYwpAQCAtMTk5NCw4ICsxOTk0LDE2IEBAIHJlbW92ZV9zZWxm
X2pvaW5fcmVsKFBsYW5uZXJJbmZvICpyb290LCBQbGFuUm93TWFyayAqa21hcmssIFBsYW5Sb3dN
YXJrICpybWFyaywKIAlDaGFuZ2VWYXJOb2Rlc0V4dGVuZGVkKChOb2RlICopIHJvb3QtPnByb2Nl
c3NlZF90bGlzdCwgdG9SZW1vdmUtPnJlbGlkLAogCQkJCQkJICAgdG9LZWVwLT5yZWxpZCwgMCwg
cmVwbGFjZV9yZWxpZF9jYWxsYmFjayk7CiAKLQlhZGp1c3RfcmVsaWRfc2V0KHJvb3QtPmFsbF9y
ZXN1bHRfcmVsaWRzLCB0b1JlbW92ZS0+cmVsaWQsIHRvS2VlcC0+cmVsaWQpOwotCWFkanVzdF9y
ZWxpZF9zZXQocm9vdC0+bGVhZl9yZXN1bHRfcmVsaWRzLCB0b1JlbW92ZS0+cmVsaWQsIHRvS2Vl
cC0+cmVsaWQpOworCS8qCisJICogTm8gbmVlZCB0byB0b3VjaCBhbGxfcmVzdWx0X3JlbGlkcyBv
ciBsZWFmX3Jlc3VsdF9yZWxpZHM6IGF0IHRoaXMgcG9pbnQKKwkgKiB0aG9zZSBzZXRzIGNvbnRh
aW4gb25seSBwYXJzZS0+cmVzdWx0UmVsYXRpb247IGluaGVyaXRhbmNlIGNoaWxkcmVuCisJICog
aGF2ZSBub3QgYmVlbiBhZGRlZCB5ZXQ7IHRoYXQgaGFwcGVucyBsYXRlciBpbiBhZGRfb3RoZXJf
cmVsc190b19xdWVyeS4KKwkgKiBBbmQgcmVtb3ZlX3NlbGZfam9pbnNfcmVjdXJzZSByZWplY3Rz
IHBhcnNlLT5yZXN1bHRSZWxhdGlvbiBhcyBhbiBTSkUKKwkgKiBjYW5kaWRhdGUgdG8gcHJlc2Vy
dmUgdGhlIEVQUSBtZWNoYW5pc20uICBTbyB0b1JlbW92ZS0+cmVsaWQgY2Fubm90IGJlCisJICog
YSBtZW1iZXIuCisJICovCisJQXNzZXJ0KCFibXNfaXNfbWVtYmVyKHRvUmVtb3ZlLT5yZWxpZCwg
cm9vdC0+YWxsX3Jlc3VsdF9yZWxpZHMpKTsKKwlBc3NlcnQoIWJtc19pc19tZW1iZXIodG9SZW1v
dmUtPnJlbGlkLCByb290LT5sZWFmX3Jlc3VsdF9yZWxpZHMpKTsKIAogCS8qCiAJICogVGhlcmUg
bWF5IGJlIHJlZmVyZW5jZXMgdG8gdGhlIHJlbCBpbiByb290LT5ma2V5X2xpc3QsIGJ1dCBpZiBz
bywKZGlmZiAtLWdpdCBhL3NyYy9pbmNsdWRlL3Jld3JpdGUvcmV3cml0ZU1hbmlwLmggYi9zcmMv
aW5jbHVkZS9yZXdyaXRlL3Jld3JpdGVNYW5pcC5oCmluZGV4IGE2ZDRlODg4ZTA2Li45ZDIzNGNi
ODc0MSAxMDA2NDQKLS0tIGEvc3JjL2luY2x1ZGUvcmV3cml0ZS9yZXdyaXRlTWFuaXAuaAorKysg
Yi9zcmMvaW5jbHVkZS9yZXdyaXRlL3Jld3JpdGVNYW5pcC5oCkBAIC01NCw3ICs1NCw3IEBAIHN0
cnVjdCBDaGFuZ2VWYXJOb2Rlc19jb250ZXh0CiAJQ2hhbmdlVmFyTm9kZXNfY2FsbGJhY2sgY2Fs
bGJhY2s7CiB9OwogCi1leHRlcm4gUmVsaWRzIGFkanVzdF9yZWxpZF9zZXQoUmVsaWRzIHJlbGlk
cywgaW50IG9sZHJlbGlkLCBpbnQgbmV3cmVsaWQpOworcGdfbm9kaXNjYXJkIGV4dGVybiBSZWxp
ZHMgYWRqdXN0X3JlbGlkX3NldChSZWxpZHMgcmVsaWRzLCBpbnQgb2xkcmVsaWQsIGludCBuZXdy
ZWxpZCk7CiBleHRlcm4gdm9pZCBDb21iaW5lUmFuZ2VUYWJsZXMoTGlzdCAqKmRzdF9ydGFibGUs
IExpc3QgKipkc3RfcGVybWluZm9zLAogCQkJCQkJCSAgIExpc3QgKnNyY19ydGFibGUsIExpc3Qg
KnNyY19wZXJtaW5mb3MpOwogZXh0ZXJuIHZvaWQgT2Zmc2V0VmFyTm9kZXMoTm9kZSAqbm9kZSwg
aW50IG9mZnNldCwgaW50IHN1YmxldmVsc191cCk7Ci0tIAoyLjM5LjUgKEFwcGxlIEdpdC0xNTQp
Cgo=
--0000000000004eb47e06501c05b7--