MIME-Version: 1.0
References: 
 <CA+3i_M8m1k2gFch+tU0JmAQh9FRV+pFrfTXDrJo+BqmwsTmOhg@mail.gmail.com>
 <CA+HiwqH+0rgMNQDog0AT9dVt0CGjXza_Li80njDhynWqzUwvZw@mail.gmail.com>
 <CA+3i_M8Mq7fr6mUspSa2rLik9+oY4sOeD1qsB_-dvnUB4NrYbg@mail.gmail.com>
 <CA+HiwqFW7uY3Jknf8VzjWoXa1bf0-U6-+JgM+o1ggJ+onyOHHA@mail.gmail.com>
 <976a432b-9221-4825-8e8f-41f4375737fe@app.fastmail.com>
 <CA+HiwqFenEgD4W1QWnHM0whbzWYH=uS6qQzxvKNH7z503HWvxw@mail.gmail.com>
In-Reply-To: 
 <CA+HiwqFenEgD4W1QWnHM0whbzWYH=uS6qQzxvKNH7z503HWvxw@mail.gmail.com>
From: Hu Xunqi <huxunqi.08@gmail.com>
Date: Fri, 17 Apr 2026 10:49:26 +0800
Message-ID: 
 <CAE4_qQacVuwviLN3qKMUp20e5B11hJ5rc5qUCLmHENixO1xWhQ@mail.gmail.com>
Subject: Re: Reject invalid databases in pg_get_database_ddl()
To: Amit Langote <amitlangote09@gmail.com>
Cc: Euler Taveira <euler@eulerto.com>, Lakshmi N <lakshmin.jhs@gmail.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
 Andrew Dunstan <andrew@dunslane.net>
Content-Type: multipart/alternative; boundary="000000000000f68a99064f9effd4"
Archived-At: 
 <https://www.postgresql.org/message-id/CAE4_qQacVuwviLN3qKMUp20e5B11hJ5rc5qUCLmHENixO1xWhQ%40mail.gmail.com>
Precedence: bulk

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

On Fri, Apr 17, 2026 at 10:16=E2=80=AFAM Amit Langote <amitlangote09@gmail.=
com>
wrote:

> On Fri, Apr 17, 2026 at 10:47=E2=80=AFAM Euler Taveira <euler@eulerto.com=
> wrote:
> > On Thu, Apr 16, 2026, at 8:46 PM, Amit Langote wrote:
> > >
> > > Thanks.  Will push the attached shortly.
> >
> > I think the errhint() is excessive in this context. It makes sense if
> you are
> > executing ALTER DATABASE, for example.
>
> Yeah, agreed.
>
> > I suggest a message like
> >
> >   database \"%s\" is an invalid database
>
> Or just drop it, because the errmsg already says "invalid database %s".
>
> > Regarding the test case suggested by Japin Li, I don't think it is wort=
h
> because
> > it is a transient state (unless something bad happened and pg_database
> contains
> > a dangling row.)
>
> Agreed.
>


+1. As this is an edge case failure, it=E2=80=99s not worth extending test =
time.


>
> Patch updated.
>
> --
> Thanks, Amit Langote
>

+       /*
+        * Reject invalid databases: datconnlimit =3D -2 would be emitted a=
s
+        * CONNECTION LIMIT =3D -2, which cannot be executed.
+        */

This comment looks a bit too centered on datconnlimit=3D-2, but the real
issue is that an invalid pg_database row should not be deparsed into DDL.
So, maybe rephrase like:

/*
* Reject invalid databases. Deparsing a pg_database row in invalid state
* can produce SQL that is not executable, such as CONNECTION LIMIT =3D -2.
*/


Regards,
Xunqi Hu

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

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote g=
mail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Fri, Apr 17,=
 2026 at 10:16=E2=80=AFAM Amit Langote &lt;<a href=3D"mailto:amitlangote09@=
gmail.com">amitlangote09@gmail.com</a>&gt; wrote:<br></div><blockquote clas=
s=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid r=
gb(204,204,204);padding-left:1ex">On Fri, Apr 17, 2026 at 10:47=E2=80=AFAM =
Euler Taveira &lt;<a href=3D"mailto:euler@eulerto.com" target=3D"_blank">eu=
ler@eulerto.com</a>&gt; wrote:<br>
&gt; On Thu, Apr 16, 2026, at 8:46 PM, Amit Langote wrote:<br>
&gt; &gt;<br>
&gt; &gt; Thanks.=C2=A0 Will push the attached shortly.<br>
&gt;<br>
&gt; I think the errhint() is excessive in this context. It makes sense if =
you are<br>
&gt; executing ALTER DATABASE, for example.<br>
<br>
Yeah, agreed.<br>
<br>
&gt; I suggest a message like<br>
&gt;<br>
&gt;=C2=A0 =C2=A0database \&quot;%s\&quot; is an invalid database<br>
<br>
Or just drop it, because the errmsg already says &quot;invalid database %s&=
quot;.<br>
<br>
&gt; Regarding the test case suggested by Japin Li, I don&#39;t think it is=
 worth because<br>
&gt; it is a transient state (unless something bad happened and pg_database=
 contains<br>
&gt; a dangling row.)<br>
<br>
Agreed.<br></blockquote><div><br></div><div><br></div><div>+1. As this is a=
n edge case failure, it=E2=80=99s not worth extending test time.</div><div>=
<br></div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margi=
n:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex=
">
<br>
Patch updated.<br>
<br>
-- <br>
Thanks, Amit Langote<br></blockquote><div><br></div><div>+=C2=A0 =C2=A0 =C2=
=A0 =C2=A0/*<br>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Reject invalid databases: da=
tconnlimit =3D -2 would be emitted as<br>+=C2=A0 =C2=A0 =C2=A0 =C2=A0 * CON=
NECTION LIMIT =3D -2, which cannot be executed.<br>+=C2=A0 =C2=A0 =C2=A0 =
=C2=A0 */</div><div><br></div>This comment looks a bit too centered on datc=
onnlimit=3D-2, but the real issue is that an invalid pg_database row should=
 not be deparsed into DDL. So, maybe rephrase like:<br><br>/*<br>* Reject i=
nvalid databases. Deparsing a pg_database row in invalid state<br>* can pro=
duce SQL that is not executable, such as CONNECTION LIMIT =3D -2.<br><div>*=
/=C2=A0</div><div><br></div><div><br></div><div>Regards,</div><div>Xunqi Hu=
</div></div></div>

--000000000000f68a99064f9effd4--