Thread: Violation of principle that plan trees are read-only
While chasing down Valgrind leakage reports, I was disturbed
to realize that some of them arise from a case where the
executor scribbles on the plan tree it's given, which it is
absolutely not supposed to do:
/*
* Initialize result tuple slot and assign its rowtype using the first
* RETURNING list. We assume the rest will look the same.
*/
mtstate->ps.plan->targetlist = (List *) linitial(returningLists);
A bit of git archaeology fingers Andres' commit 4717fdb14, which we
can't easily revert since he later got rid of ExecAssignResultType
altogether. But I think we need to do something about it --- it's
purest luck that this doesn't cause serious problems in some cases.
regards, tom lane
On Sun, May 18, 2025 at 7:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > While chasing down Valgrind leakage reports, I was disturbed > to realize that some of them arise from a case where the > executor scribbles on the plan tree it's given, which it is > absolutely not supposed to do: > > /* > * Initialize result tuple slot and assign its rowtype using the first > * RETURNING list. We assume the rest will look the same. > */ > mtstate->ps.plan->targetlist = (List *) linitial(returningLists); > > A bit of git archaeology fingers Andres' commit 4717fdb14, which we > can't easily revert since he later got rid of ExecAssignResultType > altogether. But I think we need to do something about it --- it's > purest luck that this doesn't cause serious problems in some cases. Is there some way that we can detect violations of this rule automatically? I recall that we were recently discussing with Richard Guo a proposed patch that would have had a similar problem, so it's evidently not that hard for a committer to either fail to understand what the rule is or fail to realize that they are violating it. -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, May 19, 2025 at 10:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I proposed a possible way to test for this at [1]. I was intending to > get around to that sooner or later, but the urgency of the matter just > went up in my eyes... Ah, right, I actually read that thread but had forgotten about it. I don't know if that idea will work out but it certainly seems like a good thing to try. -- Robert Haas EDB: http://www.enterprisedb.com
Isaac Morland <isaac.morland@gmail.com> writes:
> I assume this question has an obvious negative answer, but why can't we
> attach const declarations to the various structures that make up the plan
> tree (at all levels, all the way down)? I know const doesn't actually
> prevent a value from changing, but at least the compiler would complain if
> code accidentally tried.
The big problem is that a "const" attached to a top-level pointer
doesn't inherently propagate down to sub-nodes. So if I had, say,
"const Query *stmt", the compiler would complain about
stmt->jointree = foo;
but not about
stmt->jointree->quals = foo;
I guess we could imagine developing an entirely parallel set of
struct declarations with "const" on all pointer fields, like
typedef struct ConstQuery
{
...
const ConstFromExpr *jointree;
...
} ConstQuery;
but even with automated maintenance of the ConstFoo doppelganger
typedefs, it seems like that'd be a notational nightmare. For
one thing, I'm not sure how to teach the compiler that casting
"Query *" to "ConstQuery *" is okay but vice versa isn't.
Does C++ have a better story in this area? I haven't touched it
in so long that I don't remember.
regards, tom lane
On 19/5/25 16:45, Tom Lane wrote:
> [snip]
> For one thing, I'm not sure how to teach the compiler that casting
> "Query *" to "ConstQuery *" is okay but vice versa isn't.
>
> Does C++ have a better story in this area?
Hi,
A C++ compiler *does* enforce "const correctness" (for examples
see, for example, Meyer's fantastic "Efficient C++" series).
It seems that Postgres is approaching the class of complexity/maturity
that (modern) C++ was designed to solve. It should be unmatched in that
area. I believe that, with recent platform deprecations, the vast
majority of systems where one can compile a modern Postgres should
already have a decent C++ compiler available (g++ & clang probably cover
>95%)
As opposed to say, rewriting in Rust ---where the compilar also does a
very substantial checking of semantics at compile time---, introducing
some C++ in Postgres could be as simple as throwing some "#ifdef
__cplusplus__ extern "C" { #endif" in the headers and use the required
subset in certain modules *only*.
(this is how, for instance, Bacula did it ~two decades ago)
The planner and parts of the bufmgr / arenas (palloc et al.) seem to me
as the most obvious candidates to try. I'm not sure whether it'd
introduce any unintended regressions, though.
> I haven't touched it
> in so long that I don't remember.
C++ 17 would be the standard to tackle, IMHO.
HTH,
J.L.
--
Parkinson's Law: Work expands to fill the time alloted to it.
Hi, On 2025-05-18 19:31:33 -0400, Tom Lane wrote: > While chasing down Valgrind leakage reports, I was disturbed > to realize that some of them arise from a case where the > executor scribbles on the plan tree it's given, which it is > absolutely not supposed to do: > > /* > * Initialize result tuple slot and assign its rowtype using the first > * RETURNING list. We assume the rest will look the same. > */ > mtstate->ps.plan->targetlist = (List *) linitial(returningLists); > > A bit of git archaeology fingers Andres' commit 4717fdb14, which we > can't easily revert since he later got rid of ExecAssignResultType > altogether. But I think we need to do something about it --- it's > purest luck that this doesn't cause serious problems in some cases. I have no idea what I was smoking at that time, this clearly is wrong. I think the reason it doesn't cause problems is that we're just using the first child's targetlist every time and we're just assigning the same value back every time. What's even more absurd is: Why do we even need to assign the result type at all? Before & after 4717fdb14. The planner ought to have already figured this out, no? It seems that if not, we'd have a problem anyway, who says the "calling" nodes (say a wCTE) can cope with whatever output we come up with? Except of course, there is exactly one case in our tests where the tupledescs aren't equal :( I've not dug fully into this, but I thought I should send this email to confirm that I'm looking into the issue. Greetings, Andres Freund
I wrote:
> I think we can just delete this assignment (and fix these comments).
As attached. I'm tempted to back-patch this: the plan tree damage
seems harmless at present, but maybe it'd become less harmless with
future fixes.
regards, tom lane
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 46d533b7288..71bc174cfc9 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -4830,12 +4830,11 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
ExprContext *econtext;
/*
- * Initialize result tuple slot and assign its rowtype using the first
- * RETURNING list. We assume the rest will look the same.
+ * Initialize result tuple slot and assign its rowtype using the plan
+ * node's declared targetlist, which the planner set up to be the same
+ * as the first RETURNING list. We assume the rest will produce
+ * compatible output.
*/
- mtstate->ps.plan->targetlist = (List *) linitial(returningLists);
-
- /* Set up a slot for the output of the RETURNING projection(s) */
ExecInitResultTupleSlotTL(&mtstate->ps, &TTSOpsVirtual);
slot = mtstate->ps.ps_ResultTupleSlot;
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 150e9f060ee..8e1eb77dd49 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1097,9 +1097,10 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
/*
* Set up the visible plan targetlist as being the same as
- * the first RETURNING list. This is for the use of
- * EXPLAIN; the executor won't pay any attention to the
- * targetlist. We postpone this step until here so that
+ * the first RETURNING list. This is mostly for the use
+ * of EXPLAIN; the executor won't execute that targetlist,
+ * although it does use it to prepare the node's result
+ * tuple slot. We postpone this step until here so that
* we don't have to do set_returning_clause_references()
* twice on identical targetlists.
*/