From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: sqlsmith: ERROR: XX000: bogus varno: 2
Date: Mon, 20 Dec 2021 16:20:11 -0500
Message-ID: <2657967.1640035211@sss.pgh.pa.us> (raw)
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, robertmhaas@gmail.com, pryzby@telsasoft.com, pgsql-hackers@lists.postgresql.org
Subject: Re: sqlsmith: ERROR: XX000: bogus varno: 2
In-Reply-To: <2657967.1640035211@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox