public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tender Wang <[email protected]>
To: Amit Langote <[email protected]>
Cc: David Rowley <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: jian he <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re: BUG #19099: Conditional DELETE from partitioned table with non-updatable partition raises internal error
Date: Fri, 7 Nov 2025 09:01:45 +0800
Message-ID: <CAHewXN=Y9+ATEKniPX-KRyrkYOTWbFNSu0Yy=HAjXXwwXo6KtA@mail.gmail.com> (raw)
In-Reply-To: <CA+HiwqEHHTG5_TKuNw1M0dCrgUd6SauJ5dcdicz7xozMJip0SA@mail.gmail.com>
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>
Amit Langote <[email protected]> 于2025年11月6日周四 18:00写道：
> On Fri, Oct 31, 2025 at 10:50 AM David Rowley <[email protected]>
> wrote:
> > On Fri, 31 Oct 2025 at 02:48, Tom Lane <[email protected]> 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
view thread (22+ messages) latest in thread
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: <CAHewXN=Y9+ATEKniPX-KRyrkYOTWbFNSu0Yy=HAjXXwwXo6KtA@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