Re: BUG #16644: null value for defaults in OLD variable for trigger - Mailing list pgsql-bugs

I wrote:
> I fixed the should_free business, and spent a fair amount of time
> convincing myself that no other code paths in trigger.c need this,
> and pushed it.

No sooner had I pushed that than I thought of a potentially better
answer: why is it that the executor's slot hasn't got the right
descriptor, anyway?  The reason is that ExecInitModifyTable is relying
on ExecInitJunkFilter, and thence ExecCleanTypeFromTL, to build that
descriptor.  But we have the relation's *actual* descriptor right at
hand, and could use that instead.  This saves a few cycles ---
ExecCleanTypeFromTL isn't enormously expensive, but it's not free
either.  Moreover, it's more correct, even disregarding the problem
at hand, because the tlist isn't a perfectly accurate depiction of
the relation rowtype: ExecCleanTypeFromTL will not derive the correct
info for dropped columns.

We do have to refactor ExecInitJunkFilter a little to make this
possible, but it's not a big change.  (I initially tried to use the
existing ExecInitJunkFilterConversion function, but that does the
wrong thing for attisdropped columns.)  Otherwise, this reverts the
prior patch's code changes in triggers.c, but keeps the test case.

Thoughts?  I'm inclined to leave the previous patch alone in the
back branches, because that has fewer potential side-effects,
but I like this better for HEAD.

            regards, tom lane

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 59289f8d4d..092ac1646d 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -89,8 +89,6 @@ static bool GetTupleForTrigger(EState *estate,
                                LockTupleMode lockmode,
                                TupleTableSlot *oldslot,
                                TupleTableSlot **newSlot);
-static HeapTuple MaterializeTupleForTrigger(TupleTableSlot *slot,
-                                            bool *shouldFree);
 static bool TriggerEnabled(EState *estate, ResultRelInfo *relinfo,
                            Trigger *trigger, TriggerEvent event,
                            Bitmapset *modifiedCols,
@@ -2674,7 +2672,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
                 ExecCopySlot(newslot, epqslot_clean);
         }

