On Thu, Jan 6, 2022 at 3:43 PM Amit Langote <amitlangote09@gmail.com> wrote:
> 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...
Also, I wondered if it might be a good idea to expand the comment
above NodeTag definition in nodes.h to tell someone adding new types
to also look in nodeFuncs.c to check if any of the functions there
need to be updated.
--
Amit Langote
EDB: http://www.enterprisedb.com