Subject: Assert failure on 'list_member_ptr(rel->joininfo, restrictinfo)'

From: Richard Guo <[email-masked]>
Date: Tue, 14 Nov 2023 19:14:57 +0800

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

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 14 Nov 2023 14:42:13 +0200

Hi!

Thank you for spotting this and dealing with this.

> replace_relid(), like attached.

I remember, I've removed bms_copy() from here.  Now I understand why
that was needed.  But I'm still not particularly happy about it.  The
reason is that logic of replace_relid() becomes cumbersome.  In some
cases it performs modification in-place, while in other cases it
copies.

> @@ -2385,7 +2385,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,

This looks better to me.  However, I'm not sure what the overhead
would be?  How much would it increase the memory footprint?

It's possibly dumb option, but what about just removing the assert?

------
Regards,
Alexander Korotkov

---

From: Andres Freund <[email-masked]>
Date: Tue, 14 Nov 2023 22:04:21 -0800

Hi,

> It's possibly dumb option, but what about just removing the assert?

That's not at all an option - the in-place bms_* functions can free their
input. So a dangling pointer to the "old" version is a use-after-free waiting
to happen - you just need a query that actually gets to bitmapsets that are a
bit larger.

Greetings,

Andres

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 15 Nov 2023 17:06:32 +0200

> to happen - you just need a query that actually gets to bitmapsets that are a

Yeah, now I got it, thank you.  I was under the wrong impression that
bitmapset has the level of indirection, so the pointer remains valid.
Now, I see that bitmapset manipulation functions can do free/repalloc
making the previous bitmapset pointer invalid.

------
Regards,
Alexander Korotkov