Thanks Andrew for the review.
On Sat, 5 Apr 2025 at 20:12, Andrew Dunstan <and...@dunslane.net> wrote:
>
>
> On 2025-04-02 We 7:45 AM, Mahendra Singh Thalor wrote:
>
> On Fri, 28 Mar 2025 at 20:13, Nathan Bossart <nathandboss...@gmail.com> 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 <srinath2...@gmail.com> wrote:
> >
> > ./psql postgres
> >
> > Hi,
> >
> > On Fri, Mar 28, 2025 at 5:08 PM Mahendra Singh Thalor <mahi6...@gmail.com>
> > 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
