Horton
Menu

Thread

  1. [PATCH] Fix null pointer dereference in PG19

    Aleksander Alekseev <aleksander@tigerdata.com> — 2026-04-21T13:40:49Z

    Hi,

I discovered that it's possible to crash Postgres when using VIEWS,
FOR PORTION OF syntax and INSTEAD OF triggers together. See crash.sql.

This happens because in ExecModifyTable() around line 4827 there is no
check for `relkind == RELKIND_VIEW`. If this is the case `tupleid`
ends up being NULL which causes null pointer dereference later when
ExecDeleteEpilogue() or ExecUpdateEpilogue() calls
ExecForPortionOfLeftovers() with tupleid = NULL. An example stacktrace
is attached.

I propose fixing this by explicitly forbidding using the named
features together. See the patch.


-- 
Best regards,
Aleksander Alekseev

  2. Re: [PATCH] Fix null pointer dereference in PG19

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-04-21T15:24:50Z

    Aleksander Alekseev <aleksander@tigerdata.com> writes:
> I propose fixing this by explicitly forbidding using the named
> features together. See the patch.

Checking this at parse time is completely the wrong thing.
The view could have gained (or lost) triggers by the time
it's executed.

Actually, looking at it, transformForPortionOfClause is quite
full of premature error checks:

* I'd tend to move the anti-FDW check to execution too.
It's not actively wrong, since nowadays we don't permit
relations to change relkind, but it seems out of place.
Also, it seems inadequate to deal with the case of a target
that is a partitioned table having FDW partitions.

* contain_volatile_functions_after_planning is utterly wrong
to apply here.  That should happen somewhere in the planner,
where it'd be cheaper as well as not premature.

* I'm inclined to think that pretty much all the mucking with
opclasses is misplaced too: those structures are not immutable
either.  All of that should move to rewriting, planning, or
even executor startup.

			regards, tom lane

  3. Re: [PATCH] Fix null pointer dereference in PG19

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-21T23:12:45Z

    On Tue, Apr 21, 2026 at 8:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Aleksander Alekseev <aleksander@tigerdata.com> writes:
> > I propose fixing this by explicitly forbidding using the named
> > features together. See the patch.
>
> Checking this at parse time is completely the wrong thing.
> The view could have gained (or lost) triggers by the time
> it's executed.
>
> Actually, looking at it, transformForPortionOfClause is quite
> full of premature error checks:
>
> * I'd tend to move the anti-FDW check to execution too.
> It's not actively wrong, since nowadays we don't permit
> relations to change relkind, but it seems out of place.
> Also, it seems inadequate to deal with the case of a target
> that is a partitioned table having FDW partitions.
>
> * contain_volatile_functions_after_planning is utterly wrong
> to apply here.  That should happen somewhere in the planner,
> where it'd be cheaper as well as not premature.
>
> * I'm inclined to think that pretty much all the mucking with
> opclasses is misplaced too: those structures are not immutable
> either.  All of that should move to rewriting, planning, or
> even executor startup.

Thanks for this feedback! I will work on a patch to move those checks
to different phases, and also guard against FDW partitions.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  4. Re: [PATCH] Fix null pointer dereference in PG19

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-04-21T23:28:40Z

    Paul A Jungwirth <pj@illuminatedcomputing.com> writes:
> On Tue, Apr 21, 2026 at 8:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * I'm inclined to think that pretty much all the mucking with
>> opclasses is misplaced too: those structures are not immutable
>> either.  All of that should move to rewriting, planning, or
>> even executor startup.

> Thanks for this feedback! I will work on a patch to move those checks
> to different phases, and also guard against FDW partitions.

Actually, on second thought that part might be okay.  It's not so
different from things we do elsewhere in the parser, such as pick a
suitable equality operator for an IN clause.  Even though you could
argue that those decisions ought to be postponed till we're ready to
execute the statement, it's not so bad because what the construct
actually ends up depending on is a specific operator or function.
Even if the user later changes the operator class that we used to find
that function, its semantics are presumably still fit for purpose,
or close enough.

I definitely don't like checking volatility at parse time, though.

(BTW, to be clear: the situation of concern here is where we parse a
query and put it into a rule or new-style SQL function or the like.
By the time we get to executing that parse tree, much might have
changed.  Anything we expect to still be there had better be recorded
as a dependency of the parsetree.  Even then, the dependency only
guarantees existence of the object, not that its properties didn't
change.)

			regards, tom lane

  5. Re: [PATCH] Fix null pointer dereference in PG19

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-22T19:42:41Z

    On Tue, Apr 21, 2026 at 4:11 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> On Tue, Apr 21, 2026 at 6:41 AM Aleksander Alekseev
