Reject invalid databases in pg_get_database_ddl()

Hi,

On Thu, Apr 16, 2026 at 5:20 PM Lakshmi N <lakshmin.jhs@gmail.com> wrote:

pg_get_database_ddl() is not checking for databases in an invalid state
before producing ddl statements. This caused the function to emit
CONNECTION_LIMIT = -2, which is invalid SQL that Postgres rejects.
A database row can be in this inconsistent state longer, for example
server crashed during a drop database.

Attached patch to fix this issue by doing a database_is_invalid_form()
check early in pg_get_database_ddl_internal().

Thanks for the report.

Hmm, I see that the function will happily emit datconnlimit = -2 and
your patch catches that at the top instead of down below near this
code:

/* CONNECTION LIMIT */
if (dbform->datconnlimit != -1)
{
resetStringInfo(&buf);
appendStringInfo(&buf, "ALTER DATABASE %s CONNECTION LIMIT = %d;",
quote_identifier(dbname), dbform->datconnlimit);
statements = lappend(statements, pstrdup(buf.data));
}

which, I guess, makes sense.

The comment is correct but could be more explicit:

/*
* Reject invalid databases: datconnlimit = -2 would be emitted as
* CONNECTION LIMIT = -2, which fails on replay.
*/

--
Thanks, Amit Langote

Hi Amit,

On Thu, Apr 16, 2026 at 2:29 AM Amit Langote <amitlangote09@gmail.com>
wrote:

Hi,

On Thu, Apr 16, 2026 at 5:20 PM Lakshmi N <lakshmin.jhs@gmail.com> wrote:

pg_get_database_ddl() is not checking for databases in an invalid state
before producing ddl statements. This caused the function to emit
CONNECTION_LIMIT = -2, which is invalid SQL that Postgres rejects.
A database row can be in this inconsistent state longer, for example
server crashed during a drop database.

Attached patch to fix this issue by doing a database_is_invalid_form()
check early in pg_get_database_ddl_internal().

Thanks for the report.

Hmm, I see that the function will happily emit datconnlimit = -2 and
your patch catches that at the top instead of down below near this
code:

/* CONNECTION LIMIT */
if (dbform->datconnlimit != -1)
{
resetStringInfo(&buf);
appendStringInfo(&buf, "ALTER DATABASE %s CONNECTION LIMIT = %d;",
quote_identifier(dbname), dbform->datconnlimit);
statements = lappend(statements, pstrdup(buf.data));
}

which, I guess, makes sense.

The comment is correct but could be more explicit:

/*
* Reject invalid databases: datconnlimit = -2 would be emitted as
* CONNECTION LIMIT = -2, which fails on replay.
*/

Thank you for reviewing! Please find the attached v2 addressing this.

Regards,
Lakshmi

On Fri, Apr 17, 2026 at 1:46 AM Lakshmi N <lakshmin.jhs@gmail.com> wrote:

On Thu, Apr 16, 2026 at 2:29 AM Amit Langote <amitlangote09@gmail.com> wrote:

Hi,

On Thu, Apr 16, 2026 at 5:20 PM Lakshmi N <lakshmin.jhs@gmail.com> wrote:

pg_get_database_ddl() is not checking for databases in an invalid state
before producing ddl statements. This caused the function to emit
CONNECTION_LIMIT = -2, which is invalid SQL that Postgres rejects.
A database row can be in this inconsistent state longer, for example
server crashed during a drop database.

Attached patch to fix this issue by doing a database_is_invalid_form()
check early in pg_get_database_ddl_internal().

Thanks for the report.

Hmm, I see that the function will happily emit datconnlimit = -2 and
your patch catches that at the top instead of down below near this
code:

/* CONNECTION LIMIT */
if (dbform->datconnlimit != -1)
{
resetStringInfo(&buf);
appendStringInfo(&buf, "ALTER DATABASE %s CONNECTION LIMIT = %d;",
quote_identifier(dbname), dbform->datconnlimit);
statements = lappend(statements, pstrdup(buf.data));
}

which, I guess, makes sense.

The comment is correct but could be more explicit:

/*
* Reject invalid databases: datconnlimit = -2 would be emitted as
* CONNECTION LIMIT = -2, which fails on replay.
*/

Thank you for reviewing! Please find the attached v2 addressing this.

Thanks. Will push the attached shortly.

--
Thanks, Amit Langote

Hi, Lakshmi

On Thu, 16 Apr 2026 at 09:46, Lakshmi N <lakshmin.jhs@gmail.com> wrote:

Hi Amit,

On Thu, Apr 16, 2026 at 2:29 AM Amit Langote <amitlangote09@gmail.com> wrote:

Hi,

On Thu, Apr 16, 2026 at 5:20 PM Lakshmi N <lakshmin.jhs@gmail.com> wrote:

pg_get_database_ddl() is not checking for databases in an invalid state
before producing ddl statements. This caused the function to emit
CONNECTION_LIMIT = -2, which is invalid SQL that Postgres rejects.
A database row can be in this inconsistent state longer, for example
server crashed during a drop database.

Attached patch to fix this issue by doing a database_is_invalid_form()
check early in pg_get_database_ddl_internal().

Thanks for the report.

Hmm, I see that the function will happily emit datconnlimit = -2 and
your patch catches that at the top instead of down below near this
code:

/* CONNECTION LIMIT */
if (dbform->datconnlimit != -1)
{
resetStringInfo(&buf);
appendStringInfo(&buf, "ALTER DATABASE %s CONNECTION LIMIT = %d;",
quote_identifier(dbname), dbform->datconnlimit);
statements = lappend(statements, pstrdup(buf.data));
}

