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

I have reviewed all solutions but based on the commit message and comments, it is clear that the goal is to entirely forbid database names containing carriage return (CR) or line feed (LF) characters. Solution 1 LGTM and aligns with this approach by enforcing the restriction at the time of database creation, ensuring consistency throughout PostgreSQL. This approach eliminates ambiguity and guarantees that such database names cannot be created or dumped with CR or LF.

To validate this behavior, I have also implemented a TAP test for Solution 1.

You can still create a database with these using "CREATE DATABASE" though. Shouldn't we should really be preventing that?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

sorry for the noise ,previous response had my editor's formatting,just resending without that formatting.

./psql postgres

Hi,

On Wed, Mar 26, 2025 at 5:55 PM Andrew Dunstan <andrew@dunslane.net> wrote:

You can still create a database with these using "CREATE DATABASE" though. Shouldn't we should really be preventing that?

yes, solution 1 which I mentioned prevents these while we are using "CREATE DATABASE".

/*
  * Create a new database using the WAL_LOG strategy.
@@ -741,6 +742,13 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
  CreateDBStrategy dbstrategy = CREATEDB_WAL_LOG;
  createdb_failure_params fparms;
 
+ /* Report error if dbname have newline or carriage return in name. */
+ if (is_name_contain_lfcr(dbname))
+ 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"));
+

psql (18devel)
Type "help" for help.

postgres=# create database "test
postgres"# lines";
ERROR:  database name contains a newline or carriage return character
HINT:  newline or carriage return character is not allowed in database name

Yes, sorry, I misread the thread. I think we should proceed with options 1 and 3 i.e. prevent creation of new databases with a CR or LF, and have pgdumpall exit with a more useful error message.

Your invention of an is_name_contain_lfcr() function is unnecessary - we can just use the standard library function strpbrk() to look for a CR or LF.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, 27 Mar 2025 at 16:16, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2025-03-26 We 8:52 AM, Srinath Reddy wrote:
>
> sorry for the noise ,previous response had my editor's formatting,just resending without that formatting.
>
> ./psql postgres
>
> Hi,
>
> On Wed, Mar 26, 2025 at 5:55 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> You can still create a database with these using "CREATE DATABASE" though. Shouldn't we should really be preventing
that?
>
>
> yes, solution 1 which I mentioned prevents these while we are using "CREATE DATABASE".
>
> /*
>   * Create a new database using the WAL_LOG strategy.
> @@ -741,6 +742,13 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
>   CreateDBStrategy dbstrategy = CREATEDB_WAL_LOG;
>   createdb_failure_params fparms;
>
> + /* Report error if dbname have newline or carriage return in name. */
> + if (is_name_contain_lfcr(dbname))
> + 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"));
> +
>
> psql (18devel)
> Type "help" for help.
>
> postgres=# create database "test
> postgres"# lines";
> ERROR:  database name contains a newline or carriage return character
> HINT:  newline or carriage return character is not allowed in database name
>
>
>
>
> Yes, sorry, I misread the thread. I think we should proceed with options 1 and 3 i.e. prevent creation of new
databaseswith a CR or LF, and have pgdumpall exit with a more useful error message. 
>
> Your invention of an is_name_contain_lfcr() function is unnecessary - we can just use the standard library function
strpbrk()to look for a CR or LF. 
>
>
> cheers
>

Thanks Andrew and Srinath for feedback.

Yes, we should use the strpbrk function. Fixed.

