Re: [PATCH] Fix null pointer dereference in PG19
Re: [PATCH] Fix null pointer dereference in PG19
От:
Tom Lane <tgl@sss.pgh.pa.us>
Дата:
Paul A Jungwirth writes: > On Tue, Apr 21, 2026 at 8:24 AM Tom Lane 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
Re: [PATCH] Fix null pointer dereference in PG19
От:
Peter Eisentraut <peter@eisentraut.org>
Дата:
On 09.07.26 17:22, Paul A Jungwirth wrote: > Checking at v1 is Aleksander's original patch. Then v2 adds tests that > crash the server. v3 moves the check to rewrite time, and the tests > pass. v4 moves the check again to exec time. But v4 is a behavior > change: an UPDATE that touches zero rows no longer fails. I thought > that was weird; that's why I recommended leaving it out (in > https://www.postgresql.org/message-id/ > CA%2BrenyXPEJYOVov3XtzTp3NusGkObc9UeW-Nm78K8_nsWSnnfg%40mail.gmail.com > before I added the crashing tests). I committed patches 0001 through 0003 (squashed). I think I was mistaken about the need for an executor check (patch 0004). But we can add it on if a need appears.
Re: [PATCH] Fix null pointer dereference in PG19
От:
Peter Eisentraut <peter@eisentraut.org>
Дата:
On 02.07.26 16:29, Paul A Jungwirth wrote: > On Thu, Jul 2, 2026 at 12:25 AM Peter Eisentraut 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.
Re: [PATCH] Fix null pointer dereference in PG19
От:
Tom Lane <tgl@sss.pgh.pa.us>
Дата:
Aleksander Alekseev 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
Re: [PATCH] Fix null pointer dereference in PG19
От:
Peter Eisentraut <peter@eisentraut.org>
Дата:
On 08.07.26 18:48, Paul A Jungwirth wrote: > On Tue, Jul 7, 2026 at 7:07 PM Tom Lane wrote: >> >> Paul A Jungwirth writes: >>> On Tue, Apr 21, 2026 at 8:24 AM Tom Lane 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 What do you want to do with these? Here you list them as four different versions, but the attachments are four incremental patches of the same version. Do you propose to apply all of them? It seems we definitely do want the exec-time check. And I can see maybe the parse-time check as an additional user-friendliness feature. But maybe we don't need to check three times?
Re: [PATCH] Fix null pointer dereference in PG19
От:
Peter Eisentraut <peter@eisentraut.org>
Дата:
On 15.05.26 23:21, Paul A Jungwirth wrote: > On Thu, Apr 23, 2026 at 9:45 PM jian he wrote: >> >> On Tue, Apr 21, 2026 at 11:24 PM Tom Lane 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.
Re: [PATCH] Fix null pointer dereference in PG19
От:
Tom Lane <tgl@sss.pgh.pa.us>
Дата:
Paul A Jungwirth writes: > On Tue, Apr 21, 2026 at 8:24 AM Tom Lane 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
Re: [PATCH] Fix null pointer dereference in PG19
От:
Paul A Jungwirth <pj@illuminatedcomputing.com>
Дата:
On Wed, Jun 24, 2026 at 4:13 AM Aleksander Alekseev
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
Re: [PATCH] Fix null pointer dereference in PG19
От:
Paul A Jungwirth <pj@illuminatedcomputing.com>
Дата:
On Thu, Jul 2, 2026 at 12:25 AM Peter Eisentraut 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
Re: [PATCH] Fix null pointer dereference in PG19
От:
Paul A Jungwirth <pj@illuminatedcomputing.com>
Дата:
On Wed, Apr 22, 2026 at 12:42 PM Paul A Jungwirth
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
Re: [PATCH] Fix null pointer dereference in PG19
От:
Paul A Jungwirth <pj@illuminatedcomputing.com>
Дата:
On Tue, Jul 7, 2026 at 7:07 PM Tom Lane wrote:
>
> Paul A Jungwirth writes:
> > On Tue, Apr 21, 2026 at 8:24 AM Tom Lane 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
Re: [PATCH] Fix null pointer dereference in PG19
От:
Paul A Jungwirth <pj@illuminatedcomputing.com>
Дата:
On Tue, Apr 21, 2026 at 4:11 PM Paul A Jungwirth
wrote:
>
> On Tue, Apr 21, 2026 at 6:41 AM Aleksander Alekseev
> 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
Re: [PATCH] Fix null pointer dereference in PG19
От:
Paul A Jungwirth <pj@illuminatedcomputing.com>
Дата:
On Tue, Apr 21, 2026 at 8:24 AM Tom Lane wrote:
>
> Aleksander Alekseev 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
Re: [PATCH] Fix null pointer dereference in PG19
От:
Paul A Jungwirth <pj@illuminatedcomputing.com>
Дата:
On Thu, Jul 9, 2026 at 1:32 AM Peter Eisentraut wrote:
>
> > 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
>
> What do you want to do with these? Here you list them as four different
> versions, but the attachments are four incremental patches of the same
> version. Do you propose to apply all of them?
>
> It seems we definitely do want the exec-time check. And I can see maybe
> the parse-time check as an additional user-friendliness feature. But
> maybe we don't need to check three times?
I should have called them part{1-4} instead of v{1-4}. My intent is to
squash them, but I thought separating the changes clarified the
problem and how v3/v4 fix it. Either squash v1-3 if you want to check
at rewrite time, or v1-4 if you want to check at exec time.
Each part removes the check from the prior part, so there is only one
check, it's just a question of when we do it.
Checking at v1 is Aleksander's original patch. Then v2 adds tests that
crash the server. v3 moves the check to rewrite time, and the tests
pass. v4 moves the check again to exec time. But v4 is a behavior
change: an UPDATE that touches zero rows no longer fails. I thought
that was weird; that's why I recommended leaving it out (in
https://www.postgresql.org/message-id/CA%2BrenyXPEJYOVov3XtzTp3NusGkObc9UeW-Nm78K8_nsWSnnfg%40mail.gmail.com
before I added the crashing tests).
I like your idea of keeping the parse-time check too. Then we can show
line numbers. Want a patch with that?
If we check at both rewrite-time and exec-time, the exec-time check
will never happen (as far as I can tell). If we want something there,
maybe it should be an Assert or an elog.
I can give you one squashed file if you like. After reading your
email, my preference is to check at parse time, check again at rewrite
time, and Assert at exec time.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Re: [PATCH] Fix null pointer dereference in PG19
От:
Paul A Jungwirth <pj@illuminatedcomputing.com>
Дата:
On Sun, Jul 5, 2026 at 11:34 PM Peter Eisentraut 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 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
Re: [PATCH] Fix null pointer dereference in PG19
От:
jian he <jian.universality@gmail.com>
Дата:
On Tue, Apr 21, 2026 at 11:24 PM Tom Lane 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/
Re: [PATCH] Fix null pointer dereference in PG19
От:
jian he <jian.universality@gmail.com>
Дата:
On Tue, Apr 21, 2026 at 11:24 PM Tom Lane 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/
[PATCH] Fix null pointer dereference in PG19
От:
Aleksander Alekseev <aleksander@tigerdata.com>
Дата:
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
Re: [PATCH] Fix null pointer dereference in PG19
От:
Aleksander Alekseev <aleksander@tigerdata.com>
Дата:
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
Re: [PATCH] Fix null pointer dereference in PG19
От:
Aleksander Alekseev <aleksander@tigerdata.com>
Дата:
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
