public inbox for [email protected]
help / color / mirror / Atom feedFrom: Amit Langote <[email protected]>
To: Tom Lane <[email protected]>
Cc: Tender Wang <[email protected]>
Cc: David Rowley <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: jian he <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Cc: PostgreSQL mailing lists <[email protected]>
Subject: Re: BUG #19099: Conditional DELETE from partitioned table with non-updatable partition raises internal error
Date: Wed, 21 Jan 2026 21:58:13 +0900
Message-ID: <CA+HiwqFSH5j1ZBN=6NfdZ8pKAyZ3nV3Z4LJr7-jxxyKwLKsPWw@mail.gmail.com> (raw)
In-Reply-To: <CA+HiwqFr7FjcjwEi1xBiSy_t=F8mL=dz4xJt3+MKumiFiX+uMA@mail.gmail.com>
References: <[email protected]>
<CACJufxF9FcuYe8XOuWLgWK77HCUHpOc6+7+NkktFFNmzw15jKg@mail.gmail.com>
<CAHewXN=vF5d9O4R3+iUwLqEaP7pb8iYAN_e3vEE_p5sJHofn7w@mail.gmail.com>
<[email protected]>
<CALdSSPi7udsgQg3PUG=Z4+-9pRg8wT3HkDvTgYvtg30xNWQ9OA@mail.gmail.com>
<CALdSSPi9n2KGzKQn2Egqz3H8Nx0cgnZ8UeB5gk-KVdE3uBCj6Q@mail.gmail.com>
<CA+HiwqFcejrmS_H8YB-AMB7sujB7wdJXFPdAVfDC6-19FXUjgg@mail.gmail.com>
<CAHewXNmx+UXg46+WUrbPca91bmVipRTpe+SRm19GtxG6mArRhg@mail.gmail.com>
<CALdSSPi6xR1tG2kLvpwNLnAjG9e0wmaY62r2_MF81ZYg5in+qQ@mail.gmail.com>
<[email protected]>
<CAApHDvpYEqJ6h-3NWi_4S19RY9NARpJ3h8CRmWYbz5MJFqE-sg@mail.gmail.com>
<CA+HiwqEHHTG5_TKuNw1M0dCrgUd6SauJ5dcdicz7xozMJip0SA@mail.gmail.com>
<CA+HiwqGk1X-EiVL5kJjHD7V=a3JVDQodt2pwb9SK7q+cYQnpTg@mail.gmail.com>
<CAHewXNnD0744Vykj6ujE5c=rRP=61sh7K154uNdgEaTmJrRegQ@mail.gmail.com>
<CA+HiwqEEX8DsHOtcD4JkWcj4QsyY-BjVhRfc69pUq+7Czqn58w@mail.gmail.com>
<CA+HiwqFbnMWkLZSgFgtbWN2nWYEJ9kWV1VzoFTDfmOZZVc2Mww@mail.gmail.com>
<CA+HiwqEXLHvDfRYNJ02dingMb-np98u0giB+Bdw=qKpCgDNSvg@mail.gmail.com>
<[email protected]>
<CA+HiwqFr7FjcjwEi1xBiSy_t=F8mL=dz4xJt3+MKumiFiX+uMA@mail.gmail.com>
On Tue, Jan 20, 2026 at 12:07 PM Amit Langote <[email protected]> wrote:
> On Tue, Jan 20, 2026 at 3:25 AM Tom Lane <[email protected]> wrote:
> > Amit Langote <[email protected]> writes:
> > >> To summarize, the two approaches we've thought about:
> > >>
> > >> 1. Executor-side fix
> > >> (v4-0001-Fix-bogus-ctid-requirement-for-dummy-root-partiti.patch
> > >> posted with my Nov 8 email):
> > >>
> > >> Make ExecInitModifyTable() not require ctid when the only result
> > >> relation is a dummy partitioned root. This is minimally invasive but
> > >> leaves EXPLAIN VERBOSE output inconsistent depending on
> > >> enable_partition_pruning -- with pruning off, you see tableoid but no
> > >> ctid, while with pruning on, you see ctid. That's confusing for users
> > >> as mentioned upthread.
> > >>
> > >> 2. Planner-side fix
> > >> (v4-0001-Fix-row-identity-handling-for-dummy-partitioned-r.patch
> > >> posted with my last email):
> > >>
> > >> Don't add tableoid for child relations that don't contribute
> > >> row-identity columns. This keeps root->row_identity_vars empty when
> > >> there exists only one such child relation, so
> > >> distribute_row_identity_vars() can add ctid for the dummy root.
> > >> EXPLAIN output is consistent regardless of pruning setting. (Some may
> > >> notice in the patch that there's still a minor change, but that's due
> > >> to how explain.c decides whether to print the table name before the
> > >> column name, which is unrelated to this.)
> > >>
> > >> I'm inclined to go with the second approach. The only back-patching
> > >> concern is that EXPLAIN VERBOSE output order changes (ctid now appears
> > >> before tableoid). This is cosmetic -- junk columns are looked up by
> > >> name, not position -- but could affect tests or tools that parse
> > >> EXPLAIN output by position.
> > >>
> > >> If there are no objections, I'll commit patch #2 next week.
> >
> > > Tom, do you have any thoughts on the above?
> >
> > My apologies, I allowed this thread to fall off my radar.
> >
> > Of these two patches, I greatly prefer the executor-side fix.
> > I think the planner-side fix is much too invasive to consider
> > back-patching. Even if it doesn't bother any end users,
> > it will surely break some extensions' regression tests,
> > considering how many places it changes in our own tests.
>
> Ok, a fair point.
>
> > Also, I think the argument about preserving the same generated
> > tlist is fairly misguided, for two reasons:
> >
> > 1. We've never expected that the set of row-identity columns would
> > be independent of the set of child tables considered. For example,
> > different FDWs might produce different sorts of row-ID Vars.
> >
> > 2. EXPLAIN's output for a partitioned query is usually different
> > between pruning-on and pruning-off. Why's it important that
> > this tlist detail not be different?
>
> Right, the targetlist will look different if a foreign child is pruned
> vs not anyway. I was maybe too focused on this degenerate case where
> all children are excluded -- with pruning off you get tableoid but no
> ctid (because the foreign child was processed before constraint
> exclusion, leaving root->row_identity_vars non-empty triggering the
> block in distribute_row_identity_vars() to add ctid), with pruning on
> you get ctid but no tableoid (because the child was never processed,
> leaving root->row_identity_vars empty).
>
> Because, the degenerate case is a no-op at runtime, maybe we're ok.
>
> > So on the whole, I'd do #1 and call it good. I don't even see an
> > argument for applying #2 in HEAD only.
>
> Ok, I'll post an updated patch for #1 shortly.
Updated patch attached. While reworking it, I realized that
partitioned tables should only appear in the result relations list
when all leaf partitions have been pruned. So instead of checking
nrels > 1, I now Assert(nrels == 1) when we see a partitioned table
and skip the ctid requirement. Also added a corresponding adjustment
in ExecModifyTable() to allow invalid ri_RowIdAttNo for partitioned
tables.
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v5-0001-Fix-bogus-ctid-requirement-for-dummy-root-partiti.patch (5.2K, 2-v5-0001-Fix-bogus-ctid-requirement-for-dummy-root-partiti.patch)
download | inline diff:
From 0c3c3c48c5a815388c402ecbc2b83d0b6fa995ba Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Wed, 21 Jan 2026 21:47:06 +0900
Subject: [PATCH v5] Fix bogus ctid requirement for dummy-root partitioned
targets
ExecInitModifyTable() unconditionally required a ctid junk column even
when the target was a partitioned table. This led to spurious "could
not find junk ctid column" errors when all children were excluded and
only the dummy root result relation remained.
Partitioned tables should only appear in the result relations list when
all leaf partitions have been pruned, leaving only the dummy root. In
this case, no rows can be produced and ctid is not needed. Add an Assert
to verify this invariant and skip the ctid requirement for partitioned
tables. Also adjust ExecModifyTable() to allow invalid ri_RowIdAttNo
for partitioned tables.
Bug: #19099
Reported-by: Alexander Lakhin <[email protected]>
Author: Amit Langote <[email protected]>
Reviewed-by: Tender Wang <[email protected]>
Reviewed-by: Kirill Reshke <[email protected]>
Discussion: https://postgr.es/m/19099-e05dcfa022fe553d%40postgresql.org
Backpatch-through: 14
---
contrib/file_fdw/expected/file_fdw.out | 15 +++++++++++++++
contrib/file_fdw/sql/file_fdw.sql | 18 ++++++++++++++++++
src/backend/executor/nodeModifyTable.c | 19 ++++++++++++++++---
3 files changed, 49 insertions(+), 3 deletions(-)
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 5121e27dce5..51a81ed59a8 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -457,6 +457,21 @@ SELECT tableoid::regclass, * FROM p2;
p2 | 2 | xyzzy
(3 rows)
+-- Test DELETE/UPDATE/MERGE on a partitioned table when all partitions
+-- are excluded and only the dummy root result relation remains. The
+-- operation is a no-op but should not fail regardless of whether the
+-- foreign child was processed (pruning off) or not (pruning on).
+DROP TABLE p2;
+SET enable_partition_pruning TO off;
+DELETE FROM pt WHERE false;
+UPDATE pt SET b = 'x' WHERE false;
+MERGE INTO pt t USING (VALUES (1, 'x'::text)) AS s(a, b)
+ ON false WHEN MATCHED THEN UPDATE SET b = s.b;
+SET enable_partition_pruning TO on;
+DELETE FROM pt WHERE false;
+UPDATE pt SET b = 'x' WHERE false;
+MERGE INTO pt t USING (VALUES (1, 'x'::text)) AS s(a, b)
+ ON false WHEN MATCHED THEN UPDATE SET b = s.b;
DROP TABLE pt;
-- generated column tests
\set filename :abs_srcdir '/data/list1.csv'
diff --git a/contrib/file_fdw/sql/file_fdw.sql b/contrib/file_fdw/sql/file_fdw.sql
index 1a397ad4bd1..4ccdcbb8d06 100644
--- a/contrib/file_fdw/sql/file_fdw.sql
+++ b/contrib/file_fdw/sql/file_fdw.sql
@@ -242,6 +242,24 @@ UPDATE pt set a = 1 where a = 2; -- ERROR
SELECT tableoid::regclass, * FROM pt;
SELECT tableoid::regclass, * FROM p1;
SELECT tableoid::regclass, * FROM p2;
+
+-- Test DELETE/UPDATE/MERGE on a partitioned table when all partitions
+-- are excluded and only the dummy root result relation remains. The
+-- operation is a no-op but should not fail regardless of whether the
+-- foreign child was processed (pruning off) or not (pruning on).
+DROP TABLE p2;
+SET enable_partition_pruning TO off;
+DELETE FROM pt WHERE false;
+UPDATE pt SET b = 'x' WHERE false;
+MERGE INTO pt t USING (VALUES (1, 'x'::text)) AS s(a, b)
+ ON false WHEN MATCHED THEN UPDATE SET b = s.b;
+
+SET enable_partition_pruning TO on;
+DELETE FROM pt WHERE false;
+UPDATE pt SET b = 'x' WHERE false;
+MERGE INTO pt t USING (VALUES (1, 'x'::text)) AS s(a, b)
+ ON false WHEN MATCHED THEN UPDATE SET b = s.b;
+
DROP TABLE pt;
-- generated column tests
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 7d7411a7056..1126f7ed823 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -4369,8 +4369,12 @@ ExecModifyTable(PlanState *pstate)
relkind == RELKIND_MATVIEW ||
relkind == RELKIND_PARTITIONED_TABLE)
{
- /* ri_RowIdAttNo refers to a ctid attribute */
- Assert(AttributeNumberIsValid(resultRelInfo->ri_RowIdAttNo));
+ /*
+ * ri_RowIdAttNo refers to a ctid attribute. See the comment
+ * in ExecInitModifyTable().
+ */
+ Assert(AttributeNumberIsValid(resultRelInfo->ri_RowIdAttNo) ||
+ relkind == RELKIND_PARTITIONED_TABLE);
datum = ExecGetJunkAttribute(slot,
resultRelInfo->ri_RowIdAttNo,
&isNull);
@@ -4883,7 +4887,16 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
{
resultRelInfo->ri_RowIdAttNo =
ExecFindJunkAttributeInTlist(subplan->targetlist, "ctid");
- if (!AttributeNumberIsValid(resultRelInfo->ri_RowIdAttNo))
+
+ /*
+ * For heap relations, a ctid junk attribute must be present.
+ * Partitioned tables should only appear here when all leaf
+ * partitions were pruned, in which case no rows can be
+ * produced and ctid is not needed.
+ */
+ if (relkind == RELKIND_PARTITIONED_TABLE)
+ Assert(nrels == 1);
+ else if (!AttributeNumberIsValid(resultRelInfo->ri_RowIdAttNo))
elog(ERROR, "could not find junk ctid column");
}
else if (relkind == RELKIND_FOREIGN_TABLE)
--
2.47.3
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], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19099: Conditional DELETE from partitioned table with non-updatable partition raises internal error
In-Reply-To: <CA+HiwqFSH5j1ZBN=6NfdZ8pKAyZ3nV3Z4LJr7-jxxyKwLKsPWw@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