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

On Tue, Dec 21, 2021 at 6:20 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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