On Mon, Nov 2, 2026 at 5:34 AM Chao Li <[email protected]> wrote: > V1 overall looks good to me. There seems a typo in the commit message: > ``` > heap slot) and (2) the trigger returned NEW unchanged (or NULL in > ExecIRInsertTriggers). > ``` > > I think it should be "or NULL in ExecIRUpdateTriggers".
Thank you for the review! You're right; fixed. On Mon, Nov 2, 2026 at 11:03 PM surya poondla <[email protected]> wrote: > I also found that you don't need a view to hit this. A plain BEFORE > INSERT trigger on a partitioned table leaks just as much when the row is > routed to a partition with the same column layout as the parent. In that > case there's no conversion map, so ExecPrepareTupleRouting() passes the > root's virtual slot straight through and ExecBRInsertTriggers() ends up > with a copy it never frees. Inserting directly into the partition, or > routing to one with a different column order, doesn't leak. Maybe we can > put that in the commit message but not really needed. Thank you too! I updated the commit message to mention that case. v2 attached. Yours, -- Paul ~{:-) [email protected]
From 99d5cbe11ef02db190e75e375a0b8255d23d1f8e Mon Sep 17 00:00:00 2001 From: "Paul A. Jungwirth" <[email protected]> Date: Sat, 12 Sep 2026 10:51:49 -0700 Subject: [PATCH v2] Free the tuple fetched for BEFORE/INSTEAD OF row triggers We were leaking the HeapTuple fetched by ExecBRInsertTriggers, ExecBRUpdateTriggers, ExecIRInsertTriggers, and ExecIRUpdateTriggers if both (1) the slot was virtual, so fetching a tuple made a copy, and (2) the trigger returned NEW unchanged (or NULL in ExecIRUpdateTriggers). Views use virtual slots, and so do partitioned tables. Tuple routing passes the parent's slot straight through to a partition with the same column layout, so a BEFORE INSERT trigger on such a partition leaked too. Reported-by: Andres Freund <[email protected]> Author: Paul A. Jungwirth <[email protected]> Reviewed-by: Chao Li <[email protected]> Reviewed-by: Surya Poondla <[email protected]> Backpatch-through: 14 --- src/backend/commands/trigger.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 46ba932383f..ce2088c9cc1 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -2508,7 +2508,7 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo, { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; HeapTuple newtuple = NULL; - bool should_free; + bool should_free = false; TriggerData LocTriggerData = {0}; int i; @@ -2577,6 +2577,10 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo, } } + /* Free our copy of the tuple, if the last trigger handed it back. */ + if (newtuple != NULL && should_free) + heap_freetuple(newtuple); + return true; } @@ -2612,7 +2616,7 @@ ExecIRInsertTriggers(EState *estate, ResultRelInfo *relinfo, { TriggerDesc *trigdesc = relinfo->ri_TrigDesc; HeapTuple newtuple = NULL; - bool should_free; + bool should_free = false; TriggerData LocTriggerData = {0}; int i; @@ -2664,6 +2668,10 @@ ExecIRInsertTriggers(EState *estate, ResultRelInfo *relinfo, } } + /* Free our copy of the tuple, if the last trigger handed it back. */ + if (newtuple != NULL && should_free) + heap_freetuple(newtuple); + return true; } @@ -3165,6 +3173,10 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, newtuple = NULL; } } + + /* Likewise free our copy of the new tuple, if we still have it. */ + if (newtuple != NULL && should_free_new) + heap_freetuple(newtuple); if (should_free_trig) heap_freetuple(trigtuple); @@ -3258,7 +3270,7 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo, TriggerDesc *trigdesc = relinfo->ri_TrigDesc; TupleTableSlot *oldslot = ExecGetTriggerOldSlot(estate, relinfo); HeapTuple newtuple = NULL; - bool should_free; + bool should_free = false; TriggerData LocTriggerData = {0}; int i; @@ -3300,6 +3312,8 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo, GetPerTupleMemoryContext(estate)); if (newtuple == NULL) { + if (should_free) + heap_freetuple(oldtuple); return false; /* "do nothing" */ } else if (newtuple != oldtuple) @@ -3314,6 +3328,10 @@ ExecIRUpdateTriggers(EState *estate, ResultRelInfo *relinfo, } } + /* Free our copy of the new tuple, if the last trigger handed it back. */ + if (newtuple != NULL && should_free) + heap_freetuple(newtuple); + return true; } -- 2.47.3
