MIME-Version: 1.0
References: <573E45C1-31A4-4885-A00C-1A2171159A2A@gmail.com>
 <28b82ab2-5721-4e7c-bf71-979c3f198a2e@app.fastmail.com>
 <SY7PR01MB109214566B069E9C9084590FEB6232@SY7PR01MB10921.ausprd01.prod.outlook.com>
 <634F4A5D-9D38-4F9D-BC1C-70815CBB5089@gmail.com>
 <CAHg+QDcNyJ94cCD+9ZRfz==hDnghjE5BaR4+BiSWXt82hpgDtA@mail.gmail.com>
 <13310e0b-e2b0-45a2-873d-e2b51a8ea3b4@dunslane.net>
 <0777DBAC-9348-423A-AB51-5AAFDA445B95@gmail.com>
In-Reply-To: <0777DBAC-9348-423A-AB51-5AAFDA445B95@gmail.com>
From: SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>
Date: Sun, 26 Apr 2026 21:56:42 -0700
Message-ID: 
 <CAHg+QDeGL2s5MEDpoyre1avL-Rj+6c06b8ih5DEXBxBf5t+q4Q@mail.gmail.com>
Subject: Re: Fix a server crash problem from pg_get_database_ddl
To: Chao Li <li.evan.chao@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>, Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Japin Li <japinli@hotmail.com>
Content-Type: multipart/alternative; boundary="0000000000007eec4b065069f1bc"
Archived-At: 
 <https://www.postgresql.org/message-id/CAHg%2BQDeGL2s5MEDpoyre1avL-Rj%2B6c06b8ih5DEXBxBf5t%2Bq4Q%40mail.gmail.com>
Precedence: bulk

--0000000000007eec4b065069f1bc
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Chao,

On Sun, Apr 26, 2026 at 7:03=E2=80=AFPM Chao Li <li.evan.chao@gmail.com> wr=
ote:

>
>
> > On Apr 26, 2026, at 22:50, Andrew Dunstan <andrew@dunslane.net> wrote:
> >
> >
> > On 2026-04-23 Th 2:47 AM, SATYANARAYANA NARLAPURAM wrote:
> >>
> >>
> >> Thanks for printing out that. Yes, they are similar.
> >>
> >> I agree with what Tom said in [2]:
> >> ```
> >> This is not a bug. This is a superuser intentionally breaking
> >> the system by corrupting the catalogs. There are any number
> >> of ways to cause trouble with ill-advised manual updates to a
> >> catalog table. Try, eg, "DELETE FROM pg_proc" (... but not in
> >> a database you care about).
> >> ```
> >>
> >> So, let me take back this patch.
> >>
> >> [2]
> https://www.postgresql.org/message-id/1538113.1768921841@sss.pgh.pa.us
> >> In this case, it is a very corner case but not something superuser
> intentionally breaks.
> >> For example, a concurrent tablespace drop + database ddl to assign a
> different tablespace or default.
> >> We aren't acquiring Access Share lock on the DB in this function
> (intentional) so it is a good practice
> >> to do the null checks. Of course, it makes more sense to add this
> comment while doing a code review.
> >> I will let Tom and others chime in with their thoughts on fixing this.
> >>
> >> Attached an injection point test to show the race. Not intended to
> commit.
> >>
> >>
> >
> > I agree if there's a race condition we should protect against it. I
> don't much like the idea of silently ignoring it, though. Raising an erro=
r
> seems more like the right thing to do.
> >
> > cheers
> >
> > andrew
> > --
> > Andrew Dunstan
> > EDB: https://www.enterprisedb.com
> >
>
> The v1 patch raises an error when the tablespace name is NULL.
>
> PFA v2: removed hint from the error message, because I now consider the
> hint might not be necessary.
>

+ if (spcname =3D=3D NULL)
+ ereport(ERROR,
+ errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("tablespace with OID %u does not exist",
+   dbform->dattablespace));
+

A message with error detail that says a concurrent DDL could have dropped a
tablespace could be better?
System catalog is optional.
Something like:

