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+TgmoYz=M+2Z-heoBzJHAzs6heY=4icZ=-b5z6d2+XUw5CeRA@mail.gmail.com>
References: <20211219205422.GT17618@telsasoft.com>
 <2209221.1639948636@sss.pgh.pa.us>
 <CA+TgmobHkyGKM_DOgzGbh7BiWXdBGLMTnmB2S_FyyNHgqTxqBg@mail.gmail.com>
 <2445883.1640017555@sss.pgh.pa.us>
 <CA+Tgmoa7JFTYxM4AJEk+bM_J4Y+1sxQY+rAN3wE+ZmkbqWkCEA@mail.gmail.com>
 <2524668.1640024031@sss.pgh.pa.us>
 <CA+Tgmob2wVbfj47aqFxYeTuP_rcO45dip_GU-Hz=BSxu4POgXw@mail.gmail.com>
 <2598851.1640028995@sss.pgh.pa.us>
 <CA+TgmoYz=M+2Z-heoBzJHAzs6heY=4icZ=-b5z6d2+XUw5CeRA@mail.gmail.com>
Comments: In-reply-to Robert Haas <robertmhaas@gmail.com>
	message dated "Mon, 20 Dec 2021 15:17:01 -0500"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2657966.1640035211.1@sss.pgh.pa.us>
Date: Mon, 20 Dec 2021 16:20:11 -0500
Message-ID: <2657967.1640035211@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2657967.1640035211%40sss.pgh.pa.us>
Precedence: bulk

Robert Haas <robertmhaas@gmail.com> writes:
> OK ... but my point is that dump and restore does work. So whatever
> cases pg_get_expr() doesn't work must be cases that aren't needed for
> that to happen. Otherwise this problem would have been found long ago.

pg_get_expr doesn't (or didn't) depend on expression_tree_walker,
so there wasn't a problem there before.  I am worried that there
might be other code paths, now or in future, that could try to apply
expression_tree_walker/mutator to relpartbound trees, which is
why I think it's a reasonable idea to teach them about such trees.

>> It does fall over if you try to apply it to stored rules:
>> regression=# select pg_get_expr(ev_action, 0) from pg_rewrite;
>> ERROR:  unrecognized node type: 232
>> I'm not terribly excited about that, but maybe we should try to
>> improve it while we're here.

> In my view, the lack of excitement about sanity checks in functions
> that deal with node trees in the catalogs is the root of this problem.

It's only a problem if you hold the opinion that there should be
no user-reachable ERRCODE_INTERNAL_ERROR errors.  Which is a fine
ideal, but I fear we're a pretty long way off from that.

> I realize that's a deep hole out of which we're unlikely to be able to
> climb in the short or even medium term, but we don't have to keep
> digging. We either make a rule that pg_get_expr() can apply to
> everything stored in the catalogs and produce sensible answers, which
> seems to be what you prefer, or we make it return nice errors for the
> cases that it can't handle nicely, or some combination of the two. And
> whatever we decide, we also document and enforce everywhere.

I think having pg_get_expr throw an error for a query, as opposed to an
expression, is fine.  What I don't want to do is subdivide things a lot
more finely than that; thus lumping "relpartbound" into "expression"
seems like a reasonable thing to do.  Especially since we already did it
six years ago.

In a quick check of catalogs with pg_node_tree columns, I find these
other columns that pg_get_expr can fail on (at least with the
examples available in the regression DB):

regression=# select count(pg_get_expr(prosqlbody,0)) from pg_proc;
ERROR:  unrecognized node type: 232
regression=# select count(pg_get_expr(tgqual,tgrelid)) from pg_trigger ;
ERROR:  bogus varno: 2

So that looks like the same cases we already knew about: input is
a querytree not an expression, or it contains Vars for more than
one relation.

			regards, tom lane