diff --git a/compute_tools/src/pg_helpers.rs b/compute_tools/src/pg_helpers.rs index 10d8f2c878..94467a0d2f 100644 --- a/compute_tools/src/pg_helpers.rs +++ b/compute_tools/src/pg_helpers.rs @@ -213,8 +213,10 @@ impl Escaping for PgIdent { // Find the first suitable tag that is not present in the string. // Postgres' max role/DB name length is 63 bytes, so even in the - // worst case it won't take long. - while self.contains(&format!("${tag}$")) || self.contains(&format!("${outer_tag}$")) { + // worst case it won't take long. Outer tag is always `tag + "x"`, + // so if `tag` is not present in the string, `outer_tag` is not + // present in the string either. + while self.contains(&tag.to_string()) { tag += "x"; outer_tag = tag.clone() + "x"; } diff --git a/compute_tools/tests/pg_helpers_tests.rs b/compute_tools/tests/pg_helpers_tests.rs index 53f2ddad84..04b6ed2256 100644 --- a/compute_tools/tests/pg_helpers_tests.rs +++ b/compute_tools/tests/pg_helpers_tests.rs @@ -71,6 +71,14 @@ test.escaping = 'here''s a backslash \\ and a quote '' and a double-quote " hoor ("name$$$", ("$x$name$$$$x$", "xx")), ("name$$$$", ("$x$name$$$$$x$", "xx")), ("name$x$", ("$xx$name$x$$xx$", "xxx")), + ("x", ("$xx$x$xx$", "xxx")), + ("xx", ("$xxx$xx$xxx$", "xxxx")), + ("$x", ("$xx$$x$xx$", "xxx")), + ("x$", ("$xx$x$$xx$", "xxx")), + ("$x$", ("$xx$$x$$xx$", "xxx")), + ("xx$", ("$xxx$xx$$xxx$", "xxxx")), + ("$xx", ("$xxx$$xx$xxx$", "xxxx")), + ("$xx$", ("$xxx$$xx$$xxx$", "xxxx")), ]; for (input, expected) in test_cases { diff --git a/test_runner/regress/test_compute_catalog.py b/test_runner/regress/test_compute_catalog.py index b66b326360..6ee6837cd2 100644 --- a/test_runner/regress/test_compute_catalog.py +++ b/test_runner/regress/test_compute_catalog.py @@ -19,6 +19,16 @@ TEST_ROLE_NAMES = [ {"name": "role$"}, {"name": "role$$"}, {"name": "role$x$"}, + {"name": "x"}, + {"name": "xx"}, + {"name": "$x"}, + {"name": "x$"}, + {"name": "$x$"}, + {"name": "xx$"}, + {"name": "$xx"}, + {"name": "$xx$"}, + # 63 bytes is the limit for role/DB names in Postgres + {"name": "x" * 63}, ] TEST_DB_NAMES = [ @@ -74,6 +84,43 @@ TEST_DB_NAMES = [ "name": "db name$x$", "owner": "role$x$", }, + { + "name": "x", + "owner": "x", + }, + { + "name": "xx", + "owner": "xx", + }, + { + "name": "$x", + "owner": "$x", + }, + { + "name": "x$", + "owner": "x$", + }, + { + "name": "$x$", + "owner": "$x$", + }, + { + "name": "xx$", + "owner": "xx$", + }, + { + "name": "$xx", + "owner": "$xx", + }, + { + "name": "$xx$", + "owner": "$xx$", + }, + # 63 bytes is the limit for role/DB names in Postgres + { + "name": "x" * 63, + "owner": "x" * 63, + }, ] @@ -146,6 +193,10 @@ def test_compute_create_drop_dbs_and_roles(neon_simple_env: NeonEnv): """ Test that compute_ctl can create and work with databases and roles with special characters (whitespaces, %, tabs, etc.) in the name. + Also use `drop_subscriptions_before_start: true`. We do not actually + have any subscriptions in this test, so it should be no-op, but it + i) simulates the case when we create a second dev branch together with + a new project creation, and ii) just generally stresses more code paths. """ env = neon_simple_env @@ -159,6 +210,7 @@ def test_compute_create_drop_dbs_and_roles(neon_simple_env: NeonEnv): **{ "spec": { "skip_pg_catalog_updates": False, + "drop_subscriptions_before_start": True, "cluster": { "roles": TEST_ROLE_NAMES, "databases": TEST_DB_NAMES, @@ -202,6 +254,7 @@ def test_compute_create_drop_dbs_and_roles(neon_simple_env: NeonEnv): **{ "spec": { "skip_pg_catalog_updates": False, + "drop_subscriptions_before_start": True, "cluster": { "roles": [], "databases": [],