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.
+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".
+ 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
