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

Hi,

On Sun, Nov 30, 2025 at 3:00 PM Tender Wang <tndrwang(at)gmail(dot)com> wrote:
> Amit Langote <amitlangote09(at)gmail(dot)com> 于2025年11月26日周三 19:27写道：
>> On Thu, Nov 6, 2025 at 7:00 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>> > 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.
>>
>> I’ve implemented this alternative as well -- the version that prevents
>> adding tableoid when no other row-identity columns are added for the
>> child. That allows to keep root->row_identity_vars empty so the
>> dummy-root path can add ctid as intended by the above code block of
>> distribute_row_identity_vars().
>>
>> This provides an alternative approach to compare against the other patch.
>>
>> --
>> Thanks, Amit Langote
>
>
> I apply the patch, and I find it forgets to update the diff for postgres_fdw.
> So I add it in the v2 patch.

Oops.

> With this patch, the targetlists are identical whether or not enable_partition_pruning is on.
>
> In my first email on this thread, to avoid adding "tableoid", I tried to add the following codes:
> "(childrte->relkind != RELKIND_PARTITIONED_TABLE && childrte->relkind != RELKIND_FOREIGN_TABLE)"
> in expand_single_inheritance_child().
>
> But this didn't work for all test cases. It would trigger an assert failure in fix_scan_expr_walker():
> Assert(!(IsA(node, Var) && ((Var *) node)->varno == ROWID_VAR));
>
> Your patch is much better than mine.

Thanks for taking a look.

Attached is an updated version with improved comments and simplified test cases.

Regarding back-patch safety (to v14 where the bug was introduced):

* EXPLAIN VERBOSE output order changes (ctid now appears before tableoid)
* AddForeignUpdateTargets is no longer called when the FDW doesn't
support the command

Does anyone see issues with back-patching these changes?

--
Thanks, Amit Langote