From: Tom Lane <tgl@sss.pgh.pa.us>
To: exclusion@gmail.com
cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18360: Invalid memory access occurs when using geqo
In-reply-to: <831847.1708710339@sss.pgh.pa.us>
References: <18360-a23caf3157f34e62@postgresql.org>
 <831847.1708710339@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Fri, 23 Feb 2024 12:45:39 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <891278.1708716812.0@sss.pgh.pa.us>
Date: Fri, 23 Feb 2024 14:33:49 -0500
Message-ID: <891306.1708716829@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/891306.1708716829%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <891278.1708716812.1@sss.pgh.pa.us>

I wrote:
> Indeed.  Curiously, the in_operators list, which I thought was
> parallel to that, seems fine.  Anyway, something's being careless
> about which context it creates that data structure in.  Shouldn't
> be too hard to fix.

The problem seems to arise from build_child_join_sjinfo(), which
makes a translated version of the semi_rhs_exprs that is later
propagated into a UniquePath for a base relation.  This breaks
GEQO's intention that base-relation structs will be long-lived
while only join-relation data is short-lived.  (in_operators
is not modified so the original long-lived list is used for that,
explaining why it's not trashed at the same time.)

The simplest fix is as attached: just do a quick copyObject
in create_unique_path.  That's rather ugly, but create_unique_path
is already taking explicit responsibility for the context that the
Path is built in, so it doesn't quite exceed my threshold of pain.
(The alternative of making build_child_join_sjinfo force its output
to be long-lived doesn't look good: that gets invoked quite a few
times during a GEQO cycle.)  I chose to make it copy the in_operators
list as well, even though that's not minimally necessary to fix
the bug --- it would look odd if we didn't, and the copy is pretty
cheap since it's only a short integer list.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="0001-fix-bug-18360.patch";
	charset="us-ascii"
Content-ID: <891278.1708716812.2@sss.pgh.pa.us>
Content-Description: 0001-fix-bug-18360.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer=
/util/pathnode.c
index b3902b0032..6f79b2e3fe 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1707,8 +1707,13 @@ create_unique_path(PlannerInfo *root, RelOptInfo *r=
el, Path *subpath,
 	pathnode->path.pathkeys =3D NIL;
 =

 	pathnode->subpath =3D subpath;
-	pathnode->in_operators =3D sjinfo->semi_operators;
-	pathnode->uniq_exprs =3D sjinfo->semi_rhs_exprs;
+
+	/*
+	 * Under GEQO, the sjinfo might be short-lived, so we'd better make copi=
es
+	 * of data structures we extract from it.
+	 */
+	pathnode->in_operators =3D copyObject(sjinfo->semi_operators);
+	pathnode->uniq_exprs =3D copyObject(sjinfo->semi_rhs_exprs);
 =

 	/*
 	 * If the input is a relation and it has a unique index that proves the

------- =_aaaaaaaaaa0--