> <aleksander@tigerdata.com> wrote:
> >
> > I discovered that it's possible to crash Postgres when using VIEWS,
> > FOR PORTION OF syntax and INSTEAD OF triggers together. See crash.sql.
> >
> > This happens because in ExecModifyTable() around line 4827 there is no
> > check for `relkind == RELKIND_VIEW`. If this is the case `tupleid`
> > ends up being NULL which causes null pointer dereference later when
> > ExecDeleteEpilogue() or ExecUpdateEpilogue() calls
> > ExecForPortionOfLeftovers() with tupleid = NULL. An example stacktrace
> > is attached.
>
> Thanks for testing FOR PORTION OF! This specific bug has been reported
> already and has a patch at [1].
>
> [1] https://www.postgresql.org/message-id/CAHg%2BQDd74fnd4obCRMqVS0AVWf%3DcSFH%3DCv7trTJWgm%2B_bhTK6w%40mail.gmail.com
>
> > I propose fixing this by explicitly forbidding using the named
> > features together. See the patch.
>
> I don't think disabling these features is necessary. You are right
> that we can't use the tupleid when we have a view, but I think an
> INSTEAD OF trigger should cause us to skip inserting temporal
> leftovers. (If we didn't do the update, we can't draw conclusions
> about what history was touched vs untouched.)

(Quoting my full message yesterday since I forgot to Reply-All.)

Here is v4 of the fix for this. I'd like to continue the conversation
here since that other thread is huge and attached to the original
commitfest entry for the feature. I'll make a new CF entry for just
this thread.

This patch squashes jian he's test enhancements from his v3 patch.
Also I changed '[2024-01-01,2024-12-31)' to '[2024-01-01,2025-01-01)',
which look less like a mistake (not that it matters to the test), and
I cleaned up the test comment a little.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  6. Re: [PATCH] Fix null pointer dereference in PG19

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-04-22T20:02:29Z

    On Wed, Apr 22, 2026 at 12:42 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
>
> Here is v4 of the fix for this. I'd like to continue the conversation
> here since that other thread is huge and attached to the original
> commitfest entry for the feature. I'll make a new CF entry for just
> this thread.
>
> This patch squashes jian he's test enhancements from his v3 patch.
> Also I changed '[2024-01-01,2024-12-31)' to '[2024-01-01,2025-01-01)',
> which look less like a mistake (not that it matters to the test), and
> I cleaned up the test comment a little.

Here is a v5, documenting that we skip all FOR PORTION OF work in case
of an INSTEAD OF trigger.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  7. Re: [PATCH] Fix null pointer dereference in PG19

    jian he <jian.universality@gmail.com> — 2026-04-24T02:59:49Z

    On Tue, Apr 21, 2026 at 11:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> * contain_volatile_functions_after_planning is utterly wrong
> to apply here.  That should happen somewhere in the planner,
> where it'd be cheaper as well as not premature.
>

typedef struct ForPortionOfExpr
{
    NodeTag        type;
    Var           *rangeVar;        /* Range column */
    char       *range_name;        /* Range name */
    Node       *targetFrom;        /* FOR PORTION OF FROM bound, if given */
    Node       *targetTo;        /* FOR PORTION OF TO bound, if given */
    Node       *targetRange;    /* FOR PORTION OF bounds as a
range/multirange */
    Oid            rangeType;        /* (base)type of targetRange */
    bool        isDomain;        /* Is rangeVar a domain? */
    Node       *overlapsExpr;    /* range && targetRange */
    List       *rangeTargetList;    /* List of TargetEntrys to set the time
                                     * column(s) */
    Oid            withoutPortionProc; /* SRF proc for old_range -
target_range */
    ParseLoc    location;        /* token location, or -1 if unknown */
    ParseLoc    targetLocation; /* token location, or -1 if unknown */
} ForPortionOfExpr;

targetFrom and targetTo is only for deparsing purpose, skip
eval_const_expressions should be fine.

RewriteQuery, we have:
``````
AddQual(parsetree, parsetree->forPortionOf->overlapsExpr);
/* Update FOR PORTION OF column(s) automatically. */
foreach(tl, parsetree->forPortionOf->rangeTargetList)
{
    TargetEntry *tle = (TargetEntry *) lfirst(tl);
    parsetree->targetList = lappend(parsetree->targetList, tle);
}
``````
rangeTargetList and overlapsExpr will go through eval_const_expressions.
Only ForPortionOfExpr->targetRange really needs to be dealt with.

