MIME-Version: 1.0
References: <19099-e05dcfa022fe553d@postgresql.org> <CACJufxF9FcuYe8XOuWLgWK77HCUHpOc6+7+NkktFFNmzw15jKg@mail.gmail.com>
 <CAHewXN=vF5d9O4R3+iUwLqEaP7pb8iYAN_e3vEE_p5sJHofn7w@mail.gmail.com>
 <2960545.1761800903@sss.pgh.pa.us> <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>
 <3017911.1761832112@sss.pgh.pa.us> <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>
 <1112704.1768847117@sss.pgh.pa.us> <CA+HiwqFr7FjcjwEi1xBiSy_t=F8mL=dz4xJt3+MKumiFiX+uMA@mail.gmail.com>
 <CA+HiwqFSH5j1ZBN=6NfdZ8pKAyZ3nV3Z4LJr7-jxxyKwLKsPWw@mail.gmail.com>
In-Reply-To: <CA+HiwqFSH5j1ZBN=6NfdZ8pKAyZ3nV3Z4LJr7-jxxyKwLKsPWw@mail.gmail.com>
From: Amit Langote <amitlangote09@gmail.com>
Date: Fri, 23 Jan 2026 10:40:41 +0900
Message-ID: <CA+HiwqEcq3echktkintc54eWqpRfh=3u7oTSYMTpT1wdZ=RJ_A@mail.gmail.com>
Subject: Re: BUG #19099: Conditional DELETE from partitioned table with
 non-updatable partition raises internal error
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Tender Wang <tndrwang@gmail.com>, David Rowley <dgrowleyml@gmail.com>, 
	Kirill Reshke <reshkekirill@gmail.com>, jian he <jian.universality@gmail.com>, 
	Alexander Lakhin <exclusion@gmail.com>, 
	PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CA%2BHiwqEcq3echktkintc54eWqpRfh%3D3u7oTSYMTpT1wdZ%3DRJ_A%40mail.gmail.com>
Precedence: bulk

On Wed, Jan 21, 2026 at 9:58=E2=80=AFPM Amit Langote <amitlangote09@gmail.c=
om> wrote:
> On Tue, Jan 20, 2026 at 12:07=E2=80=AFPM Amit Langote <amitlangote09@gmai=
l.com> wrote:
> > On Tue, Jan 20, 2026 at 3:25=E2=80=AFAM Tom Lane <tgl@sss.pgh.pa.us> wr=
ote:
> > > Amit Langote <amitlangote09@gmail.com> 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 b=
ut
> > > >> 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 us=
ers
> > > >> 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 whe=
n
> > > >> 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 th=
e
> > > >> column name, which is unrelated to this.)
> > > >>
> > > >> I'm inclined to go with the second approach. The only back-patchin=
g
> > > >> concern is that EXPLAIN VERBOSE output order changes (ctid now app=
ears
> > > >> before tableoid). This is cosmetic -- junk columns are looked up b=
y
> > > >> 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 =3D=3D 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.

Pushed this now.  Thanks to all.

--=20
Thanks, Amit Langote