neon icon indicating copy to clipboard operation
neon copied to clipboard

Clean up global variables in safekeeper code

Open petuhovskiy opened this issue 1 year ago • 3 comments

We have at least several places in the code where global variables are used:

  • some places call GlobalTimelines::<funcname> to lookup timelines
  • static TIMELINES_STATE stores all timelines as a global variable
  • static REMOTE_STORAGE stores config for remote storage (e.g. S3)

To resolve the issue, we should replace GlobalTimelines:: calls with Arc<GlobalTimelines> struct. It will contain the hashmap of all timelines (the same way TIMELINES_STATE is working now). It should be passed around to the code that needs access to all timelines, instead of everything relying on global access through GlobalTimelines.

  • [x] remove all GlobalTimelines:: calls
  • [x] remove static TIMELINES_STATE

REMOTE_STORAGE too should be passed around as a Arc<RemoteStorage>. Each task that requires access to remote storage should get Arc<RemoteStorage> as a dependency during initialization.

  • [x] remove static REMOTE_STORAGE

If some background task requires access to all global state (remote storage, timeline, config), we can bundle them together inside Arc<SafekeeperApp>, which is easier to carry around.

Also, currently struct GlobalTimelinesState contains two fields that are part of the global state:

conf: Option<SafeKeeperConf>,
broker_active_set: Arc<TimelinesSet>,

Currently conf is required in many places and usually it just copied from global state on demand. The config is loaded once on startup and never changes. This is wasteful, we should replace it with Arc<SafeKeeperConf> and always carry Arc<..> instead of SafeKeeperConf.

  • [x] replace SafeKeeperConf usages with Arc<SafeKeeperConf> everywhere

Why?

To improve testing, to allow to run more than one safekeeper instance in a single process. To simplify creating new dependencies without introducing global variables and special getters.

petuhovskiy avatar Jun 28 '24 13:06 petuhovskiy

@petuhovskiy can I take up this issue ?

fowlerlee avatar Sep 12 '24 18:09 fowlerlee

@petuhovskiy can I take up this issue ?

@fowlerlee Sure, no one is working on this right now. You can start and open a PR, thanks!

petuhovskiy avatar Sep 13 '24 09:09 petuhovskiy

We just merged

  • https://github.com/neondatabase/neon/pull/10179

@petuhovskiy anything left on this issue?

problame avatar May 16 '25 12:05 problame

I tried to grep static in rust safekeeper code, and I don't see anything left, except metrics and jemalloc. So I think we can close this issue.

petuhovskiy avatar May 16 '25 14:05 petuhovskiy