Getting rid of CaseTestExpr (well, partially)

We've wanted to refactor or get rid of CaseTestExpr for quite awhile,
as a consequence of bugs caused by the loose connection between
CaseTestExpr and the node supplying its value. The increasing use of
it for random purposes like JsonExpr hasn't made things any safer.
While this has been in the back of my mind for awhile, looking at
the discussion at [1] nerd-sniped me into trying to actually do it.

My initial idea was to generate PARAM_EXEC Params instead of
CaseTestExprs, relying on unique assignment of their param IDs to
distinguish different values within an expression. While it'd not be
hard to get the parser to assign distinct IDs within one Query tree,
I soon found that maintaining their uniqueness through operations like
rule rewriting, subquery pullup, and qual pushdown into subqueries
would be a nightmare. Furthermore, it didn't seem like it'd be buying
much. None of those operations deform expressions in a way that would
risk inserting a different use of CaseTestExpr between a source and
target node. The thing that does cause such problems is inlining of
SQL function bodies into expressions.

Therefore, my proposal here is to leave the parser's usage of
CaseTestExpr alone, and replace CaseTestExprs with Params during
eval_const_expressions, just before any relevant function inlining
could happen. (The information transfer needed to replace them
happens during initial descent of the expression tree, while
function inlining would happen on the way back up. So by the
time a function inlining step happens, any CTEs in its arguments
will have become Params, and no ambiguity is possible.)

The executor will never see any CaseTestExprs any more, so its
support for them can still go away.

I also discovered that re-using PARAM_EXEC Params wasn't as good
an idea as it seemed. The setParam/parParam signaling mechanisms
assume that PARAM_EXEC Params represent cross-plan-node data
transfer, and the parallel-query logic does too. So this patch
series invents a new paramkind for intra-expression data transfer,
which I called PARAM_EXPR for lack of a better idea.

Given those decisions, the actual coding seemed pretty
straightforward, if tedious.

I'd originally thought of similarly replacing CoerceToDomainValue
with a Param, but desisted after finding that it wasn't as simple as
I'd hoped. There is not really any ambiguity in what's referenced by
such a node, since domain CHECK expressions can never get intermixed,
so the benefit would be quite minimal.

One idea that's left undone in the attached patch series is to
rename CaseTestExpr to something more generic that reflects its
current usage (and then rewrite the comments that claim the other
usages are "abuse"). That would be a purely cosmetic change and
I ran out of energy. Besides, I don't have a great idea for a
new name. I thought of ParamPlaceHolder to betoken that the
things will eventually get replaced by Params, but that seems
to risk confusion with the planner's PlaceHolderVar nodes.
Any ideas?

There's more detail in the per-patch commit messages below.

regards, tom lane

[1] https://www.postgresql.org/message-id/flat/CACpMh%2BAiBYAWn%2BD1aU7Rsy-V1tox06Cbc0H3qA7rwL5zdJ%3DanQ%40mail.gmail.com

I wrote:
> Therefore, my proposal here is to leave the parser's usage of
> CaseTestExpr alone, and replace CaseTestExprs with Params during
> eval_const_expressions, just before any relevant function inlining
> could happen.

I thought of a nasty defect in this idea: CASE expressions that would
have been seen as equal() may not be equal after the transformation.
(The upper nodes are fine because we can make their *param fields be
equal_ignore, but the lower PARAM_EXPRs won't match.) There's at
least one important behavior this probably breaks, which is matching
of index expressions containing a CASE or ArrayCoerce to query quals.
That might be a rare use-case, but it'd annoy anyone doing it, and
it'd be pretty hard to explain why it's not a bug.

I spent some time wondering if we could somehow number the Params
"bottom up" to fix this, so that (for example) a CASE would always
use paramid 1 unless it contains one other CASE, which would cause it
to use paramid 2, etc. I think eval_const_expressions could be made
to do that, but it's not clear how to preserve the property during
function inlining. But the main thing I don't like is that this would
make it much less obvious that Params with overlapping lifespans have
distinct IDs, which takes away a large part of the attraction of the
whole design.

Pending some better idea, I'm withdrawing this patch.

regards, tom lane