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