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

Amit Langote <amitlangote09@gmail.com> 于2025年11月6日周四 18:00写道：

> On Fri, Oct 31, 2025 at 10:50 AM David Rowley <dgrowleyml@gmail.com>
> wrote:
> > On Fri, 31 Oct 2025 at 02:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > The definition could have been "throw 'cannot delete from foreign
> > > table' only if the query actually attempts to delete some specific
> > > row from some foreign table", but it is not implemented that way.
> > > Instead the error is thrown during query startup if the query has
> > > a foreign table as a potential delete target.  Thus, as things stand
> > > today, you might or might not get the error depending on whether
> > > the planner can prove that that partition won't be deleted from.
> > > This is not a great user experience, because we don't (and won't)
> > > make any hard promises about how smart the planner is.
> >
> > It's a good point, but I doubt we could change this fact as I expect
> > there are people relying on pruned partitions being excluded from this
> > check. It seems reasonable that someone might want to do something
> > like archive ancient time-based partitioned table partitions into
> > file_fdw stored on a compressed filesystem so that they can still at
> > least query old data should they need to.  If we were to precheck that
> > all partitions support an UPDATE/DELETE, then it could break workloads
> > that do updates on recent data in heap-based partitions. Things would
> > go bad for those people if they switched off partition pruning, but I
> > doubt that would be the only reason as that would also add a huge
> > amount of overhead to their SELECT statements.
> >
> > In any case, the planner is now very efficient at not loading any
> > metadata for pruned partitions, so I don't see how we'd do this
> > without adding possibly large overhead to the planner. I'd say we're
> > well beyond the point of being able to change this now.
>
> I agree that we definitely shouldn’t load metadata for partitions that
> are excluded from the plan, especially not just to slightly improve
> user experience in this corner case.
>
> I looked at a few options, but none seem non-invasive enough for
> back-patching, apart from the first patch I posted. That one makes
> ExecInitModifyTable() not require a ctid to be present to set the root
> partitioned table’s ri_RowIdAttNo, except in the case of MERGE, which
> seems to need it. The corner case that triggers the internal error for
> UPDATE/DELETE doesn’t occur for MERGE now and likely won’t when
> foreign tables eventually gain MERGE support; don't mark my words
> though ;-).
>
> Among those options, I considered the following block, which adds a
> ctid for the partitioned root table when it’s the only target in the
> query after partition pruning removes all child tables due to the
> WHERE false condition in the problematic case:
>
>     /*
>      * Ordinarily, we expect that leaf result relation(s) will have added
> some
>      * ROWID_VAR Vars to the query.  However, it's possible that constraint
>      * exclusion suppressed every leaf relation.  The executor will get
> upset
>      * if the plan has no row identity columns at all, even though it will
>      * certainly process no rows.  Handle this edge case by re-opening the
> top
>      * result relation and adding the row identity columns it would have
> used,
>      * as preprocess_targetlist() would have done if it weren't marked
> "inh".
>      * Then re-run build_base_rel_tlists() to ensure that the added columns
>      * get propagated to the relation's reltarget.  (This is a bit ugly,
> but
>      * it seems better to confine the ugliness and extra cycles to this
>      * unusual corner case.)
>      */
>     if (root->row_identity_vars == NIL)
>     {
>         Relation    target_relation;
>
>         target_relation = table_open(target_rte->relid, NoLock);
>         add_row_identity_columns(root, result_relation,
>                                  target_rte, target_relation);
>         table_close(target_relation, NoLock);
>         build_base_rel_tlists(root, root->processed_tlist);
>         /* There are no ROWID_VAR Vars in this case, so we're done. */
>         return;
>     }
>
> If enable_partition_pruning is off, root->row_identity_vars already
> contains a RowIdentityVarInfo entry for the tableoid Var that was
> added while processing the foreign-table child partition. Because of
> that, the if (root->row_identity_vars == NIL) block doesn’t run in
> this case, so it won’t add any row identity columns such as ctid for
> the partitioned root table.
>
> In theory, we could prevent the planner from adding tableoid in the
> first place when the child table doesn’t support any row identity
> column -- or worse, doesn’t support the UPDATE/DELETE/MERGE command at
> all -- but doing so would require changing the order in which tableoid
> appears in root->processed_tlist. That would be too invasive for a
> back-patch.


Yeah, it seems to need more work if we prevent the planner from adding
tableoid
in the first place.




> So for back branches, I’d propose sticking with the smaller
> executor-side fix and perhaps revisiting the planner behavior
> separately if we ever want to refine handling of pruned partitions or
> dummy roots. I understand, as was reported upthread, that the EXPLAIN
> VERBOSE output isn’t very consistent with that patch even though the
> internal error goes away.  Making sense of the output differences
> requires knowing that the targetlist population behavior differs
> depending on whether enable_partition_pruning is on or off as I
> described above.
>

The executor-side fix works for me and the test case should be added to
your patch.
Should we add some comments to explain the output difference in EXPLAIN
VERBOSE
if enable_partition_pruning is set to a different value?


-- 
Thanks,
Tender Wang