Re: BUG #16644: null value for defaults in OLD variable for trigger

On Wed, Sep 30, 2020 at 11:21 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> After tracing through it, what I find is that:
>
> 1. Where ExecBRUpdateTriggers fetches the "trigtuple" (line 2695
> of trigger.c, in HEAD) what it gets is a raw copy of the old
> on-disk tuple, with natts = 1. That gets passed to the trigger
> function as OLD, but it reads correctly because it's being decoded
> with the relation's tupdesc, which has the needed info to fill in
> attrmissing columns.
>
> 2. When the trigger does "return OLD", trigtuple is what gets
> passed back, and it gets jammed into the slot that ExecUpdate
> passed to ExecBRUpdateTriggers. *That slot does not have any
> attrmissing info*. Thus, subsequent examination of the slot,
> in particular by ExecConstraints, concludes that the recently-
> added column is NULL.
>
> I've not tried to find where is the difference between 11 and
> 12 that makes it fail or not fail. It's likely some innocent
> seeming aspect of the slot management hacking that Andres
> was doing around that time. It's a bit odd though, because
> AFAICS the slot in question is going to be the result slot
> of the JunkFilter used by ExecModifyTable, and I'd rather have
> expected that that slot would never have had any constraints,
> in any version.

In v11, GetTupleForTrigger() expands the HeapTuple, with this:

/*
* While this is not necessary anymore after 297d627e, as a defense
* against C code that has not recompiled for minor releases after the
* fix, continue to expand the tuple.
*/
if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts)
result = heap_expand_tuple(&tuple, relation->rd_att);
else
result = heap_copytuple(&tuple);
ReleaseBuffer(buffer);

So the tuple that gets slammed into JunkFilter's result slot is an
expanded copy of the on-disk tuple, so it doesn't matter that its
TupleDesc doesn't have constraints (constr) initialized.

In v12 and further, I see that the tuple fetched by
GetTupleForTrigger() is slammed into JunkFilter's result slot
unexpanded and it remains so through ExecConstraint().

> I think we can band-aid this immediate problem by forcing
> trigger.c to materialize the "old" tuples it fetches from disk
> (or whatever needs to be done to substitute missing values into
> them).

Maybe something like the attached?

--
Amit Langote
EDB: http://www.enterprisedb.com