Re: getting "shell command argument contains a newline or carriage return:" error with pg_dumpall when db name have new line in double quote

On Fri, 28 Mar 2025 at 20:13, Nathan Bossart <nathandbossart(at)gmail(dot)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 <srinath2133(at)gmail(dot)com> wrote:
>
> ./psql postgres
>
> Hi,
>
> On Fri, Mar 28, 2025 at 5:08 PM Mahendra Singh Thalor <mahi6run(at)gmail(dot)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.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com