Re: deep copy with mutation? - Mailing list pgsql-hackers

On Thu, Apr 30, 2026 at 2:52 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> I spent a lot of time today being stupid before finally understanding
> that expression_tree_mutator() and query_tree_mutator() can't be used
> to substitute for copyObject() because they deep copy structure only
> if it's Node structure; non-nodes like plain old C strings are not
> deep-copied. But then I wondered why we don't have a thing that works
> that way, because we have stuff like this in a number of places:
>
>             parsetree->returningList = copyObject(parsetree->returningList);
>             ChangeVarNodes((Node *) parsetree->returningList, rt_index,
>                            parsetree->resultRelation, 0);
>
> If we had a fully-deeply-copying version of copyObject(), it could
> just adjust new Var nodes as it created them, instead of needing a
> separate tree walk.
>
> In CreateTriggerFiringOn, we have:
>
>             qual = copyObject(whenClause);
>             qual = (Node *)
>                 map_partition_varattnos((List *) qual, PRS2_OLD_VARNO,
>                                         childTbl, rel);
>             qual = (Node *)
>                 map_partition_varattnos((List *) qual, PRS2_NEW_VARNO,
>                                         childTbl, rel);
>
> So a copy and then two consecutive mutation passes.
>
> Or similarly in rewriteTargetView:
>
>          tmp_tlist = copyObject(view_targetlist);
>
>          ChangeVarNodes((Node *) tmp_tlist, new_rt_index,
>                         new_exclRelIndex, 0);
>
>          parsetree->onConflict = (OnConflictExpr *)
>              ReplaceVarsFromTargetList((Node *) parsetree->onConflict,
>                                        old_exclRelIndex,
>                                        0,
>                                        view_rte,
>                                        tmp_tlist,
>                                        new_rt_index,
>                                        REPLACEVARS_REPORT_ERROR,
>                                        0,
>                                        &parsetree->hasSubLinks);
>
> I'm not entirely certain how much this kind of thing matters -- maybe
> it's better to have the copy routine be as simple as possible and
> accept the cost of walking the whole tree immediately afterwards to
> make changes? Perhaps this kind of thing is so cache-friendly that the
> mutation pass is really cheap? But it certainly kinda *looks*
> inefficient...

It looks inefficient in terms of CPU as well as memory since FLATCOPY
itself does palloc_object() and I see some mutators using
copyObject(). So we are copying the whole tree node by node and then
parts of the tree are copied by mutators. I think what's needed is
instead of FLATCOPY we need a version of copyObject which just creates
a copy of the node without copying the subtrees but copying the
structures like C strings and arrays. copyObject() already copies
arrays and C strings. Add a flag to copyObject to indicate whether we
want copy subtree or not and then replace FLATCOPY with
copyObject(node, false) and all existing copyObject as
copyObject(node, true). When generating _copy* functions, we define
COPY_NODE_FIELD as COPY_SCALAR_FIELD. Would that work?

--
Best Wishes,
Ashutosh Bapat