public inbox for [email protected]
help / color / mirror / Atom feedFrom: Paul A Jungwirth <[email protected]>
To: Peter Eisentraut <[email protected]>
Cc: Aleksander Alekseev <[email protected]>
Cc: PostgreSQL Development <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: [PATCH] Fix null pointer dereference in PG19
Date: Tue, 7 Jul 2026 18:45:46 -0700
Message-ID: <CA+renyXPEJYOVov3XtzTp3NusGkObc9UeW-Nm78K8_nsWSnnfg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAJ7c6TME+ix6VRf-2TPnVTsj8qn_hy6sYAOmMhZEivwsu2wS6g@mail.gmail.com>
<[email protected]>
<CACJufxFgKVL=NnPocAwhRFqpLHckN3N1Ndf7FjGNTdtsAA26mg@mail.gmail.com>
<CA+renyW52FJ=+pnh+N_xqjUev_MNUCk_mgD2UC7UT0jyAHCE8Q@mail.gmail.com>
<[email protected]>
<CAJ7c6TO+kTwk5MBayLARxsPk-YCsDt2QH9P_PeTYoc3zPYCS3Q@mail.gmail.com>
<CA+renyU6UZnpkgAuog0BVhqyD=G+_UVLzN2Ccpt2sV-qkF-_VQ@mail.gmail.com>
<CAJ7c6TN7dekEHU8Vu4jV5+Zq7h2cVZQgwkYFCrU0jrjT26-ANw@mail.gmail.com>
<CA+renyXs5mkPb-Vg=uF2Hj0jP2ufutc1_Eht=eb3+b_WpjiqLg@mail.gmail.com>
<[email protected]>
<CA+renyUkKaWtWZX2UcojKVjM=f1F5kgdvR8Pr5QqyJqqe_dw-g@mail.gmail.com>
<[email protected]>
On Sun, Jul 5, 2026 at 11:34 PM Peter Eisentraut <[email protected]> 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 <[email protected]> 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 ~{:-)
[email protected]
Attachments:
[text/x-patch] v7-0003-Check-FOR-PORTION-OF-against-INSTEAD-OF-triggers-.patch (5.2K, ../CA+renyXPEJYOVov3XtzTp3NusGkObc9UeW-Nm78K8_nsWSnnfg@mail.gmail.com/2-v7-0003-Check-FOR-PORTION-OF-against-INSTEAD-OF-triggers-.patch)
download | inline diff:
From 1eb176eeb4c8519c260f4d1430d1673e374da8bf Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <[email protected]>
Date: Tue, 7 Jul 2026 17:23:01 -0700
Subject: [PATCH v7 3/3] Check FOR PORTION OF against INSTEAD OF triggers at
execution time
Doing the test at execution time means that zero-row updates don't fail.
Discussion: https://www.postgresql.org/message-id/CAJ7c6TME%2Bix6VRf-2TPnVTsj8qn_hy6sYAOmMhZEivwsu2wS6g%40mail.gmail.com
---
src/backend/executor/nodeModifyTable.c | 13 +++++++++++++
src/backend/rewrite/rewriteHandler.c | 8 --------
src/test/regress/expected/updatable_views.out | 10 ++++------
src/test/regress/sql/updatable_views.sql | 8 ++++----
4 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index c333d7139fa..440f535dd04 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1879,6 +1879,13 @@ ExecDelete(ModifyTableContext *context,
bool dodelete;
Assert(oldtuple != NULL);
+
+ /* We don't support FOR PORTION OF on views with INSTEAD OF triggers. */
+ if (((ModifyTable *) context->mtstate->ps.plan)->forPortionOf)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("views with INSTEAD OF triggers do not support FOR PORTION OF")));
+
dodelete = ExecIRDeleteTriggers(estate, resultRelInfo, oldtuple);
if (!dodelete) /* "do nothing" */
@@ -2772,6 +2779,12 @@ ExecUpdate(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_update_instead_row)
{
+ /* We don't support FOR PORTION OF on views with INSTEAD OF triggers. */
+ if (((ModifyTable *) context->mtstate->ps.plan)->forPortionOf)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("views with INSTEAD OF triggers do not support FOR PORTION OF")));
+
if (!ExecIRUpdateTriggers(estate, resultRelInfo,
oldtuple, slot))
return NULL; /* "do nothing" */
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 38f54b57eec..e7ae9cce65f 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -4171,14 +4171,6 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length,
*/
rt_entry_relation = relation_open(rt_entry->relid, NoLock);
- /* We don't support FOR PORTION OF on views with INSTEAD OF triggers. */
- if (parsetree->forPortionOf &&
- rt_entry_relation->rd_rel->relkind == RELKIND_VIEW &&
- view_has_instead_trigger(rt_entry_relation, event, NIL))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("views with INSTEAD OF triggers do not support FOR PORTION OF")));
-
/*
* Rewrite the targetlist as needed for the command type.
*/
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index 93af96b7ddd..d20676d1ef4 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -4183,15 +4183,13 @@ delete from uv_fpo_instead_view
for portion of valid_at from '2017-01-01' to '2022-01-01'
where id = '[1,1]'; -- error
ERROR: views with INSTEAD OF triggers do not support FOR PORTION OF
--- The check does not depend on which rows match, so it errors even when
--- no rows do.
+-- Here the check runs per row in the executor, so a statement that matches
+-- no rows never reaches it and is allowed.
update uv_fpo_instead_view
for portion of valid_at from '2015-01-01' to '2020-01-01'
- set b = 99 where id = '[9,9]'; -- error, even with no matching rows
-ERROR: views with INSTEAD OF triggers do not support FOR PORTION OF
+ set b = 99 where id = '[9,9]'; -- ok, no rows affected
delete from uv_fpo_instead_view
for portion of valid_at from '2017-01-01' to '2022-01-01'
- where id = '[9,9]'; -- error, even with no matching rows
-ERROR: views with INSTEAD OF triggers do not support FOR PORTION OF
+ where id = '[9,9]'; -- ok, no rows affected
drop view uv_fpo_instead_view;
drop function uv_fpo_instead_trig();
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
index f4a7e0327b0..0a7b3d17ec2 100644
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -2170,15 +2170,15 @@ delete from uv_fpo_instead_view
for portion of valid_at from '2017-01-01' to '2022-01-01'
where id = '[1,1]'; -- error
--- The check does not depend on which rows match, so it errors even when
--- no rows do.
+-- Here the check runs per row in the executor, so a statement that matches
+-- no rows never reaches it and is allowed.
update uv_fpo_instead_view
for portion of valid_at from '2015-01-01' to '2020-01-01'
- set b = 99 where id = '[9,9]'; -- error, even with no matching rows
+ set b = 99 where id = '[9,9]'; -- ok, no rows affected
delete from uv_fpo_instead_view
for portion of valid_at from '2017-01-01' to '2022-01-01'
- where id = '[9,9]'; -- error, even with no matching rows
+ where id = '[9,9]'; -- ok, no rows affected
drop view uv_fpo_instead_view;
drop function uv_fpo_instead_trig();
--
2.47.3
[text/x-patch] v7-0001-Forbid-FOR-PORTION-OF-on-views-with-INSTEAD-OF-tr.patch (5.6K, ../CA+renyXPEJYOVov3XtzTp3NusGkObc9UeW-Nm78K8_nsWSnnfg@mail.gmail.com/3-v7-0001-Forbid-FOR-PORTION-OF-on-views-with-INSTEAD-OF-tr.patch)
download | inline diff:
From b8765f75ec8084680937c3f0b437e1e19a2cda96 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <[email protected]>
Date: Tue, 7 Jul 2026 17:20:05 -0700
Subject: [PATCH v7 1/3] Forbid FOR PORTION OF on views with INSTEAD OF
triggers
Previously an attempt to use these features together caused a crash.
Oversight of commit 8e72d914c528.
Author: Aleksander Alekseev <[email protected]>
Discussion: https://www.postgresql.org/message-id/CAJ7c6TME%2Bix6VRf-2TPnVTsj8qn_hy6sYAOmMhZEivwsu2wS6g%40mail.gmail.com
---
src/backend/parser/analyze.c | 11 ++++++
src/test/regress/expected/updatable_views.out | 38 +++++++++++++++++++
src/test/regress/sql/updatable_views.sql | 34 +++++++++++++++++
3 files changed, 83 insertions(+)
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 2932d17a107..1e6fbc95964 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1344,6 +1344,17 @@ transformForPortionOfClause(ParseState *pstate,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("WHERE CURRENT OF with FOR PORTION OF is not implemented"));
+ /* We don't support FOR PORTION OF on views with INSTEAD OF triggers. */
+ if (targetrel->rd_rel->relkind == RELKIND_VIEW &&
+ targetrel->rd_rel->relhastriggers &&
+ targetrel->trigdesc != NULL &&
+ (isUpdate ? targetrel->trigdesc->trig_update_instead_row
+ : targetrel->trigdesc->trig_delete_instead_row))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("views with INSTEAD OF triggers do not support FOR PORTION OF"),
+ parser_errposition(pstate, forPortionOf->location)));
+
result = makeNode(ForPortionOfExpr);
/* Look up the FOR PORTION OF name requested. */
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index 7b00c742776..acab165ae4c 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -4165,3 +4165,41 @@ select * from base_tab order by a;
drop view base_tab_view;
drop table base_tab;
+-- FOR PORTION OF is not supported on views with INSTEAD OF triggers
+create view uv_fpo_instead_view as select id, valid_at, b from uv_fpo_tab;
+create function uv_fpo_instead_trig() returns trigger language plpgsql as
+$$ begin return null; end $$;
+create trigger uv_fpo_instead_upd_trig
+ instead of update on uv_fpo_instead_view
+ for each row execute function uv_fpo_instead_trig();
+create trigger uv_fpo_instead_del_trig
+ instead of delete on uv_fpo_instead_view
+ for each row execute function uv_fpo_instead_trig();
+update uv_fpo_instead_view
+ for portion of valid_at from '2015-01-01' to '2020-01-01'
+ set b = 99 where id = '[1,1]'; -- error
+ERROR: views with INSTEAD OF triggers do not support FOR PORTION OF
+LINE 2: for portion of valid_at from '2015-01-01' to '2020-01-01'
+ ^
+delete from uv_fpo_instead_view
+ for portion of valid_at from '2017-01-01' to '2022-01-01'
+ where id = '[1,1]'; -- error
+ERROR: views with INSTEAD OF triggers do not support FOR PORTION OF
+LINE 2: for portion of valid_at from '2017-01-01' to '2022-01-01'
+ ^
+-- The check does not depend on which rows match, so it errors even when
+-- no rows do.
+update uv_fpo_instead_view
+ for portion of valid_at from '2015-01-01' to '2020-01-01'
+ set b = 99 where id = '[9,9]'; -- error, even with no matching rows
+ERROR: views with INSTEAD OF triggers do not support FOR PORTION OF
+LINE 2: for portion of valid_at from '2015-01-01' to '2020-01-01'
+ ^
+delete from uv_fpo_instead_view
+ for portion of valid_at from '2017-01-01' to '2022-01-01'
+ where id = '[9,9]'; -- error, even with no matching rows
+ERROR: views with INSTEAD OF triggers do not support FOR PORTION OF
+LINE 2: for portion of valid_at from '2017-01-01' to '2022-01-01'
+ ^
+drop view uv_fpo_instead_view;
+drop function uv_fpo_instead_trig();
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
index 4a60126ec90..f4a7e0327b0 100644
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -2148,3 +2148,37 @@ values (1, 2, default, 5, 4, default, 3), (10, 11, 'C value', 14, 13, 100, 12);
select * from base_tab order by a;
drop view base_tab_view;
drop table base_tab;
+
+-- FOR PORTION OF is not supported on views with INSTEAD OF triggers
+create view uv_fpo_instead_view as select id, valid_at, b from uv_fpo_tab;
+
+create function uv_fpo_instead_trig() returns trigger language plpgsql as
+$$ begin return null; end $$;
+
+create trigger uv_fpo_instead_upd_trig
+ instead of update on uv_fpo_instead_view
+ for each row execute function uv_fpo_instead_trig();
+create trigger uv_fpo_instead_del_trig
+ instead of delete on uv_fpo_instead_view
+ for each row execute function uv_fpo_instead_trig();
+
+update uv_fpo_instead_view
+ for portion of valid_at from '2015-01-01' to '2020-01-01'
+ set b = 99 where id = '[1,1]'; -- error
+
+delete from uv_fpo_instead_view
+ for portion of valid_at from '2017-01-01' to '2022-01-01'
+ where id = '[1,1]'; -- error
+
+-- The check does not depend on which rows match, so it errors even when
+-- no rows do.
+update uv_fpo_instead_view
+ for portion of valid_at from '2015-01-01' to '2020-01-01'
+ set b = 99 where id = '[9,9]'; -- error, even with no matching rows
+
+delete from uv_fpo_instead_view
+ for portion of valid_at from '2017-01-01' to '2022-01-01'
+ where id = '[9,9]'; -- error, even with no matching rows
+
+drop view uv_fpo_instead_view;
+drop function uv_fpo_instead_trig();
--
2.47.3
[text/x-patch] v7-0002-Check-FOR-PORTION-OF-against-INSTEAD-OF-triggers-.patch (4.2K, ../CA+renyXPEJYOVov3XtzTp3NusGkObc9UeW-Nm78K8_nsWSnnfg@mail.gmail.com/4-v7-0002-Check-FOR-PORTION-OF-against-INSTEAD-OF-triggers-.patch)
download | inline diff:
From 8defe6de6f69c842ade84b9d6ac7596cf4a21283 Mon Sep 17 00:00:00 2001
From: "Paul A. Jungwirth" <[email protected]>
Date: Tue, 7 Jul 2026 17:21:34 -0700
Subject: [PATCH v7 2/3] Check FOR PORTION OF against INSTEAD OF triggers
during rewriting
Move the INSTEAD OF trigger check to the rewriter. This loses the error
position but gives the check better locality with where we look up the
triggers.
Discussion: https://www.postgresql.org/message-id/CAJ7c6TME%2Bix6VRf-2TPnVTsj8qn_hy6sYAOmMhZEivwsu2wS6g%40mail.gmail.com
---
src/backend/parser/analyze.c | 11 -----------
src/backend/rewrite/rewriteHandler.c | 8 ++++++++
src/test/regress/expected/updatable_views.out | 8 --------
3 files changed, 8 insertions(+), 19 deletions(-)
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 1e6fbc95964..2932d17a107 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1344,17 +1344,6 @@ transformForPortionOfClause(ParseState *pstate,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("WHERE CURRENT OF with FOR PORTION OF is not implemented"));
- /* We don't support FOR PORTION OF on views with INSTEAD OF triggers. */
- if (targetrel->rd_rel->relkind == RELKIND_VIEW &&
- targetrel->rd_rel->relhastriggers &&
- targetrel->trigdesc != NULL &&
- (isUpdate ? targetrel->trigdesc->trig_update_instead_row
- : targetrel->trigdesc->trig_delete_instead_row))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("views with INSTEAD OF triggers do not support FOR PORTION OF"),
- parser_errposition(pstate, forPortionOf->location)));
-
result = makeNode(ForPortionOfExpr);
/* Look up the FOR PORTION OF name requested. */
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index e7ae9cce65f..38f54b57eec 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -4171,6 +4171,14 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length,
*/
rt_entry_relation = relation_open(rt_entry->relid, NoLock);
+ /* We don't support FOR PORTION OF on views with INSTEAD OF triggers. */
+ if (parsetree->forPortionOf &&
+ rt_entry_relation->rd_rel->relkind == RELKIND_VIEW &&
+ view_has_instead_trigger(rt_entry_relation, event, NIL))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("views with INSTEAD OF triggers do not support FOR PORTION OF")));
+
/*
* Rewrite the targetlist as needed for the command type.
*/
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index acab165ae4c..93af96b7ddd 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -4179,27 +4179,19 @@ update uv_fpo_instead_view
for portion of valid_at from '2015-01-01' to '2020-01-01'
set b = 99 where id = '[1,1]'; -- error
ERROR: views with INSTEAD OF triggers do not support FOR PORTION OF
-LINE 2: for portion of valid_at from '2015-01-01' to '2020-01-01'
- ^
delete from uv_fpo_instead_view
for portion of valid_at from '2017-01-01' to '2022-01-01'
where id = '[1,1]'; -- error
ERROR: views with INSTEAD OF triggers do not support FOR PORTION OF
-LINE 2: for portion of valid_at from '2017-01-01' to '2022-01-01'
- ^
-- The check does not depend on which rows match, so it errors even when
-- no rows do.
update uv_fpo_instead_view
for portion of valid_at from '2015-01-01' to '2020-01-01'
set b = 99 where id = '[9,9]'; -- error, even with no matching rows
ERROR: views with INSTEAD OF triggers do not support FOR PORTION OF
-LINE 2: for portion of valid_at from '2015-01-01' to '2020-01-01'
- ^
delete from uv_fpo_instead_view
for portion of valid_at from '2017-01-01' to '2022-01-01'
where id = '[9,9]'; -- error, even with no matching rows
ERROR: views with INSTEAD OF triggers do not support FOR PORTION OF
-LINE 2: for portion of valid_at from '2017-01-01' to '2022-01-01'
- ^
drop view uv_fpo_instead_view;
drop function uv_fpo_instead_trig();
--
2.47.3
view thread (13+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Fix null pointer dereference in PG19
In-Reply-To: <CA+renyXPEJYOVov3XtzTp3NusGkObc9UeW-Nm78K8_nsWSnnfg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox