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

On Tue, Dec 21, 2021 at 6:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.
>
> > 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.

I admit that it was an oversight on my part that relpartbound trees
are not recognized by nodeFuncs.c. :-(

Thanks for addressing that in the patch you posted.  I guess fixing
only expression_tree_walker/mutator() suffices for now, but curious to
know if it was intentional that you decided not to touch the following
sites:

exprCollation(): it would perhaps make sense to return the collation
assigned to the 1st element of listdatums/lowerdatums/upperdatums,
especially given that transformPartitionBoundValue() does assign a
collation to the values in those lists based on the parent's partition
key specification.

exprType(): could be handled similarly

queryjumble.c: JumbleExpr(): whose header comment says:

 * expression_tree_walker() does, and therefore it's coded to be as parallel
 * to that function as possible.
 * ...
 * Note: the reason we don't simply use expression_tree_walker() is that the
 * point of that function is to support tree walkers that don't care about
 * most tree node types, but here we care about all types.  We should complain
 * about any unrecognized node type.

or maybe not, because relpartbound contents ought never reach queryjumble.c?


--
Amit Langote
EDB: http://www.enterprisedb.com