MIME-Version: 1.0
From: Richard Guo <guofenglinux@gmail.com>
Date: Tue, 14 Nov 2023 19:14:57 +0800
Message-ID: <CAMbWs4_wJthNtYBL+SsebpgF-5L2r5zFFk6xYbS0A78GKOTFHw@mail.gmail.com>
Subject: Assert failure on 'list_member_ptr(rel->joininfo, restrictinfo)'
To: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Alexander Korotkov <aekorotkov@gmail.com>
Content-Type: multipart/mixed; boundary="000000000000436bf0060a1ae67d"
Archived-At: <https://www.postgresql.org/message-id/CAMbWs4_wJthNtYBL%2BSsebpgF-5L2r5zFFk6xYbS0A78GKOTFHw%40mail.gmail.com>
Precedence: bulk

--000000000000436bf0060a1ae67d
Content-Type: multipart/alternative; boundary="000000000000436bee060a1ae67b"

--000000000000436bee060a1ae67b
Content-Type: text/plain; charset="UTF-8"

While working on BUG #18187 [1], I noticed that we also have issues with
how SJE replaces join clauses involving the removed rel.  As an example,
consider the query below, which would trigger an Assert.

create table t (a int primary key, b int);

explain (costs off)
select * from t t1
   inner join t t2 on t1.a = t2.a
    left join t t3 on t1.b > 1 and t1.b < 2;
server closed the connection unexpectedly

The Assert failure happens in remove_self_join_rel() when we're trying
to remove t1.  The two join clauses of t1, 't1.b > 1' and 't1.b < 2',
share the same pointer of 'required_relids', which is {t1, t3} at first.
After we've performed replace_varno for the first clause, the
required_relids becomes {t2, t3}, which is no problem.  However, the
second clause's required_relids also becomes {t2, t3}, because they are
actually the same pointer.  So when we proceed with replace_varno on the
second clause, we'd trigger the Assert.

Off the top of my head I'm thinking that we can fix this kind of issue
by bms_copying the bitmapset first before we make a substitution in
replace_relid(), like attached.

Alternatively, we can revise distribute_qual_to_rels() as below so that
different RestrictInfos don't share the same pointer of required_relids.

--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -2385,7 +2385,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node
*clause,
         * nonnullable-side rows failing the qual.
         */
        Assert(ojscope);
-       relids = ojscope;
+       relids = bms_copy(ojscope);
        Assert(!pseudoconstant);
    }
    else

With this way, I'm worrying that there are other places where we should
avoid sharing the same pointer to Bitmapset structure.  I'm not sure how
to discover all these places.

Any thoughts?

[1]
https://www.postgresql.org/message-id/flat/18187-831da249cbd2ff8e%40postgresql.org

Thanks
Richard

--000000000000436bee060a1ae67b
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">While working on BUG #18187 [1], I noticed that we also ha=
ve issues with<br>how SJE replaces join clauses involving the removed rel.=
=C2=A0 As an example,<br>consider the query below, which would trigger an A=
ssert.<br><br>create table t (a int primary key, b int);<br><br>explain (co=
sts off)<br>select * from t t1<br>=C2=A0 =C2=A0inner join t t2 on t1.a =3D =
t2.a<br>=C2=A0 =C2=A0 left join t t3 on t1.b &gt; 1 and t1.b &lt; 2;<br>ser=
ver closed the connection unexpectedly<br><br>The Assert failure happens in=
 remove_self_join_rel() when we&#39;re trying<br>to remove t1.=C2=A0 The tw=
o join clauses of t1, &#39;t1.b &gt; 1&#39; and &#39;t1.b &lt; 2&#39;,<br>s=
hare the same pointer of &#39;required_relids&#39;, which is {t1, t3} at fi=
rst.<br>After we&#39;ve performed replace_varno for the first clause, the<b=
r>required_relids becomes {t2, t3}, which is no problem.=C2=A0 However, the=
<br>second clause&#39;s required_relids also becomes {t2, t3}, because they=
 are<br>actually the same pointer.=C2=A0 So when we proceed with replace_va=