-        trigtuple = MaterializeTupleForTrigger(oldslot, &should_free_trig);
+        trigtuple = ExecFetchSlotHeapTuple(oldslot, true, &should_free_trig);
     }
     else
     {
@@ -3043,40 +3041,6 @@ GetTupleForTrigger(EState *estate,
     return true;
 }

-/*
- * Extract a HeapTuple that we can pass off to trigger functions.
- *
- * We must materialize the tuple and make sure it is not dependent on any
- * attrmissing data.  This is needed for the old row in BEFORE UPDATE
- * triggers, since they can choose to pass back this exact tuple as the update
- * result, causing the tuple to be inserted into an executor slot that lacks
- * the attrmissing data.
- *
- * Currently we don't seem to need to remove the attrmissing dependency in any
- * other cases, but keep this as a separate function to simplify fixing things
- * if that changes.
- */
-static HeapTuple
-MaterializeTupleForTrigger(TupleTableSlot *slot, bool *shouldFree)
-{
-    HeapTuple    tup;
-    TupleDesc    tupdesc = slot->tts_tupleDescriptor;
-
-    tup = ExecFetchSlotHeapTuple(slot, true, shouldFree);
-    if (HeapTupleHeaderGetNatts(tup->t_data) < tupdesc->natts &&
-        tupdesc->constr && tupdesc->constr->missing)
-    {
-        HeapTuple    newtup;
-
-        newtup = heap_expand_tuple(tup, tupdesc);
-        if (*shouldFree)
-            heap_freetuple(tup);
-        *shouldFree = true;
-        tup = newtup;
-    }
-    return tup;
-}
-
 /*
  * Is trigger enabled to fire?
  */
diff --git a/src/backend/executor/execJunk.c b/src/backend/executor/execJunk.c
index 40d700dd9e..1a822ff24b 100644
--- a/src/backend/executor/execJunk.c
+++ b/src/backend/executor/execJunk.c
@@ -54,23 +54,48 @@
  *
  * The source targetlist is passed in.  The output tuple descriptor is
  * built from the non-junk tlist entries.
- * An optional resultSlot can be passed as well.
+ * An optional resultSlot can be passed as well; otherwise, we create one.
  */
 JunkFilter *
 ExecInitJunkFilter(List *targetList, TupleTableSlot *slot)
 {
-    JunkFilter *junkfilter;
     TupleDesc    cleanTupType;
-    int            cleanLength;
-    AttrNumber *cleanMap;
-    ListCell   *t;
-    AttrNumber    cleanResno;

     /*
      * Compute the tuple descriptor for the cleaned tuple.
      */
     cleanTupType = ExecCleanTypeFromTL(targetList);

+    /*
+     * The rest is the same as ExecInitJunkFilterInsertion, ie, we want to map
+     * every non-junk targetlist column into the output tuple.
+     */
+    return ExecInitJunkFilterInsertion(targetList, cleanTupType, slot);
+}
+
+/*
+ * ExecInitJunkFilterInsertion
+ *
+ * Initialize a JunkFilter for insertions into a table.
+ *
+ * Here, we are given the target "clean" tuple descriptor rather than
+ * inferring it from the targetlist.  Although the target descriptor can
+ * contain deleted columns, that is not of concern here, since the targetlist
+ * should contain corresponding NULL constants (cf. ExecCheckPlanOutput).
+ * It is assumed that the caller has checked that the table's columns match up
+ * with the non-junk columns of the targetlist.
+ */
+JunkFilter *
+ExecInitJunkFilterInsertion(List *targetList,
+                            TupleDesc cleanTupType,
+                            TupleTableSlot *slot)
+{
+    JunkFilter *junkfilter;
+    int            cleanLength;
+    AttrNumber *cleanMap;
+    ListCell   *t;
+    AttrNumber    cleanResno;
+
     /*
      * Use the given slot, or make a new slot if we weren't given one.
      */
@@ -93,17 +118,18 @@ ExecInitJunkFilter(List *targetList, TupleTableSlot *slot)
     if (cleanLength > 0)
     {
         cleanMap = (AttrNumber *) palloc(cleanLength * sizeof(AttrNumber));
-        cleanResno = 1;
+        cleanResno = 0;
         foreach(t, targetList)
         {
             TargetEntry *tle = lfirst(t);

             if (!tle->resjunk)
             {
-                cleanMap[cleanResno - 1] = tle->resno;
+                cleanMap[cleanResno] = tle->resno;
                 cleanResno++;
             }
         }
+        Assert(cleanResno == cleanLength);
     }
     else
         cleanMap = NULL;
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index a33423c896..29e07b7228 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2591,15 +2591,27 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
                 TupleTableSlot *junkresslot;

                 subplan = mtstate->mt_plans[i]->plan;
-                if (operation == CMD_INSERT || operation == CMD_UPDATE)
-                    ExecCheckPlanOutput(resultRelInfo->ri_RelationDesc,
-                                        subplan->targetlist);

                 junkresslot =
                     ExecInitExtraTupleSlot(estate, NULL,
                                            table_slot_callbacks(resultRelInfo->ri_RelationDesc));
-                j = ExecInitJunkFilter(subplan->targetlist,
-                                       junkresslot);
+
+                /*
+                 * For an INSERT or UPDATE, the result tuple must always match
+                 * the target table's descriptor.  For a DELETE, it won't
+                 * (indeed, there's probably no non-junk output columns).
+                 */
+                if (operation == CMD_INSERT || operation == CMD_UPDATE)
+                {
+                    ExecCheckPlanOutput(resultRelInfo->ri_RelationDesc,
+                                        subplan->targetlist);
+                    j = ExecInitJunkFilterInsertion(subplan->targetlist,
+                                                    RelationGetDescr(resultRelInfo->ri_RelationDesc),
+                                                    junkresslot);
+                }
+                else
+                    j = ExecInitJunkFilter(subplan->targetlist,
+                                           junkresslot);

                 if (operation == CMD_UPDATE || operation == CMD_DELETE)
                 {
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index b7978cd22e..0c48d2a519 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -156,6 +156,9 @@ extern void ResetTupleHashTable(TupleHashTable hashtable);
  */
 extern JunkFilter *ExecInitJunkFilter(List *targetList,
                                       TupleTableSlot *slot);
+extern JunkFilter *ExecInitJunkFilterInsertion(List *targetList,
+                                               TupleDesc cleanTupType,
+                                               TupleTableSlot *slot);
 extern JunkFilter *ExecInitJunkFilterConversion(List *targetList,
                                                 TupleDesc cleanTupType,
                                                 TupleTableSlot *slot);