Horton
Menu

Thread

  1. deep copy with mutation?

    Robert Haas <robertmhaas@gmail.com> — 2026-04-29T21:22:05Z

    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...

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  2. Re: deep copy with mutation?

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-04-30T10:33:54Z

    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

  3. Re: deep copy with mutation?

    David Rowley <dgrowleyml@gmail.com> — 2026-06-02T23:10:22Z

    On Thu, 30 Apr 2026 at 09:22, Robert Haas <robertmhaas@gmail.com> wrote:
> 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...

These tree traversals are often pretty expensive in terms of the
overhead of stack manipulation and function call overhead. Having
something to reduce those, I expect, could be made to demonstrate
speedups. We also shouldn't forget about the reduction in pallocs and
wasted memory from the to-be-mutated-later nodes having a redundant
copy made by copyObject(). There's also the memory fragmentation issue
of that, which would increase the likelihood of cache misses later due
to non-consecutive memory access in later traversals. Whereas if the
copy/mutate was done in one pass, and palloc() didn't reuse any chunks
from the freelists, then the memory would be consecutive in tree
traversal order.

I think the main deciding factor whether we should do this is the
complexity of doing it in terms of code changes vs. observed speedup.
In the first example you gave, map_partition_varattnos() is called
twice, so if that were to perform a full copy during that traversal,
then you'd need to make that function do both replacements at the same
time to avoid copying again on the next call.

Maybe you could quickly figure out if this is worth the trouble by
adding some additional copyObjects in the places you think could be
made better to see if you can see a slowdown.  If you can't measure
that, then you could probably assume that removing a traversal
wouldn't result in much of a speedup.

David