Here, I am attaching an updated patch which has check in createdb and
RenameDatabase. For older versions, we can add more useful error
message (like: rename database as database has \n\r")

I will add some TAP tests and will make patches for older branches.

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

./psql postgres

On Thu, Mar 27, 2025 at 4:16 PM Andrew Dunstan <andrew@dunslane.net> wrote:

Yes, sorry, I misread the thread. I think we should proceed with options 1 and 3 i.e. prevent creation of new databases with a CR or LF, and have pgdumpall exit with a more useful error message.

agreed. 

Your invention of an is_name_contain_lfcr() function is unnecessary - we can just use the standard library function strpbrk() to look for a CR or LF.

makes sense,but I have a dumb doubt why in appendShellStringNoError() it still continues even after it found CR or LF? ,AFAIK The reasoning is this function is designed to silently filter out \n and \r while still producing a usable shell-safe argument. It informs the caller of the issue (false return value) but does not abruptly stop execution,then the caller will decide what to do but every place this function is called they are just throwing the error.

if we could just break the loop right after we found \n or \r in appendShellStringNoError() we can also use strpbrk() here and during creation of new database as you suggested.

thoughts?

 

I don't know. If you want to submit a patch cleaning it up go ahead. But right now I just want to get this original issue cleaned up to go along with the pg_dumpall improvements.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2025-03-27 Th 7:57 AM, Mahendra Singh Thalor wrote:
> On Thu, 27 Mar 2025 at 16:16, Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> On 2025-03-26 We 8:52 AM, Srinath Reddy wrote:
>>
>> sorry for the noise ,previous response had my editor's formatting,just resending without that formatting.
>>
>> ./psql postgres
>>
>> Hi,
>>
>> On Wed, Mar 26, 2025 at 5:55 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>>> You can still create a database with these using "CREATE DATABASE" though. Shouldn't we should really be preventing
that?
>>
>> yes, solution 1 which I mentioned prevents these while we are using "CREATE DATABASE".
>>
>> /*
>>    * Create a new database using the WAL_LOG strategy.
>> @@ -741,6 +742,13 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
>>    CreateDBStrategy dbstrategy = CREATEDB_WAL_LOG;
>>    createdb_failure_params fparms;
>>
>> + /* Report error if dbname have newline or carriage return in name. */
>> + if (is_name_contain_lfcr(dbname))
>> + 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"));
>> +
>>
>> psql (18devel)
>> Type "help" for help.
>>
>> postgres=# create database "test
>> postgres"# lines";
>> ERROR:  database name contains a newline or carriage return character
>> HINT:  newline or carriage return character is not allowed in database name
>>
>>
>>
>>
>> Yes, sorry, I misread the thread. I think we should proceed with options 1 and 3 i.e. prevent creation of new
databaseswith a CR or LF, and have pgdumpall exit with a more useful error message.
 
>>
>> Your invention of an is_name_contain_lfcr() function is unnecessary - we can just use the standard library function
strpbrk()to look for a CR or LF.
 
>>
>>
>> cheers
>>
> Thanks Andrew and Srinath for feedback.
>
> Yes, we should use the strpbrk function. Fixed.
>
> Here, I am attaching an updated patch which has check in createdb and
> RenameDatabase. For older versions, we can add more useful error
> message (like: rename database as database has \n\r")
>
> I will add some TAP tests and will make patches for older branches.
>


I don't think we can backpatch this. It's a behaviour change.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2025-Mar-27, Andrew Dunstan wrote:

> I don't think we can backpatch this. It's a behaviour change.

I agree, we can't.

Also, if we're going to enforce this rule, then pg_upgrade --check needs
to alert users that they have a database name that's no longer valid.
That needs to be part of this patch as well.  We should also ensure that
these are the only problem characters, which IMO means it should add a
test for pg_dumpall that creates a database whose name has all possible
characters and ensures that it is dumpable.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

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.

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.

+    /* 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.

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.

-- 
nathan

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.

Not sure if I will have time for this.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

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.

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

Thanks Andrew for the review.

On Sat, 5 Apr 2025 at 20:12, Andrew Dunstan <andrew@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 <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/usersnames then throw error instead of just logging database names into db_role_invalid_names and throwing
error,whichhelps the user to see at once all the invalid names and resolve then again try pg_upgrade,so here i am
attachingdelta 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
theinvalid 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
possiblyneeds 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

On 2025-03-28 Fr 10:43 AM, Nathan Bossart 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.
>
> 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.
>
> +    /* 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.


The latest patches do that, but I'm not really sure it's an improvement, 
nor that define.c is the right place for it (everything else there works 
on a defElem)


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

Maybe. I don't think there is time for that for v18, so we'd have to 
defer this for now. I can live with that - it's been like this for a 
long time.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sun, Apr 06, 2025 at 10:24:58AM -0400, Andrew Dunstan wrote:
> On 2025-03-28 Fr 10:43 AM, Nathan Bossart wrote:
>> 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.
> 
> Maybe. I don't think there is time for that for v18, so we'd have to defer
> this for now. I can live with that - it's been like this for a long time.

+1, I think deferring is the right call.

-- 
nathan

On 2025-Apr-06, Andrew Dunstan wrote:

> On 2025-03-28 Fr 10:43 AM, Nathan Bossart wrote:

> > 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.
> 
> Maybe. I don't think there is time for that for v18, so we'd have to defer
> this for now. I can live with that - it's been like this for a long time.

Grumble.  I'd rather introduce a partial restriction now only for names
that affect the tools failing outright (pg_dumpall in particular), than
do nothing for this release.  If we feel the need to extend the
restriction later, that's easy to do and bothers nobody.  We've wanted
these characters to be forbidden on database names for a long time, but
nobody has cared as much for their use on other types of names.  I'm not
even sure we'd support the idea of forbidding them on all names (though
the SQL standard doesn't allow control chars in identifiers.)

Another point is that we can easily have pg_upgrade check for invalid
database and role names, but checking *all* names would be more onerous.

I don't like the present implementation though, on translability
grounds.  I think the error message should appear at each callsite
rather than be hardcoded in the new function, to avoid string building.
I think it'd be cleaner if the new function (maybe "name_contains_crlf"
or "is_identifier_awful") just returned a boolean based on strpbrk(),
and the callsite throws the error.

I wonder why does the patch restrict both database and role names.  Does
a user with a newline also cause pg_upgrade to fail?  I mean, this
thread started with a consideration for database names only, and the
usage on role names seemed to have appeared out of nowhere in [1].

Cheers

[1] https://postgr.es/m/CAKYtNAoVKQL5rLD4P4hZXZSnThwO-j4q3Y1vTDHQGjzwC-kUJg@mail.gmail.com

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Aprender sin pensar es inútil; pensar sin aprender, peligroso" (Confucio)

Sent from my iPhone

> On Apr 6, 2025, at 11:23 AM, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Apr-06, Andrew Dunstan wrote:
>
>> On 2025-03-28 Fr 10:43 AM, Nathan Bossart wrote:
>
>>> 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.
>>
>> Maybe. I don't think there is time for that for v18, so we'd have to defer
>> this for now. I can live with that - it's been like this for a long time.
>
> Grumble.  I'd rather introduce a partial restriction now only for names
> that affect the tools failing outright (pg_dumpall in particular), than
> do nothing for this release.  If we feel the need to extend the
> restriction later, that's easy to do and bothers nobody.  We've wanted
> these characters to be forbidden on database names for a long time, but
> nobody has cared as much for their use on other types of names.  I'm not
> even sure we'd support the idea of forbidding them on all names (though
> the SQL standard doesn't allow control chars in identifiers.)

Ok. That seems reasonable, let’s do this now and revisit in the v19 cycle

Cheers

Andrew

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:
> Grumble.  I'd rather introduce a partial restriction now only for names
> that affect the tools failing outright (pg_dumpall in particular), than
> do nothing for this release.  If we feel the need to extend the
> restriction later, that's easy to do and bothers nobody.  We've wanted
> these characters to be forbidden on database names for a long time, but
> nobody has cared as much for their use on other types of names.  I'm not
> even sure we'd support the idea of forbidding them on all names (though
> the SQL standard doesn't allow control chars in identifiers.)

I'd be 100% behind forbidding all ASCII control characters in all
identifiers.  I can't see any situation in which that's a good thing,
and I can think of plenty where it's a mistake (eg your editor
decided to change space to tab) or done with underhanded intent.
If we can cite the SQL standard then it's an entirely defensible
restriction.

Having said that, I'm not quite sure where we ought to implement
the restriction, and it's possible that there are multiple places
that would need to check.  I concur that the day before feature
freeze is not a good time to be designing this.  Let's defer.

            regards, tom lane

On 2025-Apr-06, Tom Lane wrote:

> I'd be 100% behind forbidding all ASCII control characters in all
> identifiers.  I can't see any situation in which that's a good thing,
> and I can think of plenty where it's a mistake (eg your editor
> decided to change space to tab) or done with underhanded intent.

Right.

> If we can cite the SQL standard then it's an entirely defensible
> restriction.

We can.  It says (in 5.2 <token> and <separator>)

<regular identifier> ::= <identifier body>
<identifier body> ::= <identifier start> [ <identifier part>... ]
<identifier part> ::= <identifier start> | <identifier extend>
<identifier start> ::= !! See the Syntax Rules.
<identifier extend> ::= !! See the Syntax Rules.

Syntax Rules
1) An <identifier start> is any character in the Unicode General Category
   classes “Lu”, “Ll”, “Lt”, “Lm”, “Lo”, or “Nl”.
      NOTE 112 — The Unicode General Category classes “Lu”, “Ll”, “Lt”, “Lm”,
      “Lo”, and “Nl” are assigned to Unicode characters that are, respectively,
      upper-case letters, lower-case letters, title-case letters, modifier
      letters, other letters, and letter numbers.
2) An <identifier extend> is U+00B7, “Middle Dot”, or any character in the
   Unicode General Category classes “Mn”, “Mc”, “Nd”, or “Pc”.
      NOTE 113 — The Unicode General Category classes “Mn”, “Mc”, “Nd”, and
      “Pc”, are assigned to Unicode characters that are, respectively,
      non-spacing marks, spacing combining marks, decimal numbers, and connector
      punctuations.

The class for control characters is "C", so there are allowed nowhere.

https://www.unicode.org/charts/script/

> Having said that, I'm not quite sure where we ought to implement
> the restriction, and it's possible that there are multiple places
> that would need to check.

Yeah, a general ban on control characters for all identifiers is harder
to implement than a restricted ban, because it probably involves the
lexer, and I'm not sure the resulting "syntax error" type of rejections
are going to be nice enough to users.  A C-function based rejection
seems more convenient at this stage.

> I concur that the day before feature freeze is not a good time to be
> designing this.  Let's defer.

Augh.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"In fact, the basic problem with Perl 5's subroutines is that they're not
crufty enough, so the cruft leaks out into user-defined code instead, by
the Conservation of Cruft Principle."  (Larry Wall, Apocalypse 6)

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:
> On 2025-Apr-06, Tom Lane wrote:
>> If we can cite the SQL standard then it's an entirely defensible
>> restriction.

> We can.  It says (in 5.2 <token> and <separator>)

> <regular identifier> ::= <identifier body>
> <identifier body> ::= <identifier start> [ <identifier part>... ]
> <identifier part> ::= <identifier start> | <identifier extend>
> <identifier start> ::= !! See the Syntax Rules.
> <identifier extend> ::= !! See the Syntax Rules.

Hmm, but that's about non-quoted identifiers, so of course their
character set is pretty restricted.  What's of concern here is
what's allowed in double-quoted identifiers.  AFAICS there's
not much restriction: it can be any <nondoublequote character>,
and SR 7 says

    7) A <nondoublequote character> is any character of the source
    language character set other than a <double quote>.

        NOTE 115 — “source language character set” is defined in
        Subclause 4.10.1, “Host languages”, in ISO/IEC 9075-1.

The referenced bit of 9075-1 is pretty vague too:

    No matter what binding style is chosen, SQL-statements are written
    in an implementation-defined character set, known as the source
    language character set. The source language character set is not
    required to be the same as the character set of any character
    string appearing in SQL-data.

So I'm not really seeing anything there that justifies forbidding any
characters.  However, I still think that if we're going to forbid CR
or LF, we might as well go the whole way and forbid all the ASCII
control characters; none of them are any saner to use in identifiers
than those two.  (I'd be for banning   and similar as well, on
the same usability grounds as banning tabs, except that putting an
encoding dependency into this rule will not end well.)

            regards, tom lane

On 2025-04-06 Su 1:51 PM, Tom Lane wrote:
> =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:
>> On 2025-Apr-06, Tom Lane wrote:
>>> If we can cite the SQL standard then it's an entirely defensible
>>> restriction.
>> We can.  It says (in 5.2 <token> and <separator>)
>> <regular identifier> ::= <identifier body>
>> <identifier body> ::= <identifier start> [ <identifier part>... ]
>> <identifier part> ::= <identifier start> | <identifier extend>
>> <identifier start> ::= !! See the Syntax Rules.
>> <identifier extend> ::= !! See the Syntax Rules.
> Hmm, but that's about non-quoted identifiers, so of course their
> character set is pretty restricted.  What's of concern here is
> what's allowed in double-quoted identifiers.  AFAICS there's
> not much restriction: it can be any <nondoublequote character>,
> and SR 7 says
>
>      7) A <nondoublequote character> is any character of the source
>      language character set other than a <double quote>.
>
>          NOTE 115 — “source language character set” is defined in
>          Subclause 4.10.1, “Host languages”, in ISO/IEC 9075-1.
>
> The referenced bit of 9075-1 is pretty vague too:
>
>      No matter what binding style is chosen, SQL-statements are written
>      in an implementation-defined character set, known as the source
>      language character set. The source language character set is not
>      required to be the same as the character set of any character
>      string appearing in SQL-data.
>
> So I'm not really seeing anything there that justifies forbidding any
> characters.  However, I still think that if we're going to forbid CR
> or LF, we might as well go the whole way and forbid all the ASCII
> control characters; none of them are any saner to use in identifiers
> than those two.  (I'd be for banning   and similar as well, on
> the same usability grounds as banning tabs, except that putting an
> encoding dependency into this rule will not end well.)


Sound like we have some work to do, and that's not going to happen in 24 
hours.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, 7 Apr 2025 at 02:40, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2025-04-06 Su 1:51 PM, Tom Lane wrote:
> > =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org> writes:
> >> On 2025-Apr-06, Tom Lane wrote:
> >>> If we can cite the SQL standard then it's an entirely defensible
> >>> restriction.
> >> We can.  It says (in 5.2 <token> and <separator>)
> >> <regular identifier> ::= <identifier body>
> >> <identifier body> ::= <identifier start> [ <identifier part>... ]
> >> <identifier part> ::= <identifier start> | <identifier extend>
> >> <identifier start> ::= !! See the Syntax Rules.
> >> <identifier extend> ::= !! See the Syntax Rules.
> > Hmm, but that's about non-quoted identifiers, so of course their
> > character set is pretty restricted.  What's of concern here is
> > what's allowed in double-quoted identifiers.  AFAICS there's
> > not much restriction: it can be any <nondoublequote character>,
> > and SR 7 says
> >
> >      7) A <nondoublequote character> is any character of the source
> >      language character set other than a <double quote>.
> >
> >          NOTE 115 — “source language character set” is defined in
> >          Subclause 4.10.1, “Host languages”, in ISO/IEC 9075-1.
> >
> > The referenced bit of 9075-1 is pretty vague too:
> >
> >      No matter what binding style is chosen, SQL-statements are written
> >      in an implementation-defined character set, known as the source
> >      language character set. The source language character set is not
> >      required to be the same as the character set of any character
> >      string appearing in SQL-data.
> >
> > So I'm not really seeing anything there that justifies forbidding any
> > characters.  However, I still think that if we're going to forbid CR
> > or LF, we might as well go the whole way and forbid all the ASCII
> > control characters; none of them are any saner to use in identifiers
> > than those two.  (I'd be for banning   and similar as well, on
> > the same usability grounds as banning tabs, except that putting an
> > encoding dependency into this rule will not end well.)
>
>
> Sound like we have some work to do, and that's not going to happen in 24
> hours.
>
>
> cheers
>
>
> andrew
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>

Hi,
As per above discussions, for v18, we will not do any change to server
side to fix the issue of \n\r in database names. But as a cleanup
patch, we can give an alert to the user by "pg_upgrade --check". As
per current code, pg_dump and pg_upgrade will fail with "shell
command" error but in the attached patch, we will give some extra info
to the user by "pg_upgrade --check" so that they can fix database
names before trying to upgrade.

Here, I am attaching a patch which will give a list of invalid
database names in "pg_upgrade --check". We can consider this as a
cleanup patch.

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

On Thu, Apr 10, 2025 at 11:58:41PM +0530, Mahendra Singh Thalor wrote:
> As per above discussions, for v18, we will not do any change to server
> side to fix the issue of \n\r in database names. But as a cleanup
> patch, we can give an alert to the user by "pg_upgrade --check". As
> per current code, pg_dump and pg_upgrade will fail with "shell
> command" error but in the attached patch, we will give some extra info
> to the user by "pg_upgrade --check" so that they can fix database
> names before trying to upgrade.
> 
> Here, I am attaching a patch which will give a list of invalid
> database names in "pg_upgrade --check". We can consider this as a
> cleanup patch.

Are you proposing this for v18?  I think this is all v19 material at this
point.  Perhaps we could argue this is a bug fix that should be
back-patched, but IMHO that's a bit of a stretch.  I don't sense a
tremendous amount of urgency, either.

-- 
nathan