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

Amit Langote <amitlangote09(at)gmail(dot)com> writes:
>> To summarize, the two approaches we've thought about:
>>
>> 1. Executor-side fix
>> (v4-0001-Fix-bogus-ctid-requirement-for-dummy-root-partiti.patch
>> posted with my Nov 8 email):
>>
>> Make ExecInitModifyTable() not require ctid when the only result
>> relation is a dummy partitioned root. This is minimally invasive but
>> leaves EXPLAIN VERBOSE output inconsistent depending on
>> enable_partition_pruning -- with pruning off, you see tableoid but no
>> ctid, while with pruning on, you see ctid. That's confusing for users
>> as mentioned upthread.
>>
>> 2. Planner-side fix
>> (v4-0001-Fix-row-identity-handling-for-dummy-partitioned-r.patch
>> posted with my last email):
>>
>> Don't add tableoid for child relations that don't contribute
>> row-identity columns. This keeps root->row_identity_vars empty when
>> there exists only one such child relation, so
>> distribute_row_identity_vars() can add ctid for the dummy root.
>> EXPLAIN output is consistent regardless of pruning setting. (Some may
>> notice in the patch that there's still a minor change, but that's due
>> to how explain.c decides whether to print the table name before the
>> column name, which is unrelated to this.)
>>
>> I'm inclined to go with the second approach. The only back-patching
>> concern is that EXPLAIN VERBOSE output order changes (ctid now appears
>> before tableoid). This is cosmetic -- junk columns are looked up by
>> name, not position -- but could affect tests or tools that parse
>> EXPLAIN output by position.
>>
>> If there are no objections, I'll commit patch #2 next week.

> Tom, do you have any thoughts on the above?

My apologies, I allowed this thread to fall off my radar.

Of these two patches, I greatly prefer the executor-side fix.
I think the planner-side fix is much too invasive to consider
back-patching. Even if it doesn't bother any end users,
it will surely break some extensions' regression tests,
considering how many places it changes in our own tests.
Also, I think the argument about preserving the same generated
tlist is fairly misguided, for two reasons:

1. We've never expected that the set of row-identity columns would
be independent of the set of child tables considered. For example,
different FDWs might produce different sorts of row-ID Vars.

2. EXPLAIN's output for a partitioned query is usually different
between pruning-on and pruning-off. Why's it important that
this tlist detail not be different?

So on the whole, I'd do #1 and call it good. I don't even see an
argument for applying #2 in HEAD only.

regards, tom lane