In ExecInitModifyTable, we already did
ExecPrepareExpr(forPortionOf->targetRange),
which will do eval_const_expressions(forPortionOf->targetRange).

moving contain_volatile_functions_after_planning to
ExecInitModifyTable should be fine.

In ExecInitModifyTable, we can't just
```
exprState = ExecPrepareExpr((Expr *) forPortionOf->targetRange, estate);
if (contain_volatile_functions_after_planning(exprState->expr)
```

Because of the comments below in execnodes.h:

typedef struct ExprState
    /* original expression tree, for debugging only */
    Expr       *expr;

While at it, add errcode to the surrounding code.



--
jian
https://www.enterprisedb.com/

  8. Re: [PATCH] Fix null pointer dereference in PG19

    jian he <jian.universality@gmail.com> — 2026-04-24T04:44:35Z

    On Tue, Apr 21, 2026 at 11:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> * I'd tend to move the anti-FDW check to execution too.
> It's not actively wrong, since nowadays we don't permit
> relations to change relkind, but it seems out of place.
> Also, it seems inadequate to deal with the case of a target
> that is a partitioned table having FDW partitions.
>
Hi.

Instead of adding another subnode in CheckValidResultRel,
I am passing ModifyTable to it, this will be more future-proof.



--
jian
https://www.enterprisedb.com/

  9. Re: [PATCH] Fix null pointer dereference in PG19

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-05-15T21:21:48Z

    On Thu, Apr 23, 2026 at 9:45 PM jian he <jian.universality@gmail.com> wrote:
>
> On Tue, Apr 21, 2026 at 11:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > * I'd tend to move the anti-FDW check to execution too.
> > It's not actively wrong, since nowadays we don't permit
> > relations to change relkind, but it seems out of place.
> > Also, it seems inadequate to deal with the case of a target
> > that is a partitioned table having FDW partitions.
> >
> Hi.
>
> Instead of adding another subnode in CheckValidResultRel,
> I am passing ModifyTable to it, this will be more future-proof.

Thank you for working on this! I started a new thread at [1] so that I
could give it a commitfest entry.

Your patch looks great to me. I didn't notice that you had posted one,
so I made my own, but yours is better. Doing the check in
CheckValidResultRel makes a lot of sense.

I thought there was one other case we should test for: when a
partition has a child FDW that gets pruned, we should not raise an
error. So I swapped in my own tests, which were otherwise similar to
yours.

That new thread also includes a patch to move the functionality check
into plan-time.

[1] https://www.postgresql.org/message-id/CA%2BrenyUte0_UJsJiDJQi82oaBsMJn%3Dcct0Wn%3DvOqXtuDn%3DYYJA%40mail.gmail.com

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  10. Re: [PATCH] Fix null pointer dereference in PG19

    Peter Eisentraut <peter@eisentraut.org> — 2026-06-24T08:52:24Z

    On 15.05.26 23:21, Paul A Jungwirth wrote:
> On Thu, Apr 23, 2026 at 9:45 PM jian he <jian.universality@gmail.com> wrote:
>>
>> On Tue, Apr 21, 2026 at 11:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>
>>> * I'd tend to move the anti-FDW check to execution too.
>>> It's not actively wrong, since nowadays we don't permit
>>> relations to change relkind, but it seems out of place.
>>> Also, it seems inadequate to deal with the case of a target
>>> that is a partitioned table having FDW partitions.
>>>
>> Hi.
>>
>> Instead of adding another subnode in CheckValidResultRel,
>> I am passing ModifyTable to it, this will be more future-proof.
> 
> Thank you for working on this! I started a new thread at [1] so that I
> could give it a commitfest entry.
> 
> Your patch looks great to me. I didn't notice that you had posted one,
> so I made my own, but yours is better. Doing the check in
> CheckValidResultRel makes a lot of sense.
> 
> I thought there was one other case we should test for: when a
> partition has a child FDW that gets pruned, we should not raise an
> error. So I swapped in my own tests, which were otherwise similar to
> yours.
> 
> That new thread also includes a patch to move the functionality check
> into plan-time.
> 
> [1] https://www.postgresql.org/message-id/CA%2BrenyUte0_UJsJiDJQi82oaBsMJn%3Dcct0Wn%3DvOqXtuDn%3DYYJA%40mail.gmail.com

