public inbox for [email protected]
help / color / mirror / Atom feedFrom: Amit Langote <[email protected]>
To: Tom Lane <[email protected]>
Cc: Tender Wang <[email protected]>
Cc: David Rowley <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: jian he <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Cc: PostgreSQL mailing lists <[email protected]>
Subject: Re: BUG #19099: Conditional DELETE from partitioned table with non-updatable partition raises internal error
Date: Tue, 20 Jan 2026 12:07:48 +0900
Message-ID: <CA+HiwqFr7FjcjwEi1xBiSy_t=F8mL=dz4xJt3+MKumiFiX+uMA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CACJufxF9FcuYe8XOuWLgWK77HCUHpOc6+7+NkktFFNmzw15jKg@mail.gmail.com>
<CAHewXN=vF5d9O4R3+iUwLqEaP7pb8iYAN_e3vEE_p5sJHofn7w@mail.gmail.com>
<[email protected]>
<CALdSSPi7udsgQg3PUG=Z4+-9pRg8wT3HkDvTgYvtg30xNWQ9OA@mail.gmail.com>
<CALdSSPi9n2KGzKQn2Egqz3H8Nx0cgnZ8UeB5gk-KVdE3uBCj6Q@mail.gmail.com>
<CA+HiwqFcejrmS_H8YB-AMB7sujB7wdJXFPdAVfDC6-19FXUjgg@mail.gmail.com>
<CAHewXNmx+UXg46+WUrbPca91bmVipRTpe+SRm19GtxG6mArRhg@mail.gmail.com>
<CALdSSPi6xR1tG2kLvpwNLnAjG9e0wmaY62r2_MF81ZYg5in+qQ@mail.gmail.com>
<[email protected]>
<CAApHDvpYEqJ6h-3NWi_4S19RY9NARpJ3h8CRmWYbz5MJFqE-sg@mail.gmail.com>
<CA+HiwqEHHTG5_TKuNw1M0dCrgUd6SauJ5dcdicz7xozMJip0SA@mail.gmail.com>
<CA+HiwqGk1X-EiVL5kJjHD7V=a3JVDQodt2pwb9SK7q+cYQnpTg@mail.gmail.com>
<CAHewXNnD0744Vykj6ujE5c=rRP=61sh7K154uNdgEaTmJrRegQ@mail.gmail.com>
<CA+HiwqEEX8DsHOtcD4JkWcj4QsyY-BjVhRfc69pUq+7Czqn58w@mail.gmail.com>
<CA+HiwqFbnMWkLZSgFgtbWN2nWYEJ9kWV1VzoFTDfmOZZVc2Mww@mail.gmail.com>
<CA+HiwqEXLHvDfRYNJ02dingMb-np98u0giB+Bdw=qKpCgDNSvg@mail.gmail.com>
<[email protected]>
On Tue, Jan 20, 2026 at 3:25 AM Tom Lane <[email protected]> wrote:
> Amit Langote <[email protected]> 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.
Ok, a fair point.
> 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?
Right, the targetlist will look different if a foreign child is pruned
vs not anyway. I was maybe too focused on this degenerate case where
all children are excluded -- with pruning off you get tableoid but no
ctid (because the foreign child was processed before constraint
exclusion, leaving root->row_identity_vars non-empty triggering the
block in distribute_row_identity_vars() to add ctid), with pruning on
you get ctid but no tableoid (because the child was never processed,
leaving root->row_identity_vars empty).
Because, the degenerate case is a no-op at runtime, maybe we're ok.
> 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.
Ok, I'll post an updated patch for #1 shortly.
Thanks a lot for chiming in.
--
Thanks, Amit Langote
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19099: Conditional DELETE from partitioned table with non-updatable partition raises internal error
In-Reply-To: <CA+HiwqFr7FjcjwEi1xBiSy_t=F8mL=dz4xJt3+MKumiFiX+uMA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox