Thanks Andrew for the review. On Sat, 5 Apr 2025 at 20:12, Andrew Dunstan <[email protected]> wrote: > > > On 2025-04-02 We 7:45 AM, Mahendra Singh Thalor wrote: > > On Fri, 28 Mar 2025 at 20:13, Nathan Bossart <[email protected]> wrote: > > > > On Fri, Mar 28, 2025 at 05:08:26PM +0530, Mahendra Singh Thalor wrote: > > > Here, I am attaching updated patches for review. > > > > > > v04_001* has the changes for CREATE DATABASE/ROLE/USER and > > > v04_002* has the changes into pg_upgrade to give ALERTS for invalid names. > > > > In general, +1 for these changes. Thanks for picking this up. > > Thanks Nathan for quick feedback. > > > > > If these are intended for v18, we probably should have a committer > > attached to it soon. I'm not confident that I'll have time for it, > > unfortunately. > > I think Andrew is planning this as a cleanup for "non-text pg_dumpall" patch. > Andrew, please if you have some bandwidth, then please let us know your > feedback for these patches. > > > > > + /* Report error if dbname have newline or carriage return in name. > > */ > > + if (strpbrk(dbname, "\n\r")) > > + ereport(ERROR, > > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE)), > > + errmsg("database name contains a newline or > > carriage return character"), > > + errhint("newline or carriage return > > character is not allowed in database name")); > > > > I think it would be better to move this to a helper function instead of > > duplicating this code in several places. > > Agreed. Fixed this and as Srinath pointed out, I moved it inside > "src/backend/commands/define.c" file. > > > > > Taking a step back, are we sure that 1) this is the right place to do these > > checks and 2) we shouldn't apply the same restrictions to all names? I'm > > wondering if it would be better to add these checks to the grammar instead > > of trying to patch up all the various places they are used in the tree. > > > > As of now, we made this restriction to only "database/role/user" because dump > is not allowed with these special characters. > yes, we can add these checks in grammar also. (probably we should add checks > in grammar only). If others also feel same, I can try to add these checks in > grammar. > > On Sun, 30 Mar 2025 at 00:03, Srinath Reddy <[email protected]> wrote: > > > > ./psql postgres > > > > Hi, > > > > On Fri, Mar 28, 2025 at 5:08 PM Mahendra Singh Thalor <[email protected]> > > wrote: > >> > >> > >> v04_001* has the changes for CREATE DATABASE/ROLE/USER and > >> v04_002* has the changes into pg_upgrade to give ALERTS for invalid names. > > > > > > i have reviewed these patches,in v04_002* i think it would be better if log > > all the names like database ,roles/users names then throw error instead of > > just logging database names into db_role_invalid_names and throwing > > error,which helps the user to see at once all the invalid names and resolve > > then again try pg_upgrade,so here i am attaching delta patch for the same > > ,except this the patches LGTM. > > > Thanks Srinath for the review. > > In attached v05_0002* patch, instead of 2 exec commands, I merged into 1 and > as per your comment, we will report all the invalid names at once and > additionally we are giving count of invalid names. > > Ex: pg_upgrade --check: >> >> Performing Consistency Checks >> ----------------------------- >> Checking cluster versions ok >> Checking database connection settings ok >> Checking names of databases/users/roles fatal >> >> All the database, role/user names should have only valid characters. A >> newline or >> carriage return character is not allowed in these object names. To fix >> this, please >> rename these names with valid names. >> To see all 5 invalid object names, refer db_role_user_invalid_names.txt file. >> >> /home/mst/pg_all/head_pg/postgres/inst/bin/data/pg_upgrade_output.d/20250402T160610.664/db_role_user_invalid_names.txt >> Failure, exiting > > > Here, I am attaching updated patches for review. > > > > +void > +check_lfcr_in_objname(const char *objname, const char *objtype) > +{ > + /* Report error if name has \n or \r character. */ > + if (strpbrk(objname, "\n\r")) > + ereport(ERROR, > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE)), > + errmsg("%s name contains a newline or carriage return > character", objtype), > + errhint("a newline or a carriage return character is not > allowed in %s name\n" > + "here, given %s name is : \"%s\"", objtype, objtype, > objname)); > +} > > > I don't think the errhint here adds much. If we're going to put the offending > name in an error message I think it possibly needs to be escaped so it's more > obvious where the CR/LF are. This should be in an errdetail rather than an > errhint.
Fixed. I replaced errhint with errdetail as "errdetail("invalid %s
name is \"%s\"", objtype, objname));"
>
> +static void
> +check_database_user_role_names_in_old_cluser(ClusterInfo *cluster)
>
> s/cluser/cluster/
>
> + prep_status("Checking names of databases/users/roles ");
>
> I would just say "Checking names of databases and roles".
Fixed.
>
>
> + pg_fatal("All the database, role/user names should have only valid
> characters. A newline or \n"
> + "carriage return character is not allowed in these object
> names. To fix this, please \n"
> + "rename these names with valid names. \n"
> + "To see all %d invalid object names, refer
> db_role_user_invalid_names.txt file. \n"
> + " %s", count, output_path);
>
>
> Also needs some cleanup.
>
>
>
> cheers
>
>
> andrew
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
Here, I am attaching updated patches for review.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
v06_0001_don-t-allow-newline-or-carriage-return-in-db-user-role-names.patch
Description: Binary data
v06_0002-add-handling-to-pg_upgrade-to-report-alert-for-invalid-names.patch
Description: Binary data
