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

On Mon, Dec 20, 2021 at 2:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm not sure why you're astonished by that, considering that
> psql has applied pg_get_expr to relpartbound since f0e44751d,
> which was the same commit that put code into ruleutils.c to
> make pg_get_expr work on relpartbounds.
>
> It seems a bit late to change our minds on this; and anyway,
> if pg_get_expr didn't handle them, we'd just need to invent
> another function that did.

OK.

> > Alternatively, maybe pg_get_expr() should just fail and tell you that
> > this is not an expression, and if you want to see what's in that
> > column, you should use the SQL-callable functions specifically
> > provided for that purpose (pg_get_partkeydef, I think).
>
> pg_get_partkeydef does something different.
>
> regression=# select pg_get_expr(relpartbound,oid) from pg_class where relname = 'beta_neg';
>            pg_get_expr
> ----------------------------------
>  FOR VALUES FROM ('-10') TO ('0')
> (1 row)
>
> regression=# select pg_get_partkeydef('beta_neg'::regclass);
>  pg_get_partkeydef
> -------------------
>  RANGE (b)
> (1 row)

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.

> > I don't know
> > why it should be legitimate for pg_get_expr() to just assume that any
> > random node tree it gets handed must be an expression without doing
> > any sanity checking.
>
> 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.
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 don't think it's any more correct for pg_get_expr() to elog(ERROR,
"some internal thing") than it would be for to_timestamp() or
date_bin() or whatever to do something similar. And I think that
careful thinking about supported cases makes life easier for both
users (who know that if they see some junk error report, it's a
mistake rather than intentional) and for developers (who then have a
better chance of knowing what code they need to update to avoid
getting called bozos). Sloppy thinking about which cases are supported
and unsupported leads to bugs, and some of those are likely to be
security bugs.

-- 
Robert Haas
EDB: http://www.enterprisedb.com