rno on the<br>second clause, we&#39;d trigger the Assert.<br><br>Off the to=
p of my head I&#39;m thinking that we can fix this kind of issue<br>by bms_=
copying the bitmapset first before we make a substitution in<br>replace_rel=
id(), like attached.<br><br>Alternatively, we can revise distribute_qual_to=
_rels() as below so that<br>different RestrictInfos don&#39;t share the sam=
e pointer of required_relids.<br><br>--- a/src/backend/optimizer/plan/inits=
plan.c<br>+++ b/src/backend/optimizer/plan/initsplan.c<br>@@ -2385,7 +2385,=
7 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,<br>=C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0* nonnullable-side rows failing the qual.<br>=C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 Assert(ojscop=
e);<br>- =C2=A0 =C2=A0 =C2=A0 relids =3D ojscope;<br>+ =C2=A0 =C2=A0 =C2=A0=
 relids =3D bms_copy(ojscope);<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 Assert(!pseud=
oconstant);<br>=C2=A0 =C2=A0 }<br>=C2=A0 =C2=A0 else<br><br>With this way, =
I&#39;m worrying that there are other places where we should<br>avoid shari=
ng the same pointer to Bitmapset structure.=C2=A0 I&#39;m not sure how<br>t=
o discover all these places.<br><br>Any thoughts?<br><br>[1] <a href=3D"htt=
ps://www.postgresql.org/message-id/flat/18187-831da249cbd2ff8e%40postgresql=
.org">https://www.postgresql.org/message-id/flat/18187-831da249cbd2ff8e%40p=
ostgresql.org</a><br><br>Thanks<br>Richard<br></div>

--000000000000436bee060a1ae67b--
--000000000000436bf0060a1ae67d
Content-Type: application/octet-stream; 
	name="v1-0001-Fix-how-SJE-replaces-join-clauses.patch"
Content-Disposition: attachment; 
	filename="v1-0001-Fix-how-SJE-replaces-join-clauses.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_loy8jseq0>
X-Attachment-Id: f_loy8jseq0

