Andrew Dunstan <[email protected]> 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.
> 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:
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.
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.
4. There's no point in running check_database_role_names_in_old_cluser
against a v19 or later source server.
regards, tom lane
