On Fri, Apr 17, 2026 at 10:16 AM Amit Langote <[email protected]> wrote:
> On Fri, Apr 17, 2026 at 10:47 AM Euler Taveira <[email protected]> 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