RnJvbSA4NjY5ZWRiOTkxZjRjYjlkNDhmZThmODViZWM4NjYzNmVlZjI0NjUyIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBSaWNoYXJkIEd1byA8Z3VvZmVuZ2xpbnV4QGdtYWlsLmNvbT4K
RGF0ZTogVHVlLCAxNCBOb3YgMjAyMyAxODo1NToyNCArMDgwMApTdWJqZWN0OiBbUEFUQ0ggdjFd
IEZpeCBob3cgU0pFIHJlcGxhY2VzIGpvaW4gY2xhdXNlcwoKLS0tCiBzcmMvYmFja2VuZC9vcHRp
bWl6ZXIvcGxhbi9hbmFseXplam9pbnMuYyB8ICA5ICsrKysrKystLQogc3JjL3Rlc3QvcmVncmVz
cy9leHBlY3RlZC9qb2luLm91dCAgICAgICAgfCAxNSArKysrKysrKysrKysrKysKIHNyYy90ZXN0
L3JlZ3Jlc3Mvc3FsL2pvaW4uc3FsICAgICAgICAgICAgIHwgIDYgKysrKysrCiAzIGZpbGVzIGNo
YW5nZWQsIDI4IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvc3Jj
L2JhY2tlbmQvb3B0aW1pemVyL3BsYW4vYW5hbHl6ZWpvaW5zLmMgYi9zcmMvYmFja2VuZC9vcHRp
bWl6ZXIvcGxhbi9hbmFseXplam9pbnMuYwppbmRleCBiOWJlMTlhNjg3Li5mOTQ0NDlkZjk0IDEw
MDY0NAotLS0gYS9zcmMvYmFja2VuZC9vcHRpbWl6ZXIvcGxhbi9hbmFseXplam9pbnMuYworKysg
Yi9zcmMvYmFja2VuZC9vcHRpbWl6ZXIvcGxhbi9hbmFseXplam9pbnMuYwpAQCAtMTUxOCwxMiAr
MTUxOCwxNyBAQCByZXBsYWNlX3JlbGlkKFJlbGlkcyByZWxpZHMsIGludCBvbGRJZCwgaW50IG5l
d0lkKQogCWlmIChvbGRJZCA8IDApCiAJCXJldHVybiByZWxpZHM7CiAKKwkvKiBEZWxldGUgcmVs
aWQgd2l0aG91dCBzdWJzdGl0dXRpb24uICovCiAJaWYgKG5ld0lkIDwgMCkKLQkJLyogRGVsZXRl
IHJlbGlkIHdpdGhvdXQgc3Vic3RpdHV0aW9uLiAqLwogCQlyZXR1cm4gYm1zX2RlbF9tZW1iZXIo
cmVsaWRzLCBvbGRJZCk7CiAKKwkvKgorCSAqIFN1YnN0aXR1dGUgbmV3SWQgZm9yIG9sZElkLiAg
V2UgbXVzdCBtYWtlIGEgY29weSBvZiB0aGUgb3JpZ2luYWwgcmVsaWRzCisJICogYmVmb3JlIHN0
YXJ0aW5nIHRoZSBzdWJzdGl0dXRpb24sIGJlY2F1c2UgdGhlIHNhbWUgcG9pbnRlciB0byBhCisJ
ICogQml0bWFwc2V0IHN0cnVjdHVyZSBtaWdodCBiZSBzaGFyZWQgYW1vbmcgZGlmZmVyZW50IHBs
YWNlcy4KKwkgKi8KIAlpZiAoYm1zX2lzX21lbWJlcihvbGRJZCwgcmVsaWRzKSkKLQkJcmV0dXJu
IGJtc19hZGRfbWVtYmVyKGJtc19kZWxfbWVtYmVyKHJlbGlkcywgb2xkSWQpLCBuZXdJZCk7CisJ
CXJldHVybiBibXNfYWRkX21lbWJlcihibXNfZGVsX21lbWJlcihibXNfY29weShyZWxpZHMpLCBv
bGRJZCksIG5ld0lkKTsKIAogCXJldHVybiByZWxpZHM7CiB9CmRpZmYgLS1naXQgYS9zcmMvdGVz
dC9yZWdyZXNzL2V4cGVjdGVkL2pvaW4ub3V0IGIvc3JjL3Rlc3QvcmVncmVzcy9leHBlY3RlZC9q
b2luLm91dAppbmRleCAyYzczMjcwMTQzLi41Y2VlY2M0NDEzIDEwMDY0NAotLS0gYS9zcmMvdGVz
dC9yZWdyZXNzL2V4cGVjdGVkL2pvaW4ub3V0CisrKyBiL3NyYy90ZXN0L3JlZ3Jlc3MvZXhwZWN0
ZWQvam9pbi5vdXQKQEAgLTY4MzYsNiArNjgzNiwyMSBAQCBzZWxlY3QgKiBmcm9tIGVtcDEgdDEg
am9pbiBlbXAxIHQyIG9uIHQxLmlkID0gdDIuaWQgbGVmdCBqb2luCiAgICAtPiAgRnVuY3Rpb24g
U2NhbiBvbiBnZW5lcmF0ZV9zZXJpZXMgdDMKICg2IHJvd3MpCiAKKy0tIENoZWNrIHRoYXQgU0pF
IHJlcGxhY2VzIGpvaW4gY2xhdXNlcyBpbnZvbHZpbmcgdGhlIHJlbW92ZWQgcmVsIGNvcnJlY3Rs
eQorZXhwbGFpbiAoY29zdHMgb2ZmKQorc2VsZWN0ICogZnJvbSBlbXAxIHQxCisgICBpbm5lciBq
b2luIGVtcDEgdDIgb24gdDEuaWQgPSB0Mi5pZAorICAgIGxlZnQgam9pbiBlbXAxIHQzIG9uIHQx
LmlkID4gMSBhbmQgdDEuaWQgPCAyOworICAgICAgICAgICAgICAgICAgUVVFUlkgUExBTiAgICAg
ICAgICAgICAgICAgIAorLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLQorIE5lc3RlZCBMb29wIExlZnQgSm9pbgorICAgSm9pbiBGaWx0ZXI6ICgodDIuaWQgPiAx
KSBBTkQgKHQyLmlkIDwgMikpCisgICAtPiAgU2VxIFNjYW4gb24gZW1wMSB0MgorICAgICAgICAg
RmlsdGVyOiAoaWQgSVMgTk9UIE5VTEwpCisgICAtPiAgTWF0ZXJpYWxpemUKKyAgICAgICAgIC0+
ICBTZXEgU2NhbiBvbiBlbXAxIHQzCisoNiByb3dzKQorCiAtLSBXZSBjYW4gcmVtb3ZlIHRoZSBq
b2luIGV2ZW4gaWYgd2UgZmluZCB0aGUgam9pbiBjYW4ndCBkdXBsaWNhdGUgcm93cyBhbmQKIC0t
IHRoZSBiYXNlIHF1YWxzIG9mIGVhY2ggc2lkZSBhcmUgZGlmZmVyZW50LiAgSW4gdGhlIGZvbGxv
d2luZyBjYXNlIHdlIGVuZCB1cAogLS0gbW92aW5nIHF1YWxzIG92ZXIgdG8gczEgdG8gbWFrZSBp
dCBzbyBpdCBjYW4ndCBtYXRjaCBhbnkgcm93cy4KZGlmZiAtLWdpdCBhL3NyYy90ZXN0L3JlZ3Jl
c3Mvc3FsL2pvaW4uc3FsIGIvc3JjL3Rlc3QvcmVncmVzcy9zcWwvam9pbi5zcWwKaW5kZXggOGE4
YTYzYmQyZi4uYzNlYzM0MGUyZCAxMDA2NDQKLS0tIGEvc3JjL3Rlc3QvcmVncmVzcy9zcWwvam9p
bi5zcWwKKysrIGIvc3JjL3Rlc3QvcmVncmVzcy9zcWwvam9pbi5zcWwKQEAgLTI2MDYsNiArMjYw
NiwxMiBAQCBleHBsYWluIChjb3N0cyBvZmYpCiBzZWxlY3QgKiBmcm9tIGVtcDEgdDEgam9pbiBl
bXAxIHQyIG9uIHQxLmlkID0gdDIuaWQgbGVmdCBqb2luCiAgICAgbGF0ZXJhbCAoc2VsZWN0IHQx
LmlkIGFzIHQxaWQsICogZnJvbSBnZW5lcmF0ZV9zZXJpZXMoMSwxKSB0MykgcyBvbiB0cnVlOwog
CistLSBDaGVjayB0aGF0IFNKRSByZXBsYWNlcyBqb2luIGNsYXVzZXMgaW52b2x2aW5nIHRoZSBy
ZW1vdmVkIHJlbCBjb3JyZWN0bHkKK2V4cGxhaW4gKGNvc3RzIG9mZikKK3NlbGVjdCAqIGZyb20g
ZW1wMSB0MQorICAgaW5uZXIgam9pbiBlbXAxIHQyIG9uIHQxLmlkID0gdDIuaWQKKyAgICBsZWZ0
IGpvaW4gZW1wMSB0MyBvbiB0MS5pZCA+IDEgYW5kIHQxLmlkIDwgMjsKKwogLS0gV2UgY2FuIHJl
bW92ZSB0aGUgam9pbiBldmVuIGlmIHdlIGZpbmQgdGhlIGpvaW4gY2FuJ3QgZHVwbGljYXRlIHJv
d3MgYW5kCiAtLSB0aGUgYmFzZSBxdWFscyBvZiBlYWNoIHNpZGUgYXJlIGRpZmZlcmVudC4gIElu
IHRoZSBmb2xsb3dpbmcgY2FzZSB3ZSBlbmQgdXAKIC0tIG1vdmluZyBxdWFscyBvdmVyIHRvIHMx
IHRvIG1ha2UgaXQgc28gaXQgY2FuJ3QgbWF0Y2ggYW55IHJvd3MuCi0tIAoyLjMxLjAKCg==
--000000000000436bf0060a1ae67d--