James Coleman <jtc...@gmail.com> writes:
> Two things I wonder:
> 1. Should we add tests for the relabel code path?
As far as that goes, the Relabel-stripping loops in
find_ec_member_matching_expr are already exercised in the core
regression tests (I didn't bother to discover exactly where, but
a quick coverage test run says that they're hit). The ones in
exprlist_member_ignore_relabel are not iterated though. On
reflection, the first loop stripping the input node is visibly
unreachable by the sole caller, since everything in the exprvars
list will be a Var, Aggref, WindowFunc, or PlaceHolderVar. I'm
less sure about what is possible in the targetlist that we're
referencing, but it strikes me that ignoring relabel on that
side is probably functionally wrong: if we have say "f(textcol)"
as an expression to be sorted on, but what is in the tlist is
textcol::varchar or the like, I do not think setrefs.c will
consider that an acceptable match. So now that's seeming like
an actual bug --- although the lack of field reports suggests
that it's unreachable, most likely because if we do have
"f(textcol)" as a sort candidate, we'll have made sure to emit
plain "textcol" from the source relation, regardless of whether
there might also be a reason to emit textcol::varchar.
Anyway I'm now inclined to remove that behavior from
find_computable_ec_member, and adjust comments accordingly.
> 2. It'd be nice not to have the IS_SRF_CALL duplicated, but that might
> add enough complexity that it's not worth it.
Yeah, I'd messed around with variants that put more smarts
into the bottom-level functions, and decided that it wasn't
a net improvement.
regards, tom lane
