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

On Thu, Oct 30, 2025 at 5:55 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> On Thu, 30 Oct 2025 at 13:29, Amit Langote <amitlangote09@gmail.com> wrote:
> >
> > Hi,
> >
> > On Thu, Oct 30, 2025 at 3:44 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> > > On Thu, 30 Oct 2025 at 10:31, I wrote:
> > > >
> > > >  I mean, we I believe we need to execute
> > > > CheckValidResultRel against all partitions in ExecInitModifyTable, at
> > > > least when no partition pruning has been performed
> > > >
> > >
> > > So, the problem is that we managed to exclude all child relations, and
> > > only have a single (dummy) root relation as a result of the
> > > modifyTable plan. Maybe we should populate its target list with
> > > pseudo-junk columns in create_modifytable_plan ?
> > >
> > > For instance, they query does not error-out if we have at least one
> > > another non-file-fdw partition:
> > >
> > > create table p2 partition of pt for values in ( 2) ;
> > >
> > > this is because we have this in create_modifytable_plan
> > >
> > > ```
> > > /* Transfer resname/resjunk labeling, too, to keep executor happy */
> > > apply_tlist_labeling(subplan->targetlist, root->processed_tlist);
> > > ```
> > >
> > > and we successfully found a junk column in the p2 partition.
> > >
> > > The problem is, it works iff root->processed_tlist has at least one
> > > relation which can give us junk columns. Should we add handling for
> > > corner case here?
> > > Another option is to remove this 'Transfer resname/resjunk labeling'
> > > completely and rework planner-executer contracts somehow.
> >
> > I am not really sure if we should play with the planner code.
> >
> > I suspect the real issue is that we’re assuming partitioned tables
> > always need a ctid, which wasn’t true before MERGE started using the
> > root ResultRelInfo. In fact, the old code already looked wrong -- it’s
> > been requiring a ctid even for partitioned tables where that was never
> > necessary. We can fix this by only requiring the junk ctid when we
> > actually operate through the root partitioned table, that is, for
> > MERGE.  Like the attached.
> >
> > --
> > Thanks, Amit Langote
>
> Hi! Thanks for the patch. I can see your points, however I am unsure
> if this is the most right thing to do.
> As per ab5fcf2b04f9 commit message and
> src/backend/optimizer/plan/planner.c comments, I am under impression
> that the postgres-way of fixing that would allow for
> ExecInitModifyTable to operate with a NULL result relation list.

Isn't that what happens with my patch?

> And, in any case, I am still unsure if we should allow the 'DELETE'
> statement from Alexander's repro to successfully execute, which yout
> patch still does

What behavior do you propose in that case? The WHERE false part makes
the plan a dummy ModifyTable on the root partitioned table pt (per
ab5fcf2b0 I guess), and there’s nothing left in the plan that can be
flagged at execution time; the error Alexander reported is a bug we're
trying to fix.  Are you suggesting instead that the attempt to plan
DELETE on the file_fdw partition -- or any foreign table that doesn’t
support DELETE -- should be prevented?

--
Thanks, Amit Langote