From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Langote <amitlangote09@gmail.com>
Cc: fedor_erastov@mail.ru, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Cc: Andrew Dunstan <andrew@dunslane.net>
Subject: Re: BUG #16644: null value for defaults in OLD variable for trigger
Date: Sun, 25 Oct 2020 16:53:35 -0400
Message-ID: <623211.1603659215@sss.pgh.pa.us> (raw)
In-Reply-To: <568704.1603648770@sss.pgh.pa.us>
References: <16644-5da7ef98a7ac4545@postgresql.org>
<802555.1601432494@sss.pgh.pa.us>
<CA+HiwqH7ys=WmnbGm+wgQwt0WzvQ-R-Qn4vdE01Q_QsX7cyFsw@mail.gmail.com>
<207142.1603468329@sss.pgh.pa.us>
<568704.1603648770@sss.pgh.pa.us>
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
Attachments:use-the-right-descriptor-during-UPDATE-1.patch (text/x-diff, 6.4K) downloaddiff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.cindex 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.cindex 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.cindex 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.hindex 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);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, amitlangote09@gmail.com, pgsql-bugs@lists.postgresql.org, andres@anarazel.de, andrew@dunslane.net
Subject: Re: BUG #16644: null value for defaults in OLD variable for trigger
In-Reply-To: <623211.1603659215@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox