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

On Thu, 30 Oct 2025, 14:18 Amit Langote, <amitlangote09@gmail.com> wrote:

> 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
>


Okay, after putting more thought on it, I think your fix is OK. WFM, LGTM

>