Fix crash caused by EPQ happening with a before update trigger present.

When ExecBRUpdateTriggers()'s GetTupleForTrigger() follows an EPQ
chain the former needs to run the result tuple through the junkfilter
again, and update the slot containing the new version of the tuple to
contain that new version. The input tuple may already be in the
junkfilter's output slot, which used to be OK - we don't need the
previous version anymore. Unfortunately ff11e7f4b9ae started to use
ExecCopySlot() to update newslot, and ExecCopySlot() doesn't support
copying a slot into itself, leading to a slot in a corrupt
state, which then can cause crashes or other symptoms.

Fix this by skipping the ExecCopySlot() when copying into itself.

While we could have easily made ExecCopySlot() handle that case, it
seems better to add an assert forbidding doing so instead. As the goal
of copying might be to make the contents of one slot independent from
another, it seems failure prone to handle doing so silently.

A follow-up commit will add tests for the obviously under-covered
combination of EPQ and triggers. Done as a separate commit as it might
make sense to backpatch them further than this bug.

Also remove confusion with confusing variable names for slots in
ExecBRDeleteTriggers() and ExecBRUpdateTriggers().

Bug: #16036
Reported-By: Антон Власов
Author: Andres Freund
Discussion: https://postgr.es/m/16036-28184c90d952fb7f@postgresql.org
Backpatch: 12-, where ff11e7f4b9ae was merged

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index cdb1105b4a7f5cd96cf182bb449e00c29142b0d0..7ba859d446d6c459bd7a551d8bb08dfbebc64006 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2773,10 +2773,10 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
    Assert(HeapTupleIsValid(fdw_trigtuple) ^ ItemPointerIsValid(tupleid));
    if (fdw_trigtuple == NULL)
    {
-       TupleTableSlot *newSlot;
+       TupleTableSlot *epqslot_candidate = NULL;
 
        if (!GetTupleForTrigger(estate, epqstate, relinfo, tupleid,
-                               LockTupleExclusive, slot, &newSlot))
+                               LockTupleExclusive, slot, &epqslot_candidate))
            return false;
 
        /*
@@ -2784,9 +2784,9 @@ ExecBRDeleteTriggers(EState *estate, EPQState *epqstate,
         * function requested for the updated tuple, skip the trigger
         * execution.
         */
-       if (newSlot != NULL && epqslot != NULL)
+       if (epqslot_candidate != NULL && epqslot != NULL)
        {
-           *epqslot = newSlot;
+           *epqslot = epqslot_candidate;
            return false;
        }
 
@@ -3026,11 +3026,11 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
    Assert(HeapTupleIsValid(fdw_trigtuple) ^ ItemPointerIsValid(tupleid));
    if (fdw_trigtuple == NULL)
    {
-       TupleTableSlot *newSlot = NULL;
+       TupleTableSlot *epqslot_candidate = NULL;
 
        /* get a copy of the on-disk tuple we are planning to update */
        if (!GetTupleForTrigger(estate, epqstate, relinfo, tupleid,
-                               lockmode, oldslot, &newSlot))
+                               lockmode, oldslot, &epqslot_candidate))
            return false;       /* cancel the update action */
 
        /*
@@ -3045,11 +3045,14 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
         * nor our caller have any more interest in the prior contents of that
         * slot.
         */
-       if (newSlot != NULL)
+       if (epqslot_candidate != NULL)
        {
-           TupleTableSlot *slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot);
+           TupleTableSlot *epqslot_clean;
 
-           ExecCopySlot(newslot, slot);
+           epqslot_clean = ExecFilterJunk(relinfo->ri_junkFilter, epqslot_candidate);
+
+           if (newslot != epqslot_clean)
+               ExecCopySlot(newslot, epqslot_clean);
        }
 
        trigtuple = ExecFetchSlotHeapTuple(oldslot, true, &should_free_trig);
@@ -3311,17 +3314,17 @@ GetTupleForTrigger(EState *estate,
                   ItemPointer tid,
                   LockTupleMode lockmode,
                   TupleTableSlot *oldslot,
-                  TupleTableSlot **newSlot)
+                  TupleTableSlot **epqslot)
 {
    Relation    relation = relinfo->ri_RelationDesc;
 
-   if (newSlot != NULL)
+   if (epqslot != NULL)
    {
        TM_Result   test;
        TM_FailureData tmfd;
        int         lockflags = 0;
 
-       *newSlot = NULL;
+       *epqslot = NULL;
 
        /* caller must pass an epqstate if EvalPlanQual is possible */
        Assert(epqstate != NULL);
@@ -3361,21 +3364,20 @@ GetTupleForTrigger(EState *estate,
            case TM_Ok:
                if (tmfd.traversed)
                {
-                   TupleTableSlot *epqslot;
-
-                   epqslot = EvalPlanQual(epqstate,
-                                          relation,
-                                          relinfo->ri_RangeTableIndex,
-                                          oldslot);
+                   *epqslot = EvalPlanQual(epqstate,
+                                           relation,
+                                           relinfo->ri_RangeTableIndex,
+                                           oldslot);
 
                    /*
                     * If PlanQual failed for updated tuple - we must not
                     * process this tuple!
                     */
-                   if (TupIsNull(epqslot))
+                   if (TupIsNull(*epqslot))
+                   {
+                       *epqslot = NULL;
                        return false;
-
-                   *newSlot = epqslot;
+                   }
                }
                break;
 

diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h
index 203b1ab7dca99c685d70248f1f910704ec0c6076..885b481d9a476d2b604875d47c9d6dcf61cc8167 100644 (file)
--- a/src/include/executor/tuptable.h
+++ b/src/include/executor/tuptable.h
@@ -476,6 +476,7 @@ static inline TupleTableSlot *
 ExecCopySlot(TupleTableSlot *dstslot, TupleTableSlot *srcslot)
 {
    Assert(!TTS_EMPTY(srcslot));
+   AssertArg(srcslot != dstslot);
 
    dstslot->tts_ops->copyslot(dstslot, srcslot);