Re: sqlsmith: ERROR: XX000: bogus varno: 2

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