Thanks Tom for the review.
On Thu, 29 Jan 2026 at 21:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andrew Dunstan <andrew@dunslane.net> writes:
> > These patches need a little copy editing (e.g.
> > "check_database_role_names_in_old_cluser" seems to be missing a "t") and
> > the error messages and comments need some tidying, but I think they are
> > basically sound.
Thanks Andrew for the review.
Fixed this typo.
> > Is there any objection to them in principle?
>
> +1 in principle. As you say, there's some tidying needed.
> A couple of points I noted:
>
> 1. check_lfcr_in_objname is about as unmusical a name as I can readily
> imagine. I was thinking about proposing "reject_newline_in_name"
> instead, but really I would drop that subroutine altogether and just
> code the checks in-line, because:
Fixed. As of now, I renamed it to reject_newline_in_name and changed
the error message as per suggestion but I kept this function as
previously suggested by some reviewers but I can remove this if this
looks odd.
>
> 2. I don't think this approach to constructing the error message
> meets our translatability guidelines. Better to just write out
> "role name \"%s\" contains ..." or "database name \"%s\" contains
> ...". We do use the other approach in some cases where it saves
> dozens of repetitive messages, but when there are only ever going
> to be two I'd rather err on the side of translatability.
>
Fixed.
> 3. I do not like the tests added to 040_createuser.pl, as they
> do not verify that the command fails for the expected reason.
>
Fixed. Added test case in .sql file to verify invalid names.
> 4. There's no point in running check_database_role_names_in_old_cluser
> against a v19 or later source server.
Fixed.
>
> regards, tom lane
Here, I am attaching updated patches for the review.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
