Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: Fix a server crash problem from pg_get_database_ddl
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAHg+QDeGL2s5MEDpoyre1avL-Rj+6c06b8ih5DEXBxBf5t+q4Q@mail.gmail.com>
Date: Thu, 30 Apr 2026 14:40:21 +0800
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-Transfer-Encoding: quoted-printable
Message-Id: <C3187259-0655-4FBA-AFB6-E443EB610994@gmail.com>
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>
 <CAHg+QDeGL2s5MEDpoyre1avL-Rj+6c06b8ih5DEXBxBf5t+q4Q@mail.gmail.com>
To: SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/C3187259-0655-4FBA-AFB6-E443EB610994%40gmail.com>
Precedence: bulk


> On Apr 27, 2026, at 12:56, SATYANARAYANA NARLAPURAM =
<satyanarlapuram@gmail.com> wrote:
>=20
> Hi Chao,
>=20
> On Sun, Apr 26, 2026 at 7:03=E2=80=AFPM Chao Li =
<li.evan.chao@gmail.com> wrote:
>=20
>=20
> > On Apr 26, 2026, at 22:50, Andrew Dunstan <andrew@dunslane.net> =
wrote:
> >=20
> >=20
> > On 2026-04-23 Th 2:47 AM, SATYANARAYANA NARLAPURAM wrote:
> >>=20
> >>=20
> >> Thanks for printing out that. Yes, they are similar.
> >>=20
> >> 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).
> >> ```
> >>=20
> >> So, let me take back this patch.
> >>=20
> >> [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.
> >>=20
> >> Attached an injection point test to show the race. Not intended to =
commit.
> >>=20
> >>=20
> >=20
> > 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 =
error seems more like the right thing to do.
> >=20
> > cheers
> >=20
> > andrew
> > --
> > Andrew Dunstan
> > EDB: https://www.enterprisedb.com
> >=20
>=20
> The v1 patch raises an error when the tablespace name is NULL.
>=20
> PFA v2: removed hint from the error message, because I now consider =
the hint might not be necessary.
>=20
> + if (spcname =3D=3D NULL)
> + ereport(ERROR,
> + errcode(ERRCODE_UNDEFINED_OBJECT),
> + errmsg("tablespace with OID %u does not exist",
> +   dbform->dattablespace));
> +
>=20
> A message with error detail that says a concurrent DDL could have =
dropped a tablespace could be better?
> System catalog is optional.
> Something like:
>=20
> errdetail("The tablespace may have been dropped concurrently, or the =
system catalog is inconsistent.")));
>=20
> Thanks,
> Satya

Hi Satya,

Thanks for your review. I hesitate to add a detail message here because =
we do not actually know the root cause. A concurrent DROP TABLESPACE =
could be one cause, but some unusual user operation could also lead to =
the same result, so I am not sure the detail message would help much.

The main purpose of this patch is to avoid passing NULL to =
pg_strcasecmp() and to report the missing object clearly. So I think the =
errmsg itself is enough.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/