errdetail("The tablespace may have been dropped concurrently, or the system
catalog is inconsistent.")));

Thanks,
Satya

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

<div dir=3D"ltr"><div dir=3D"ltr">Hi Chao,</div><br><div class=3D"gmail_quo=
te gmail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Sun, Apr=
 26, 2026 at 7:03=E2=80=AFPM Chao Li &lt;<a href=3D"mailto:li.evan.chao@gma=
il.com">li.evan.chao@gmail.com</a>&gt; wrote:<br></div><blockquote class=3D=
"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(2=
04,204,204);padding-left:1ex"><br>
<br>
&gt; On Apr 26, 2026, at 22:50, Andrew Dunstan &lt;<a href=3D"mailto:andrew=
@dunslane.net" target=3D"_blank">andrew@dunslane.net</a>&gt; wrote:<br>
&gt; <br>
&gt; <br>
&gt; On 2026-04-23 Th 2:47 AM, SATYANARAYANA NARLAPURAM wrote:<br>
&gt;&gt; <br>
&gt;&gt; <br>
&gt;&gt; Thanks for printing out that. Yes, they are similar.<br>
&gt;&gt; <br>
&gt;&gt; I agree with what Tom said in [2]:<br>
&gt;&gt; ```<br>
&gt;&gt; This is not a bug. This is a superuser intentionally breaking<br>
&gt;&gt; the system by corrupting the catalogs. There are any number<br>
&gt;&gt; of ways to cause trouble with ill-advised manual updates to a<br>
&gt;&gt; catalog table. Try, eg, &quot;DELETE FROM pg_proc&quot; (... but n=
ot in<br>
&gt;&gt; a database you care about).<br>
&gt;&gt; ```<br>
&gt;&gt; <br>
&gt;&gt; So, let me take back this patch.<br>
&gt;&gt; <br>
&gt;&gt; [2] <a href=3D"https://www.postgresql.org/message-id/1538113.17689=
21841@sss.pgh.pa.us" rel=3D"noreferrer" target=3D"_blank">https://www.postg=
resql.org/message-id/1538113.1768921841@sss.pgh.pa.us</a> <br>
&gt;&gt; In this case, it is a very corner case but not something superuser=
 intentionally breaks.<br>
&gt;&gt; For example, a concurrent tablespace drop + database ddl to assign=
 a different tablespace or default.<br>
&gt;&gt; We aren&#39;t acquiring Access Share lock on the DB in this functi=
on (intentional) so it is a good practice<br>
&gt;&gt; to do the null checks. Of course, it makes more sense to add this =
comment while doing a code review.<br>
&gt;&gt; I will let Tom and others chime in with their thoughts on fixing t=
his.<br>
&gt;&gt; <br>
&gt;&gt; Attached an injection point test to show the race. Not intended to=
 commit.<br>
&gt;&gt; <br>
&gt;&gt; <br>
&gt; <br>
&gt; I agree if there&#39;s a race condition we should protect against it. =
I don&#39;t much like the idea of silently ignoring it, though. Raising an =
error seems more like the right thing to do.<br>
&gt; <br>
&gt; cheers<br>
&gt; <br>
&gt; andrew<br>
&gt; --<br>
&gt; Andrew Dunstan<br>
&gt; EDB: <a href=3D"https://www.enterprisedb.com" rel=3D"noreferrer" targe=
t=3D"_blank">https://www.enterprisedb.com</a><br>
&gt; <br>
<br>
The v1 patch raises an error when the tablespace name is NULL.<br>
<br>
PFA v2: removed hint from the error message, because I now consider the hin=
t might not be necessary.<br></blockquote><div><br></div>+		if (spcname =3D=
=3D NULL)<br>+			ereport(ERROR,<br>+					errcode(ERRCODE_UNDEFINED_OBJECT),=
<br>+					errmsg(&quot;tablespace with OID %u does not exist&quot;,<br>+			=
			 =C2=A0 dbform-&gt;dattablespace));<br><div>+</div><div><br></div><div>A=
 message with error detail that says a concurrent DDL could have dropped a =
tablespace could be better?</div><div>System catalog is optional.</div><div=
>Something like:</div><div><br></div><div> <span class=3D"gmail-=CD=BCe">er=
rdetail</span>(<span class=3D"gmail-=CD=BCc">&quot;The tablespace may have =
been dropped concurrently, or the system catalog is inconsistent.&quot;</sp=
an>)));</div><div><br></div><div>Thanks,</div><div>Satya</div></div></div>

--0000000000007eec4b065069f1bc--