Thread

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

amit <amitlangote09@gmail.com> — 2025-11-06T10:00:33Z

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.

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.

--
Thanks, Amit Langote