Re: [PATCH] Fix null pointer dereference in PG19

On Sun, Jul 5, 2026 at 11:34 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> >> It seems to me that both FOR PORTION OF and INSTEAD OF triggers are SQL
> >> standard features, so this discussion should refer to what the standard
> >> says, and possibly consider what other implementations do (in addition
> >> to discussing what makes sense). Since that hasn't been done yet, maybe
> >> prohibiting this combination for now, as proposed by Aleksander, would
> >> be best.
> >
> > Okay, let's do that. I'll do some research to see what the standard
> > says and whether any other RDBMS offers guidance.
>
> Do we have a suitable patch for that?
>
> The patch proposed at the top of this thread checks for the presence of
> triggers at parse time, which, I think, again has the problem that the
> presence of triggers could change between parse and execution time.

Here are patches for that.

Actually, even though Tom originally objected to checking at
parse-time, I think it is safe. He said:

On Tue, Apr 21, 2026 at 8:24 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Checking this at parse time is completely the wrong thing.
> The view could have gained (or lost) triggers by the time
> it's executed.

But INSTEAD OF triggers are selected in the rewriter, which uses the
same relcache snapshot as parse analysis. And a concurrent change
can't sneak in different triggers, because that causes a relcache
invalidation, so we redo the parse & rewrite phases. I can't find any
way to get a crash. I tried running UPDATE FOR PORTION OF in one
session, adding a trigger in another, and then re-running the UPDATE
in the first session. But the plan is re-analyzed and the check fires.

Here is a copy of Aleksander's patch rebased, with a v2 moving the
check to rewrite-time and a v3 moving the check to execution-time. The
v1 and v2 patches have nearly identical behavior, as far as I can
tell. Perhaps v2 is better since it checks at the same time we find
the INSTEAD OF triggers. (Maybe this could avoid a future TOCTOU
problem?) OTOH v1 gives a slightly nicer error message, since it has
the position info.

There is one difference I could find with the execution-time check: a
zero-row update fails a check at parse-time or rewrite-time, but not
at execution-time. But I think failing is better, isn't it? I've added
tests to capture that difference.

So my preference is to apply v2 (squashed), but not v3. Squashing all
three is okay though if you think not failing on zero rows is better.

--
Paul ~{:-)
pj(at)illuminatedcomputing(dot)com