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

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