I don't understand the status of this discussion.  The original bug 
report is related to views and INSTEAD OF triggers.  But the last few 
patches that have been posted here appear to be unrelated to that 
specific issue.

  11. Re: [PATCH] Fix null pointer dereference in PG19

    Aleksander Alekseev <aleksander@tigerdata.com> — 2026-06-24T11:12:59Z

    Hi Peter,

Thanks for your attention to this.

> I don't understand the status of this discussion.  The original bug
> report is related to views and INSTEAD OF triggers.  But the last few
> patches that have been posted here appear to be unrelated to that
> specific issue.

That makes two of us. My humble understanding is that there are
several related discussions but they are happening elsewhere now. Feel
free correcting me if this is not the case.

Here I propose to focus on a particular crash and the particular
proposed bugfix, v1-0001. Perhaps it should be rewritten or maybe we
should reject it in favor of another patch. Both are possible
outcomes. Right now I'm only interested in fixing a particular crash.

> I definitely don't like checking volatility at parse time, though.

So.... the last comment from Tom was that he is not happy :) but I'm
not sure about the actionable items. Tom, could you please elaborate a
bit?

-- 
Best regards,
Aleksander Alekseev

  12. Re: [PATCH] Fix null pointer dereference in PG19

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-06-24T16:02:21Z

    On Wed, Jun 24, 2026 at 4:13 AM Aleksander Alekseev
<aleksander@tigerdata.com> wrote:
>
> Hi Peter,
>
> Thanks for your attention to this.
>
> > I don't understand the status of this discussion.  The original bug
> > report is related to views and INSTEAD OF triggers.  But the last few
> > patches that have been posted here appear to be unrelated to that
> > specific issue.
>
> That makes two of us. My humble understanding is that there are
> several related discussions but they are happening elsewhere now. Feel
> free correcting me if this is not the case.

I agree it is confusing. A lot of things were originally reported on
unrelated threads. I've been making separate threads for each
issue+patch and trying to steer discussion there. I'd like to keep
this thread limited to the original issue reported by Aleksander: that
we crash trying to insert temporal leftovers on a view with an INSTEAD
OF trigger.

One thing confusing about this bug is that it was reported twice. The
first time was here:
https://www.postgresql.org/message-id/CAHg+QDd74fnd4obCRMqVS0AVWf=cSFH=Cv7trTJWgm+_bhTK6w@mail.gmail.com
But since that was on the original development thread, I thought this
thread was a better place to continue discussion.

The Open Items page should point to the right thread for each issue.
But I see that for this issue, it points to the original bug report,
so you have to click through a lot of messages to wind up in the right
place. I'll update the link to come here.

> Here I propose to focus on a particular crash and the particular
> proposed bugfix, v1-0001. Perhaps it should be rewritten or maybe we
> should reject it in favor of another patch. Both are possible
> outcomes. Right now I'm only interested in fixing a particular crash.

I don't think forbidding INSTEAD OF triggers with FOR PORTION OF is
the right solution. It seems too heavy-handed. But we *should* skip
trying to insert temporal leftovers. After all if you did something
*instead of* the update/delete, we can't know what the leftovers
should be. Or another way of putting it: the trigger function runs
instead of the original command, but inserting leftovers is part of
that original command. Skipping temporal leftovers is implemented in
my v5 patch on this thread from April 22:
https://www.postgresql.org/message-id/CA%2BrenyUoTRnn0o4Pnfy2AOdtqMH3%2Bn29_AfD4Aih3ifwMX9vyA%40mail.gmail.com

> > I definitely don't like checking volatility at parse time, though.
>
> So.... the last comment from Tom was that he is not happy :) but I'm
> not sure about the actionable items. Tom, could you please elaborate a
> bit?

Here is the thread for moving checks out of analysis:
https://www.postgresql.org/message-id/CA%2BrenyUte0_UJsJiDJQi82oaBsMJn%3Dcct0Wn%3DvOqXtuDn%3DYYJA%40mail.gmail.com

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  13. Re: [PATCH] Fix null pointer dereference in PG19

    Aleksander Alekseev <aleksander@tigerdata.com> — 2026-06-25T10:32:43Z

    Hi Paul,

> > > I definitely don't like checking volatility at parse time, though.
> >
> > So.... the last comment from Tom was that he is not happy :) but I'm
> > not sure about the actionable items. Tom, could you please elaborate a
> > bit?
>
> Here is the thread for moving checks out of analysis:
> https://www.postgresql.org/message-id/CA%2BrenyUte0_UJsJiDJQi82oaBsMJn%3Dcct0Wn%3DvOqXtuDn%3DYYJA%40mail.gmail.com

