Re: BUG #19099: Conditional DELETE from partitioned table with non-updatable partition raises internal error

On Tue, Jan 20, 2026 at 12:07 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Tue, Jan 20, 2026 at 3:25 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Amit Langote <amitlangote09(at)gmail(dot)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 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