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

On Mon, Oct 26, 2020 at 5:53 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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.

Thanks for that.

> 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.

Agreed.

> 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.

Hmm, I don't understand. Isn't it the planner's job to make the
targetlist correctly account for dropped columns; what
expand_targetlist() does?

> 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.)

So, with the map generated by ExecInitJunkFilterConversion(), dropped
attributes of the target composite type are effectively mapped to a
NULL datum. That to me seems to be more or less the same thing as
ExecInitJunkFilterInsertion() mapping dropped attributes of the target
table to NULL datums in the source plan's targetlist. What am I
missing?

> 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.

I do agree that this is a better fix for HEAD.

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