Thanks for the clarification! So if my understanding is correct this
is out of scope of the fix under question.

> I don't think forbidding INSTEAD OF triggers with FOR PORTION OF is
> the right solution. It seems too heavy-handed. But we *should* skip
> trying to insert temporal leftovers. After all if you did something
> *instead of* the update/delete, we can't know what the leftovers
> should be. Or another way of putting it: the trigger function runs
> instead of the original command, but inserting leftovers is part of
> that original command. Skipping temporal leftovers is implemented in
> my v5 patch on this thread from April 22:
> https://www.postgresql.org/message-id/CA%2BrenyUoTRnn0o4Pnfy2AOdtqMH3%2Bn29_AfD4Aih3ifwMX9vyA%40mail.gmail.com

This patch rotted and needed a rebase (attached as .txt). Also it's
incorrect, it only masks the crash:

```
CREATE TABLE t (id int, valid_at daterange, val int);
INSERT INTO t VALUES (1, '[2024-01-01,2025-01-01)', 100);
CREATE VIEW v AS SELECT * FROM t;

CREATE FUNCTION trig_fn() RETURNS trigger LANGUAGE plpgsql AS $$
BEGIN
    RAISE NOTICE 'UPDATE OLD: %, NEW: %', OLD, NEW;
    RETURN NEW;
END;
$$;

CREATE TRIGGER trig INSTEAD OF UPDATE ON v FOR EACH ROW EXECUTE
FUNCTION trig_fn();
UPDATE v FOR PORTION OF valid_at FROM '2024-04-01' TO '2024-08-01' SET
val = 999 WHERE id = 1;

server closed the connection unexpectedly
```

v1-0001 passes the tests successfully:

```
=# UPDATE v FOR PORTION OF valid_at FROM '2024-04-01' TO '2024-08-01'
SET val = 999 WHERE id = 1;
ERROR:  views with INSTEAD OF triggers do not support FOR PORTION OF
LINE 1: UPDATE v FOR PORTION OF valid_at FROM '2024-04-01' TO '2024-...
```

IMO it's a little bit late in the PG19 cycle for making this work.
This is an implementation of a new feature which is not present at the
moment which to my knowledge we don't do after the feature freeze. Our
goal is to fix the crash and leave the rest for the PG20 cycle.
Clearly the feature needs more discussion and thorough testing.

--
Best regards,
Aleksander Alekseev

  14. Re: [PATCH] Fix null pointer dereference in PG19

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-06-29T07:38:34Z

    On Thu, Jun 25, 2026 at 3:32 AM Aleksander Alekseev
<aleksander@tigerdata.com> wrote:
>
> This patch rotted and needed a rebase (attached as .txt).

Thank you for taking care of the rebase!

> Also it's
> incorrect, it only masks the crash:
>
> ```
> CREATE TABLE t (id int, valid_at daterange, val int);
> INSERT INTO t VALUES (1, '[2024-01-01,2025-01-01)', 100);
> CREATE VIEW v AS SELECT * FROM t;
>
> CREATE FUNCTION trig_fn() RETURNS trigger LANGUAGE plpgsql AS $$
> BEGIN
>     RAISE NOTICE 'UPDATE OLD: %, NEW: %', OLD, NEW;
>     RETURN NEW;
> END;
> $$;
>
> CREATE TRIGGER trig INSTEAD OF UPDATE ON v FOR EACH ROW EXECUTE
> FUNCTION trig_fn();
> UPDATE v FOR PORTION OF valid_at FROM '2024-04-01' TO '2024-08-01' SET
> val = 999 WHERE id = 1;
>
> server closed the connection unexpectedly
> ```

You're right, this was due to a copy/paste bug (looking for a delete
trigger in the update code). By splitting the test into two triggers,
we catch the problem and correctly skip the leftovers. I've attached a
patch with that correction.

> IMO it's a little bit late in the PG19 cycle for making this work.
> This is an implementation of a new feature which is not present at the
> moment which to my knowledge we don't do after the feature freeze. Our
> goal is to fix the crash and leave the rest for the PG20 cycle.
> Clearly the feature needs more discussion and thorough testing.

I don't really think this is a new feature. It is a fix to make FOR
PORTION OF not execute when it shouldn't. The change here is quite
simple.