which, I guess, makes sense.

The comment is correct but could be more explicit:

/*
* Reject invalid databases: datconnlimit = -2 would be emitted as
* CONNECTION LIMIT = -2, which fails on replay.
*/

Thank you for reviewing! Please find the attached v2 addressing this.

Thanks for updating the patch. Is it possible to cover this with a test case?

Regards,
Lakshmi

[4. text/x-diff; v2-0001-Reject-pg_get_database_ddl-for-invalid-databases.patch]...

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

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. I suggest a message like

database \"%s\" is an invalid database

Regarding the test case suggested by Japin Li, I don't think it is worth because
it is a transient state (unless something bad happened and pg_database contains
a dangling row.)

--
Euler Taveira
EDB https://www.enterprisedb.com/

On Fri, Apr 17, 2026 at 10:47 AM 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 worth because
it is a transient state (unless something bad happened and pg_database contains
a dangling row.)

Agreed.

Patch updated.

--
Thanks, Amit Langote

On Thu, 16 Apr 2026 at 22:46, "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. I suggest a message like

database \"%s\" is an invalid database

Regarding the test case suggested by Japin Li, I don't think it is worth because
it is a transient state (unless something bad happened and pg_database contains
a dangling row.)

Thanks for clarifying. Got it.

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

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

On Fri, Apr 17, 2026 at 10:47 AM 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 worth

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’s not worth extending test time.

Patch updated.

--
Thanks, Amit Langote

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

This comment looks a bit too centered on datconnlimit=-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 = -2.
*/

Regards,
Xunqi Hu

Hi,

On Fri, Apr 17, 2026 at 11:49 AM Hu Xunqi <huxunqi.08@gmail.com> wrote:

On Fri, Apr 17, 2026 at 10:16 AM Amit Langote <amitlangote09@gmail.com> wrote:
+       /*
+        * Reject invalid databases: datconnlimit = -2 would be emitted as
+        * CONNECTION LIMIT = -2, which cannot be executed.
+        */

This comment looks a bit too centered on datconnlimit=-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 = -2.
*/

I was trying to be precise about datconnlimit = -2 being the thing
that produces invalid SQL. But your version covers that with the "such
as CONNECTION LIMIT = -2" example, and it's closer to the original,
which was on the right track, just needed to be more precise. Let's go
with it.

--
Thanks, Amit Langote

Hi,

On Thu, Apr 16, 2026 at 8:31 PM Amit Langote <amitlangote09@gmail.com>
wrote:

Hi,

On Fri, Apr 17, 2026 at 11:49 AM Hu Xunqi <huxunqi.08@gmail.com> wrote:

On Fri, Apr 17, 2026 at 10:16 AM Amit Langote <amitlangote09@gmail.com>

wrote:

+       /*
+        * Reject invalid databases: datconnlimit = -2 would be emitted

as

+        * CONNECTION LIMIT = -2, which cannot be executed.
+        */

This comment looks a bit too centered on datconnlimit=-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 = -2.
*/

I was trying to be precise about datconnlimit = -2 being the thing
that produces invalid SQL. But your version covers that with the "such
as CONNECTION LIMIT = -2" example, and it's closer to the original,
which was on the right track, just needed to be more precise. Let's go
with it.

This looks good to me. Thank you for reviewing and making the changes!

Regards,
Lakshmi

On Fri, Apr 17, 2026 at 1:21 PM Lakshmi N <lakshmin.jhs@gmail.com> wrote:

On Thu, Apr 16, 2026 at 8:31 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Apr 17, 2026 at 11:49 AM Hu Xunqi <huxunqi.08@gmail.com> wrote:

On Fri, Apr 17, 2026 at 10:16 AM Amit Langote <amitlangote09@gmail.com> wrote:
+       /*
+        * Reject invalid databases: datconnlimit = -2 would be emitted as
+        * CONNECTION LIMIT = -2, which cannot be executed.
+        */

This comment looks a bit too centered on datconnlimit=-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 = -2.
*/

I was trying to be precise about datconnlimit = -2 being the thing
that produces invalid SQL. But your version covers that with the "such
as CONNECTION LIMIT = -2" example, and it's closer to the original,
which was on the right track, just needed to be more precise. Let's go
with it.

This looks good to me. Thank you for reviewing and making the changes!

Pushed.

--
Thanks, Amit Langote

On 2026-04-17 Fr 12:22 AM, Amit Langote wrote:

On Fri, Apr 17, 2026 at 1:21 PM Lakshmi N <lakshmin.jhs@gmail.com> wrote:

On Thu, Apr 16, 2026 at 8:31 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Apr 17, 2026 at 11:49 AM Hu Xunqi <huxunqi.08@gmail.com> wrote:

On Fri, Apr 17, 2026 at 10:16 AM Amit Langote <amitlangote09@gmail.com> wrote:
+       /*
+        * Reject invalid databases: datconnlimit = -2 would be emitted as
+        * CONNECTION LIMIT = -2, which cannot be executed.
+        */

This comment looks a bit too centered on datconnlimit=-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 = -2.
*/

I was trying to be precise about datconnlimit = -2 being the thing
that produces invalid SQL. But your version covers that with the "such
as CONNECTION LIMIT = -2" example, and it's closer to the original,
which was on the right track, just needed to be more precise. Let's go
with it.

This looks good to me. Thank you for reviewing and making the changes!

Pushed.

Thanks for jumping on this.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com