From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Getting rid of CaseTestExpr (well, partially) |
Date: | 2025-01-30 03:10:54 |
Message-ID: | 3068812.1738206654@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
v1-0001-Replace-CaseTestExpr-with-a-Param-during-planning.patch | text/x-diff | 54.6 KB |
v1-0002-Convert-ArrayCoerceExpr-to-use-a-Param-instead-of.patch | text/x-diff | 10.9 KB |
v1-0003-Remove-contain_context_dependent_node-function-in.patch | text/x-diff | 6.2 KB |
v1-0004-Convert-JsonConstructorExpr-JsonExpr-to-use-a-Par.patch | text/x-diff | 11.8 KB |
v1-0005-Convert-nested-assignments-to-use-Params-instead-.patch | text/x-diff | 23.3 KB |
v1-0006-Remove-plpgsql-s-abuse-of-CaseTestExpr.patch | text/x-diff | 5.1 KB |
v1-0007-Remove-dead-code-for-executor-support-of-CaseTest.patch | text/x-diff | 15.3 KB |
v1-0008-Optimize-away-useless-Param-set-load-pairs.patch | text/x-diff | 3.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2025-01-30 03:29:32 | Re: JIT: The nullness of casetest.value can be determined at the JIT compile time. |
Previous Message | Michail Nikolaev | 2025-01-30 01:00:00 | Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements |