MIME-Version: 1.0
References: 
 <CAEoWx2=SLga-xH09Cq_PAvsHhQHrBK+V0vF821JKgzS=Bm0haA@mail.gmail.com>
 <CAKFQuwZXt2dAuHFs6MtRCzfMP6YZFzssuzdW2woJBVmco=CDdQ@mail.gmail.com>
 <CAEoWx2nic2MaPUEKwb0Me1iwO3LsCf8wUHm55pjqaTRgJEZpcQ@mail.gmail.com>
 <CAKFQuwahxpPKT-LXGJ34BO5cVBUxFxzgNPTY8E_VW-cuNU_DkQ@mail.gmail.com>
 <CAEoWx2kQ9aSn-0LGPKc=woj+h-HR4uBV9TL20aeM3HzLmFqgFQ@mail.gmail.com>
 <6eff5e43-cacd-4a2a-ad1d-e3b313c86050@uni-muenster.de>
 <950BB7B5-0180-4C36-82A0-7E17B920F740@gmail.com>
 <CAEoWx2mZsWnyS_XLDL5mL+yuU_g65p_tmHNesyfngqow7gtS3A@mail.gmail.com>
 <fb70022a-5403-4e16-9efc-28b041f35d9a@uni-muenster.de>
 <8ECD9403-F0BB-4971-94CF-2709EEB4E3B9@gmail.com>
 <f4f70647-8738-48b3-abbd-5b52cde97374@uni-muenster.de>
 <9174F0CF-2F70-4B4F-AED4-CAF113B7F093@gmail.com>
 <14669a83-c7b4-4cdf-890c-dceecd025ee1@uni-muenster.de>
 <3F59D90E-9A47-4C1C-B330-D62D668A462E@gmail.com>
 <5e01263a-1994-44d5-9e98-7212acf9c985@uni-muenster.de>
In-Reply-To: <5e01263a-1994-44d5-9e98-7212acf9c985@uni-muenster.de>
From: Greg Sabino Mullane <htamfids@gmail.com>
Date: Tue, 10 Mar 2026 11:32:29 -0400
Message-ID: 
 <CAKAnmmKzzOWKQVhaAt8LrBmUW=FLWBqS4v8cwKOG53hBToQZRA@mail.gmail.com>
Subject: Re: ALTER TABLE: warn when actions do not recurse to partitions
To: Jim Jones <jim.jones@uni-muenster.de>
Cc: Chao Li <li.evan.chao@gmail.com>,
	"David G. Johnston" <david.g.johnston@gmail.com>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000005e072a064cad3cb8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAKAnmmKzzOWKQVhaAt8LrBmUW%3DFLWBqS4v8cwKOG53hBToQZRA%40mail.gmail.com>
Precedence: bulk

--0000000000005e072a064cad3cb8
Content-Type: text/plain; charset="UTF-8"

Review of v6:

typedef struct partitionNoRecurseNotice
> {
>        List       *notices;
> }                      partitionNoRecurseNotice;


Not sure why we need a struct here, rather than just passing the list
around?

Also should be PartitionNoRecurseNotice (CamelCase)

               foreach(cell, postNotice->notices)
>                {
>                        if (strcmp((char *) lfirst(cell), notice_msg) == 0)
>                        {
>                                pfree(notice_msg);
>                                found = true;
>                                break;
>                        }
>                }


This seems a lot of extra work that could be avoided. Since we know each
message is unique to the cmdtype/AlterTableType and the rel/Relation
combination, use those two to drive the duplicate check. Then we can only
build the notice_msg if needed! Perhaps adding two more fields to that
lonely struct above?

 partitionNoRecurseNotice * postNotice);


postNotice is a little misleading - maybe pending_notices or just notices?

does not affect present partitions


s/present/existing/g


>   CollectPartitionNoRecurseNotice(AT_SetSchema, rel, stmt->relation->inh,
> false, &postNotice);


This hard-coded AT_SetSchema just to return a "SET SCHEMA" later on feels
hacky. Don't have a workaround off the top of my head, just registering my
mild unease. :)

               /* Emit a notice only if there are partitions */
>                if (nparts == 0)
>                        return;


It doesn't look like this particular case is tested. Other than that, the
tests look very good.


Cheers,
Greg

--0000000000005e072a064cad3cb8
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Review of v6:<div><br></div><div><blockqu=
ote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px=
 solid rgb(204,204,204);padding-left:1ex">typedef struct partitionNoRecurse=
Notice<br>{<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0List =C2=A0 =C2=A0 =C2=A0 *notice=
s;<br>} =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0partitionNoRecurseNotice;</blockquote><br></div><div>Not sure why=
 we need a struct here, rather than just passing the list around?</div><div=
><br></div><div>Also should be PartitionNoRecurseNotice (CamelCase)</div><d=
iv><br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px =
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">=C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0foreach(cell, postNotice-&gt;n=
otices)<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{<br>=C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0if (strcmp((char *) lfirst(cell), notice_msg) =3D=3D 0)<br>=C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{<=
br>=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=A0pfree(notice_msg);<br>=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=A0found =3D true;<br>=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=A0break;<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0}<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0}</blockquote><div><br></div><div>This seems a lot of extra wo=
rk that could be avoided. Since we know each message is unique to the cmdty=
pe/AlterTableType and the rel/Relation combination, use those two to drive =
the duplicate check. Then we can only build the notice_msg if needed! Perha=
ps adding two more fields to that lonely struct above?</div><div><br></div>=
<blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-=
left:1px solid rgb(204,204,204);padding-left:1ex">=C2=A0partitionNoRecurseN=
otice * postNotice);</blockquote><div><br></div><div>postNotice is a little=
 misleading - maybe pending_notices or just notices?</div><div><br></div><b=
lockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-le=
ft:1px solid rgb(204,204,204);padding-left:1ex">does not affect present par=
titions</blockquote><div><br></div><div>s/present/existing/g</div><div>=C2=
=A0</div></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0p=
x 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">=C2=A0 Col=
lectPartitionNoRecurseNotice(AT_SetSchema, rel, stmt-&gt;relation-&gt;inh, =
false, &amp;postNotice);</blockquote><div><br></div><div>This hard-coded AT=
_SetSchema just to return a &quot;SET SCHEMA&quot; later on feels hacky. Do=
n&#39;t have a workaround off the top of my head, just registering my mild =
unease. :)=C2=A0</div><div><br></div><blockquote class=3D"gmail_quote" styl=
e=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddin=
g-left:1ex">=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Emit =
a notice only if there are partitions */<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0if (nparts =3D=3D 0)<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;</blockquote=
><div><br></div><div>It doesn&#39;t look like this particular case is teste=
d. Other than that, the tests look very good.</div><div><br></div><div><br>=
</div><div dir=3D"ltr" class=3D"gmail_signature"><div dir=3D"ltr"><div>Chee=
rs,</div><div>Greg</div><div><br></div><div><br></div></div></div></div>

--0000000000005e072a064cad3cb8--