compute_ctl: Streamline and Pipeline startup SQL

[MMeent]

Before, compute_ctl didn't have a good registry for what command would run when, depending exclusively on sync code to apply changes. When users have many databases/roles to manage, this step can take a substantial amount of time, breaking assumptions about low (re)start times in other systems.

This commit reduces the time compute_ctl takes to restart when changes must be applied, by making all commands more or less blind writes, and applying these commands in an asynchronous context, only waiting for completion once we know the commands have all been sent.

Additionally, this reduces time spent by batching per-database operations where previously we would create a new SQL connection for every user-database operation we planned to execute.

Problem

Performance of starting compute with 100s of users and 100s of databases is quite suboptimal. This is one way to reduce the pain.

Summary of changes

• Split "what to do" and "do it" for compute_ctl's spec apply with SQL commands
• Rework compute_ctl to use async to apply the SQL changes built in the above system
• Rework compute_ctl even further to batch all operations done in each database, so that we don't needlessly (re)connect to the same database over and over when a large amount of roles was deleted.

Note for CP reviewer: I don't expect much to have changed on your part, as it is mostly data flow changes in the tool itself, with no definition changes on the Compute Spec side.

Checklist before requesting a review

• I have performed a self-review of my code.
• If it is a core feature, I have added thorough tests.
• Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
• If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

• Do not forget to reformat commit message to not include the above checklist

[tristan957]

Looks really nice. Any benchmarks?

[github-actions]

5490 tests run: 5247 passed, 0 failed, 243 skipped (full report)

---

Flaky tests (3)

Postgres 16

• test_create_churn_during_restart: release-x86-64

Postgres 15

• test_isolation[None]: release-arm64

Postgres 14

• test_pull_timeline[True]: release-arm64

Code coverage* (full report)

• functions: 31.8% (7930 of 24899 functions)
• lines: 49.4% (62829 of 127211 lines)

* collected from Rust tests only

---

_{The comment gets automatically updated with the latest test results
d796cc2 at 2024-11-15T15:27:42.216Z :recycle:}

[clipperhouse]

Is there a control plane aspect here? Not obvious to me if so.

[tristan957]

Is there a control plane aspect here? Not obvious to me if so.

Not that I'm aware of. I'm betting you got the ping due to the compute spec reading code.

[knizhnik]

I have general comment not directly related with this PR and which may be should be addressed separately. Right now we applying all this statements concurrently with user statements. In other words, at the moment of executing this statements, database already accepts user's connections and so can execute user's queries.

It may lead to some unexpected and confusing behaviour: for example user may observe old version of Neon extension, because it was not yet upgraded. I actually reproduced such problem myself in one of the tests.

So I wonder if we could (should?) somehow prevent user from accessing this database instance before compute_ctl completes node update? Not sure what is the bestway tp do it:

• lock some catalog table
• configure different port
• somehow block connections at proxy level
  ...

[knizhnik]

I have even more radical proposal than @tristan957 suggested (use include_str!): can we specify all statements which should be executed in some file and read and apply them from this file?

Yes, I certainly understand that we need some parameters for this statements (roe names database names,...). Its can be solved in multiple ways: placeholders for parameters, per-roles and per-database files (so per-role file contains list of statements which should be applied for each role and, for example {role} should be substituted with role name. In the same way - per-database file contains statement which should be executed for each database and {db} should be substituted with database name.

[MMeent]

I don't think that's easy, and that it'll take much more effort, especially as some statements need to be executed N_DATABASES times, some are conditional on the database (not) being owned by a user, etc. There is complex logic that I prefer to apply before starting/checking the SQL session.

Maybe it can be done in a future iteration, but I take "done" over "perfect".

[MMeent]

Right now we applying all this statements concurrently with user statements. In other words, at the moment of executing this statements, database already accepts user's connections and so can execute user's queries.

Don't we only start allowing connections from outside the container after we've transitioned to the Running state? During startup we're the sole user of the database, right? At least, that's what's been diagnosed as causing the start_compute timeouts when the spec has many databases and roles...

For apply_config, you are correct, but as that's always on a Running endpoint I don't think we should block user connections during that phase.

[F]or example user may observe old version of Neon extension, because it was not yet upgraded. I actually reproduced such problem myself in one of the tests.

I think that's more related to neon_local 's (default) lack of applying config changes during startup, as endpoint.json's skip_pg_catalog_updates field is default true, thus never applying the code being modified here.

[tristan957]

s/granting/grant will match the verb tense of the other comments

[MMeent]

The other comments have mixed tenses, with this verb tense actually being slightly more common, and much more common for the anon steps:

• dropping x1
• renaming x2
• creating x2 (1x anon)
• clearing x1
• deleting x1
• changing x1
• initializing x1 (1x anon)
• granting x4 (4x anon)
• change x1 (1x anon)
• add x1
• install x1
• make x1
• alter x1
• update x1

[MMeent]

Performace review

No-op sync of 1024 roles and 1024 databases, on my WSL box (16vCPU, enough RAM/SSD):

• main @ 4534f5c: took 5m30s after first connection to user database (5m38s incl. all overheads), ~10-20% max across all cores.

• This PR @ c20e452: took 15s after first connection to user database (21s incl. all overheads), 100% across all cores.

Config

1. postgresql.conf updated with these settings:

   shared_buffers=128MB
   neon.max_file_cache_size=128MB
   neon.file_cache_size_limit=128MB
   neon.file_cache_path='/tmp/pg_cache'
   log_connections=on
   log_statement=all
   

2. I'd updated the code of neon_cloud locally to generate num_dbs bogus databases and num_roles bogus roles.

First I let the system apply the changes, then shut the database down. Finally, measure start time with nothing to be done.

Future work

Many of the modifying queries generated for the system database (e.g. role- and database creation, deletions) can be parallellized within their respective phases, too. Maybe we can look into that at a later date, as creating 1k databases sequentially takes a lot of time (and are thus likely to hit start_compute timeouts).