Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1vHAri-00GA0r-JW for pgsql-bugs@arkaria.postgresql.org; Fri, 07 Nov 2025 01:02:10 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1vHArf-00BAVZ-T4 for pgsql-bugs@arkaria.postgresql.org; Fri, 07 Nov 2025 01:02:07 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1vHArf-00BAVR-CZ for pgsql-bugs@lists.postgresql.org; Fri, 07 Nov 2025 01:02:07 +0000 Received: from mail-ed1-x52a.google.com ([2a00:1450:4864:20::52a]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1vHArd-005oz6-1G for pgsql-bugs@lists.postgresql.org; Fri, 07 Nov 2025 01:02:06 +0000 Received: by mail-ed1-x52a.google.com with SMTP id 4fb4d7f45d1cf-63b9da57cecso405301a12.0 for ; Thu, 06 Nov 2025 17:02:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1762477319; x=1763082119; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=GR3mp+p5vuoXPkjoYYhTQ81atHqf2nyoHKaXNMfUQRQ=; b=d4boHBTuX5URQXr9GLgf8BTU+L5KBYUFt0ORIc8X5RLjFRrTv4S8n/XaAddQ/4Odhl QRQkRn100isi53uty+kpeXzWgYdHTYxc3SzlwWZ+mkgwARdKDCdlNSeecXCjJchTfjHz nh0iY5ExMoaOoGa53vhqHiLnQ7umYQNkx1LvyOlAcv2IEd2vTLZRPStrjIRI78lzZbJY jlgu3ih+TzrIl60ZqyxADlRHD2ZckoYyo3uEIinzWVWnvk13JSrf64010BWZrmUnvk/J sU/d+AELB4yPUV7a+m7iCyoJ5tl1049vyZ655+aJZmFYg+I92W69znJPywQox4/qrrmF XFZg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1762477319; x=1763082119; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=GR3mp+p5vuoXPkjoYYhTQ81atHqf2nyoHKaXNMfUQRQ=; b=ctUzic7qwSP5ei3stCrvtr8PyulnzVvIIWj/qi2IxNy79zcXN1ge4oZkX5Ds1AaVWi IiHvSthmDDA2ZZF7WnDVxojc6d6deBUW5u2/QLbGIzYrkHSMYmmU++i8xLYIR9iKtCZO 8mVx+70tMry8AT9Du5bMifRCZk4p7Jnk0F2IqOBpNP470AjP9SmJegEmprIH2B8LVck3 peWspogqNnD002F/6vgLMWe2DMqJVJjdfLN1HX2Ntq785jtkD0FClW+3de17+/UYA1cT pTgz+5afcAxd7E1DFfCakOP6qvBDOtsEZO95jMJbn5vQHXAgsyVH7HXkgIYffzsknnu0 IGOQ== X-Forwarded-Encrypted: i=1; AJvYcCWZk/K4U2RqZ8T1iUNLycDE8Q/VtZ+KsaDfszsS2UMKrZ1Ds0tshqwUc8ld9yXFEEw5D8O/XyQH51uf@lists.postgresql.org X-Gm-Message-State: AOJu0Yztvp1CR41DUEwcIfdolcM63FY3ucD9UiWOIO0QBaSA4G/Rxsvq AOoNOqKiTY3j+I4qYpqWqxCAl930/9LXpOuaVWeDBX4tWR1SaJpX4npxnte4f9FOIoPOXMlFpEI 1zyJXIOlWjyn372R0EdeubgybKtsqZZQ= X-Gm-Gg: ASbGncsU2C40CQIYIIt1hCkXUGMrumzwYLtEKWG2WYX9wnGwzbZI0YjgnRvZOFp7g43 n6Lj0k/MUnO5QnJf9F7IYrAl41HC0LvblDyTV83mxaTRR+Eu6mFJyPIuAq5gqmjUe3pAuKMVcwT 7VOFqhQzgJ83hGgNicCbo8ZvIqgLdORrQzydwu417PqazNRcMmZoYC2rVcszpMuEUzWn08ilo8I PAJ4dZJ1G9ddpV0shlTMZhzQ642m/SfGF1Vgz1J/XiBOoU23i9U/HRj1geJSYmByN3ifVIOsA== X-Google-Smtp-Source: AGHT+IHtbZfCbivcxKZL9I/ddAc+Sksfz6kp/PrDEebyVkd1cwGdYWaeML7ROOFE7zbTWU+M1wckyd2huoGi+0BpLMs= X-Received: by 2002:a17:907:da3:b0:b6d:4df9:68bc with SMTP id a640c23a62f3a-b72c0933162mr129974066b.1.1762477318647; Thu, 06 Nov 2025 17:01:58 -0800 (PST) MIME-Version: 1.0 References: <19099-e05dcfa022fe553d@postgresql.org> <2960545.1761800903@sss.pgh.pa.us> <3017911.1761832112@sss.pgh.pa.us> In-Reply-To: From: Tender Wang Date: Fri, 7 Nov 2025 09:01:45 +0800 X-Gm-Features: AWmQ_bmpUk8XGYiEqDrrGV3Wte1ySifj4Ntmwi4ybeDx14wFx-N_mIo7W1tESjA Message-ID: Subject: Re: BUG #19099: Conditional DELETE from partitioned table with non-updatable partition raises internal error To: Amit Langote Cc: David Rowley , Tom Lane , Kirill Reshke , jian he , exclusion@gmail.com, pgsql-bugs@lists.postgresql.org Content-Type: multipart/alternative; boundary="00000000000080b4250642f6babc" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --00000000000080b4250642f6babc Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Amit Langote =E4=BA=8E2025=E5=B9=B411=E6=9C=886= =E6=97=A5=E5=91=A8=E5=9B=9B 18:00=E5=86=99=E9=81=93=EF=BC=9A > On Fri, Oct 31, 2025 at 10:50=E2=80=AFAM David Rowley > wrote: > > On Fri, 31 Oct 2025 at 02:48, Tom Lane 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=E2=80=99t 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=E2=80=99s ri_RowIdAttNo, except in the case of MERGE, w= hich > seems to need it. The corner case that triggers the internal error for > UPDATE/DELETE doesn=E2=80=99t occur for MERGE now and likely won=E2=80=99= 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=E2=80=99s 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 constrai= nt > * exclusion suppressed every leaf relation. The executor will get > upset > * if the plan has no row identity columns at all, even though it wil= l > * certainly process no rows. Handle this edge case by re-opening th= e > 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 colum= ns > * 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 =3D=3D NIL) > { > Relation target_relation; > > target_relation =3D 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 =3D=3D NIL) block doesn=E2=80=99t r= un in > this case, so it won=E2=80=99t 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=E2=80=99t support any row identity > column -- or worse, doesn=E2=80=99t support the UPDATE/DELETE/MERGE comma= nd 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=E2=80=99d 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=E2=80=99t 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? --=20 Thanks, Tender Wang --00000000000080b4250642f6babc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


Amit Langote &l= t;amitlangote09@gmail.com>= ; =E4=BA=8E2025=E5=B9=B411=E6=9C=886=E6=97=A5=E5=91=A8=E5=9B=9B 18:00=E5=86= =99=E9=81=93=EF=BC=9A
On Fri, Oct 31, 2025 at 10:50=E2=80=AFAM 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 fro= m foreign
> > table' only if the query actually attempts to delete some spe= cific
> > 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.=C2=A0 Thus, as thin= gs stand
> > today, you might or might not get the error depending on whether<= br> > > the planner can prove that that partition won't be deleted fr= om.
> > This is not a great user experience, because we don't (and wo= n'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 expe= ct
> 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.=C2=A0 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<= br> > go bad for those people if they switched off partition pruning, but I<= br> > 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 thi= s
> without adding possibly large overhead to the planner. I'd say we&= #39;re
> well beyond the point of being able to change this now.

I agree that we definitely shouldn=E2=80=99t load metadata for partitions t= hat
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=E2=80=99s ri_RowIdAttNo, except in the case of MERGE, whi= ch
seems to need it. The corner case that triggers the internal error for
UPDATE/DELETE doesn=E2=80=99t occur for MERGE now and likely won=E2=80=99t = 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=E2=80=99s the only target in th= e
query after partition pruning removes all child tables due to the
WHERE false condition in the problematic case:

=C2=A0 =C2=A0 /*
=C2=A0 =C2=A0 =C2=A0* Ordinarily, we expect that leaf result relation(s) wi= ll have added some
=C2=A0 =C2=A0 =C2=A0* ROWID_VAR Vars to the query.=C2=A0 However, it's = possible that constraint
=C2=A0 =C2=A0 =C2=A0* exclusion suppressed every leaf relation.=C2=A0 The e= xecutor will get upset
=C2=A0 =C2=A0 =C2=A0* if the plan has no row identity columns at all, even = though it will
=C2=A0 =C2=A0 =C2=A0* certainly process no rows.=C2=A0 Handle this edge cas= e by re-opening the top
=C2=A0 =C2=A0 =C2=A0* result relation and adding the row identity columns i= t would have used,
=C2=A0 =C2=A0 =C2=A0* as preprocess_targetlist() would have done if it were= n't marked "inh".
=C2=A0 =C2=A0 =C2=A0* Then re-run build_base_rel_tlists() to ensure that th= e added columns
=C2=A0 =C2=A0 =C2=A0* get propagated to the relation's reltarget.=C2=A0= (This is a bit ugly, but
=C2=A0 =C2=A0 =C2=A0* it seems better to confine the ugliness and extra cyc= les to this
=C2=A0 =C2=A0 =C2=A0* unusual corner case.)
=C2=A0 =C2=A0 =C2=A0*/
=C2=A0 =C2=A0 if (root->row_identity_vars =3D=3D NIL)
=C2=A0 =C2=A0 {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 Relation=C2=A0 =C2=A0 target_relation;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 target_relation =3D table_open(target_rte->r= elid, NoLock);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 add_row_identity_columns(root, result_relation,=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0target_rte, target_relation);<= br> =C2=A0 =C2=A0 =C2=A0 =C2=A0 table_close(target_relation, NoLock);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 build_base_rel_tlists(root, root->processed_= tlist);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* There are no ROWID_VAR Vars in this case, so= we're done. */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
=C2=A0 =C2=A0 }

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 =3D=3D NIL) block doesn=E2=80=99t = run in
this case, so it won=E2=80=99t add any row identity columns such as ctid fo= r
the partitioned root table.

In theory, we could prevent the planner from adding tableoid in the
first place when the child table doesn=E2=80=99t support any row identity column -- or worse, doesn=E2=80=99t 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 wor= k if we prevent the planner from adding tableoid
in the first pla= ce.

= =C2=A0

So for back branches, I=E2=80=99d 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=E2=80=99t very consistent with that patch even though th= e
internal error goes away.=C2=A0 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.
Sho= uld we add some comments to explain the output difference in EXPLAIN VERBOS= E
if enable_partition_pruning is set to a different value?
<= /div>


--
Thanks,
Tender Wang
--00000000000080b4250642f6babc--