But I don't mind holding it back if that's what people want to do.
Looking more closely at INSTEAD OF triggers, I found another bug: the
FOR PORTION OF qual (and TLE) were not added, so the trigger would
fire on more rows than it should, and NEW.valid_at was not
pre-computed. The second patch here fixes that. I'll defer to others
whether we should fix the INSTEAD OF interaction now or wait 'til v20.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

  15. Re: [PATCH] Fix null pointer dereference in PG19

    Peter Eisentraut <peter@eisentraut.org> — 2026-07-02T07:25:33Z

    On 29.06.26 09:38, Paul A Jungwirth wrote:
>> IMO it's a little bit late in the PG19 cycle for making this work.
>> This is an implementation of a new feature which is not present at the
>> moment which to my knowledge we don't do after the feature freeze. Our
>> goal is to fix the crash and leave the rest for the PG20 cycle.
>> Clearly the feature needs more discussion and thorough testing.
> I don't really think this is a new feature. It is a fix to make FOR
> PORTION OF not execute when it shouldn't. The change here is quite
> simple.
> 
> But I don't mind holding it back if that's what people want to do.
> Looking more closely at INSTEAD OF triggers, I found another bug: the
> FOR PORTION OF qual (and TLE) were not added, so the trigger would
> fire on more rows than it should, and NEW.valid_at was not
> pre-computed. The second patch here fixes that. I'll defer to others
> whether we should fix the INSTEAD OF interaction now or wait 'til v20.

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.

  16. Re: [PATCH] Fix null pointer dereference in PG19

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-07-02T14:29:03Z

    On Thu, Jul 2, 2026 at 12:25 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> > But I don't mind holding it back if that's what people want to do.
> > Looking more closely at INSTEAD OF triggers, I found another bug: the
> > FOR PORTION OF qual (and TLE) were not added, so the trigger would
> > fire on more rows than it should, and NEW.valid_at was not
> > pre-computed. The second patch here fixes that. I'll defer to others
> > whether we should fix the INSTEAD OF interaction now or wait 'til v20.
>
> 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.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

  17. Re: [PATCH] Fix null pointer dereference in PG19

    Peter Eisentraut <peter@eisentraut.org> — 2026-07-06T06:34:27Z

    On 02.07.26 16:29, Paul A Jungwirth wrote:
> On Thu, Jul 2, 2026 at 12:25 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>>> But I don't mind holding it back if that's what people want to do.
>>> Looking more closely at INSTEAD OF triggers, I found another bug: the
>>> FOR PORTION OF qual (and TLE) were not added, so the trigger would
>>> fire on more rows than it should, and NEW.valid_at was not
>>> pre-computed. The second patch here fixes that. I'll defer to others
>>> whether we should fix the INSTEAD OF interaction now or wait 'til v20.
>>
>> 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.

  18. Re: [PATCH] Fix null pointer dereference in PG19

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-07-08T01:45:46Z

    On Sun, Jul 5, 2026 at 11:34 PM Peter Eisentraut <peter@eisentraut.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@sss.pgh.pa.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@illuminatedcomputing.com

  19. Re: [PATCH] Fix null pointer dereference in PG19

    Tom Lane <tgl@sss.pgh.pa.us> — 2026-07-08T02:07:37Z

    Paul A Jungwirth <pj@illuminatedcomputing.com> writes:
> On Tue, Apr 21, 2026 at 8:24 AM Tom Lane <tgl@sss.pgh.pa.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.

You have forgotten about views and rewrite rules.  Those go to disk in
post-parser form, and will be rewritten only at execution sometime
later, *without* a re-parse.

			regards, tom lane

  20. Re: [PATCH] Fix null pointer dereference in PG19

    Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-07-08T16:48:08Z

    On Tue, Jul 7, 2026 at 7:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Paul A Jungwirth <pj@illuminatedcomputing.com> writes:
> > On Tue, Apr 21, 2026 at 8:24 AM Tom Lane <tgl@sss.pgh.pa.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.
>
> You have forgotten about views and rewrite rules.  Those go to disk in
> post-parser form, and will be rewritten only at execution sometime
> later, *without* a re-parse.

Ah yes, thank you! I should have been able to work that out.

Here is another patch series. No code changes, but I inserted a new
patch with tests showing that parse-time checking crashes, but
rewrite-time and exec-time checking catches the forbidden statement.
So the series is:

v1: parse-time check, tests pass
v2: add tests that crash the server
v3: rewrite-time check: tests pass
v4: exec-time check: tests pass

These are against REL_19_STABLE, not master (but I don't think it
makes a difference).

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com