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.
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
--
Best regards,
Kirill Reshke
