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

From: Tom Lane <[email-masked]>
Date: Sun, 06 Apr 2025 13:21:53 -0400

=?utf-8?Q?=C3=81lvaro?= Herrera <[email-masked]> 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

---

From: Álvaro Herrera <[email-masked]>
Date: Sun, 6 Apr 2025 19:34:51 +0200

> 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

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/

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

---

From: Tom Lane <[email-masked]>
Date: Sun, 06 Apr 2025 13:51:49 -0400

> <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 &nbsp; 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

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 6 Apr 2025 17:10:51 -0400

> 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

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Thu, 10 Apr 2025 23:58:41 +0530

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

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

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 11 Apr 2025 09:47:49 -0500

> database names in "pg_upgrade --check". We can consider this as a

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

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Tue, 24 Jun 2025 11:37:00 +0530

> tremendous amount of urgency, either.

Thanks Nathan for the review.

I want to re-start this thread for v19. I posted v06* patches in my
previous updates[1]. Please someone review it and let me know feedback.

v06 patches: v06_patches
<https://www.postgresql.org/message-id/CAKYtNAqC5pkjmh8UgvbNLtMyEVeKUtDF3_%2B9dvG9zb8YrhTJQQ%40mail.gmail.com>

[1] :
https://www.postgresql.org/message-id/CAKYtNAqC5pkjmh8UgvbNLtMyEVeKUtDF3_%2B9dvG9zb8YrhTJQQ%40mail.gmail.com