From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
cc: Justin Pryzby <pryzby@telsasoft.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: sqlsmith: ERROR: XX000: bogus varno: 2
In-reply-to: 
 <CA+TgmobHkyGKM_DOgzGbh7BiWXdBGLMTnmB2S_FyyNHgqTxqBg@mail.gmail.com>
References: <20211219205422.GT17618@telsasoft.com>
 <2209221.1639948636@sss.pgh.pa.us>
 <CA+TgmobHkyGKM_DOgzGbh7BiWXdBGLMTnmB2S_FyyNHgqTxqBg@mail.gmail.com>
Comments: In-reply-to Robert Haas <robertmhaas@gmail.com>
	message dated "Mon, 20 Dec 2021 09:49:09 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <2445354.1640017418.0@sss.pgh.pa.us>
Date: Mon, 20 Dec 2021 11:25:55 -0500
Message-ID: <2445883.1640017555@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2445883.1640017555%40sss.pgh.pa.us>
Precedence: bulk

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Dec 19, 2021 at 4:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't see anything particularly surprising here.  pg_get_expr is only
>> able to cope with expression trees over a single relation, but ON UPDATE
>> rules can refer to both OLD and NEW relations.  Maybe we could make the
>> error message more friendly, but there's not much else to be done,
>> I think.

> +1 for making the error message more friendly.

The problem is that the spot where it's thrown doesn't have a lot of
context.  We can fix that by having pg_get_expr itself check for
out-of-spec Vars before starting the recursion, which adds a bit of
overhead but I don't think we're terribly concerned about that.

I figured this would be just a quick hack in ruleutils.c, but was
dismayed to find the regression tests falling over, because some
bozo neglected to teach nodeFuncs.c about partition expressions.
It might be a good idea to back-patch that part, before we find
some other place that fails.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="check-for-bogus-vars.patch";
	charset="us-ascii"
Content-ID: <2445354.1640017418.2@sss.pgh.pa.us>
Content-Description: check-for-bogus-vars.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index e276264882..5d4700430c 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2201,6 +2201,26 @@ expression_tree_walker(Node *node,
 					return true;
 			}
 			break;
+		case T_PartitionBoundSpec:
+			{
+				PartitionBoundSpec *pbs =3D (PartitionBoundSpec *) node;
+
+				if (walker(pbs->listdatums, context))
+					return true;
+				if (walker(pbs->lowerdatums, context))
+					return true;
+				if (walker(pbs->upperdatums, context))
+					return true;
+			}
+			break;
+		case T_PartitionRangeDatum:
+			{
+				PartitionRangeDatum *prd =3D (PartitionRangeDatum *) node;
+
+				if (walker(prd->value, context))
+					return true;
+			}
+			break;
 		case T_List:
 			foreach(temp, (List *) node)
 			{
@@ -3092,6 +3112,28 @@ expression_tree_mutator(Node *node,
 				return (Node *) newnode;
 			}
 			break;
+		case T_PartitionBoundSpec:
+			{
+				PartitionBoundSpec *pbs =3D (PartitionBoundSpec *) node;
+				PartitionBoundSpec *newnode;
+
+				FLATCOPY(newnode, pbs, PartitionBoundSpec);
+				MUTATE(newnode->listdatums, pbs->listdatums, List *);
+				MUTATE(newnode->lowerdatums, pbs->lowerdatums, List *);
+				MUTATE(newnode->upperdatums, pbs->upperdatums, List *);
+				return (Node *) newnode;
+			}
+			break;
+		case T_PartitionRangeDatum:
+			{
+				PartitionRangeDatum *prd =3D (PartitionRangeDatum *) node;
+				PartitionRangeDatum *newnode;
+
+				FLATCOPY(newnode, prd, PartitionRangeDatum);
+				MUTATE(newnode->value, prd->value, Node *);
+				return (Node *) newnode;
+			}
+			break;
 		case T_List:
 			{
 				/*
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/rul=
eutils.c
index 8da525c715..5d9596d445 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2622,6 +2625,7 @@ static text *
 pg_get_expr_worker(text *expr, Oid relid, const char *relname, int pretty=
Flags)
 {
 	Node	   *node;
+	Relids		relids;
 	List	   *context;
 	char	   *exprstr;
 	char	   *str;
@@ -2634,6 +2638,27 @@ pg_get_expr_worker(text *expr, Oid relid, const cha=
r *relname, int prettyFlags)
 =

 	pfree(exprstr);
 =

+	/*
+	 * Throw a user-friendly error if the expression contains Vars we won't =
be
+	 * able to deparse.  pull_varnos requires a PlannerInfo, but fortunately
+	 * that can be dummy -- it need only contain an empty placeholder_list.
+	 */
+	relids =3D pull_varnos(makeNode(PlannerInfo), node);
+	if (OidIsValid(relid))
+	{
+		if (!bms_is_subset(relids, bms_make_singleton(1)))
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("expression contains variables of more than one relation")))=
;
+	}
+	else
+	{
+		if (!bms_is_empty(relids))
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("expression contains variables")));
+	}
+
 	/* Prepare deparse context if needed */
 	if (OidIsValid(relid))
 		context =3D deparse_context_for(relname, relid);

------- =_aaaaaaaaaa0--