On Sun, 6 Apr 2025 at 20:53, Álvaro Herrera <alvhe...@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.) > > 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].
In my testing, pg_dumpall was not failing with roles/user (\n\r) in my machine but due to the below comment in code, I restricted roles also. +++ b/src/fe_utils/string_utils.c > @@ -568,12 +568,6 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned > char *str, size_t length, > * Append the given string to the shell command being built in the buffer, > * with shell-style quoting as needed to create exactly one argument. > * > - * Forbid LF or CR characters, which have scant practical use beyond > designing > - * security breaches. The Windows command shell is unusable as a conduit > for > - * arguments containing LF or CR characters. A future major release > should > - * reject those characters in CREATE ROLE and CREATE DATABASE, because use > - * there eventually leads to errors here. > - * > * appendShellString() simply prints an error and dies if LF or CR > appears. > Cheers > > [1] https://postgr.es/m/cakytnaovkql5rld4p4hzxzsnthwo-j4q3y1vtdhqgjzwc-k...@mail.gmail.com > > -- > Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ > "Aprender sin pensar es inútil; pensar sin aprender, peligroso" (Confucio) -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
