fix(compute_ctl): Allow usage of DB names with whitespaces

[ololobus]

Problem

We used set_path() to replace the database name in the connection string. It automatically does url-safe encoding if the path is not already encoded, but it does it as per the URL standard, which assumes that tabs can be safely removed from the path without changing the meaning of the URL. See, e.g., https://url.spec.whatwg.org/#concept-basic-url-parser. It also breaks for DBs with properly %-encoded names, like with %20, as they are kept intact, but actually should be escaped.

Yet, this is not true for Postgres, where it's completely valid to have trailing tabs in the database name.

I think this is the PR that caused this regression #9717, as it switched from postgres::config::Config back to set_path().

This was fixed a while ago already 1, btw, I just haven't added a test to catch this regression back then :(

Summary of changes

This commit changes the code back to use postgres/tokio_postgres::Config everywhere.

While on it, also do some changes around, as I had to touch this code:

1. Bump some logging from debug to info in the spec apply path. We do not use debug in prod, and it was tricky to understand what was going on with this bug in prod.
2. Refactor configuration concurrency calculation code so it was reusable. Yet, still keep 1 in the case of reconfiguration. The database can be actively used at this moment, so we cannot guarantee that there will be enough spare connection slots, and the underlying code won't handle connection errors properly.
3. Simplify the installed extensions code. It was spawning a blocking task inside async function, which doesn't make much sense. Instead, just have a main sync function and call it with spawn_blocking in the API code -- the only place we need it to be async.
4. Add regression python test to cover this and related problems in the future. Also, add more extensive testing of schema dump and DBs and roles listing API.

Resolves neondatabase/cloud#20869

[ololobus]

I wonder, is the way I did it this time is also OK, because I'm not sure how to switch to postgres::config::Config in all places now. Schema dump is the most problematic, as it just passes connstr to the cli

[github-actions]

6952 tests run: 6644 passed, 0 failed, 308 skipped (full report)

---

Code coverage* (full report)

• functions: 30.6% (7985 of 26064 functions)
• lines: 48.6% (63482 of 130635 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
5b68765 at 2024-11-28T20:17:03.326Z :recycle:}

[tristan957]

is it possible to add a test?

[hlinnaka]

Looks good at high level. Please add test and address all the minor comments though

[hlinnaka]

is it possible to add a test?

+1. The connstr_for_db function would be an ideal candidate for a unit test. Maybe max_service_connections too.

[hlinnaka]

Let's put out the fire first, but let's also consider if this is actually an upstream bug in tokio-postgres, and submit a fix there too.

[ololobus]

+1. The connstr_for_db function would be an ideal candidate for a unit test. Maybe max_service_connections too.

Added a python test instead for now. It fails in main and passes with my changes. There are many places, and I'm afraid that with some future refactoring someone will use a set_path() again in some of the places. +1 for writing more unit tests and doing some further refactoring, but will keep it for further work

Also, this python test checks that we can actually create and configure databases via spec, which I think is good to catch regressions right in the neon test suite

[ololobus]

Let's put out the fire first, but let's also consider if this is actually an upstream bug in tokio-postgres, and submit a fix there too.

@hlinnaka What do you mean by upstream bug in tokio-postgres? We used set_path(), which is a part of Url crate. I think it actually obeys the standard. After switching back to postgres/tokio-postgres::Config usage, it works just fine

[ololobus]

Going to merge when CI is ready

[hlinnaka]

Let's put out the fire first, but let's also consider if this is actually an upstream bug in tokio-postgres, and submit a fix there too.

@hlinnaka What do you mean by upstream bug in tokio-postgres? We used set_path(), which is a part of Url crate. I think it actually obeys the standard. After switching back to postgres/tokio-postgres::Config usage, it works just fine

Oh right, I somehow thought this was in tokio-postgres. Never mind

[hlinnaka]

        let base_url = url::Url::parse("postgres://user@example.com/basedb").unwrap();

        connstr_for_db(&base_url, "foo db").as_str()

returns postgres://user@example.com/foo+db. But at least psql interprets that as a + sign, not as space

[ololobus]

returns postgres://user@example.com/foo+db. But at least psql interprets that as a + sign, not as space

It's still bad. I think I've finally got rid of messing with urls everywhere, so just dropped this helper