pgrx
pgrx copied to clipboard
pgcentralfoundation
Implement dynamic background worker registration
@workingjubilee I've added the first draft of the test; please let me know what you think.
@workingjubilee I've updated this PR to be based on the latest develop and removed shmem startup hook as it doesn't make sense in the dynamic registration scenario.
I've considered changing the API for the startup hook to make it impossible to use dynamic registration if the hook is provided, but for now, I decided to keep it as is unless you want me to. Something like:
pub struct BackgroundWorkerBuilder<const Dynamic: bool = true> { .... }
pub fn enable_shmem_access(mut self: Self) -> Self { ... }
pub fn shmem_startup_hook(mut self: Self, unsafe extern "C" fn()) -> BackgroundWorkerBuilder<false> { ... }
impl BackgroundWorkerBuilder<true> {
pub fn load_dynamic(self: Self) -> BackgroundWorkerHandle { ... }
}
I think this looks good now. Let’s have @workingjubilee confirm next week before we merge.
Thanks for the work, @yrashk!
A design question that came up in my mind that I don't know enough about BackgroundWorkers to remark on: Can a BackgroundWorker, if registered and loaded dynamically, move to any other lifecycle point? Or does it always progress in a fairly linear fashion (from "not yet started" to "active" to "ended")? In other words: is this enum truly an enum or is this trying to represent a state machine? We can enforce a much more rigid progression if so, if we want to. This would make the design more complex but would also make it harder to misuse the interface.
A design question that came up in my mind that I don't know enough about BackgroundWorkers to remark on: Can a BackgroundWorker, if registered and loaded dynamically, move to any other lifecycle point? Or does it always progress in a fairly linear fashion (from "not yet started" to "active" to "ended")? In other words: is this enum truly an enum or is this trying to represent a state machine? We can enforce a much more rigid progression if so, if we want to. This would make the design more complex but would also make it harder to misuse the interface.
I can't say I am an expert in the intricacies of PostgreSQL. Reading this:
typedef enum BgwHandleStatus { BGWH_STARTED, /* worker is running / BGWH_NOT_YET_STARTED, / worker hasn't been started yet / BGWH_STOPPED, / worker has exited / BGWH_POSTMASTER_DIED / postmaster died; worker status unclear */ } BgwHandleStatus;
and the documentation suggests that it is linear. not yet started -> started -> stopped and unclear.
What kind of state machine did you have in mind that it might represent?
I figured something like that would be the case. I was wondering if using a generic-enforced state machine (as in https://hoverbear.org/blog/rust-state-machine-pattern/ 3.3 and 3.4) might be better as a model (with the obvious clause that we can go from the "ending" back to the start again if we restart the worker, I think?) or if that would be too cumbersome. We kind of already have that half-way here.
I figured something like that would be the case. I was wondering if using a generic-enforced state machine (as in https://hoverbear.org/blog/rust-state-machine-pattern/ 3.3 and 3.4) might be better as a model (with the obvious clause that we can go from the "ending" back to the start again if we restart the worker, I think?) or if that would be too cumbersome. We kind of already have that half-way here.
My current feeling about this is that it is a) an overkill for now b) Postgres handles restarts itself so one can't really go "stopped->not_started->started" on their own.
I propose to break this out into a separate discussion. I am more than willing to commit some time to improving these small details in followups.
Hm. I think it's fine to defer some of this to followups and I think we'll need a little time experimenting with this before we can be sure of what the right API is, anyways, so a major break on it is inevitable, but in the general case we should try to tend towards stricter (within reason) APIs -> less strict APIs. It's harder to take back relaxations of the rules when you've already smoothed a given path for something.
I'm 100% on this with you. Good APIs take time and stricter are better, but only when not taken to an extreme. It should still be usable :)
I'll be happy to help improving this over time.
Do we have anything blocking this PR that I can address before merging as a first version?
On Tue., Oct. 18, 2022, 7:45 p.m. Jubilee, @.***> wrote:
Hm. I think it's fine to defer some of this to followups and I think we'll need a little time experimenting with this before we can be sure of what the right API is, anyways, so a major break on it is inevitable, but in the general case we should try to tend towards stricter (within reason) APIs -> less strict APIs. It's harder to take back relaxations of the rules when you've already smoothed a given path for something.
— Reply to this email directly, view it on GitHub https://github.com/tcdi/pgx/pull/710#issuecomment-1283329018, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAADRCCDHW5XBZOO2JMA3DWD5OGNANCNFSM6AAAAAAQVNP5L4 . You are receiving this because you were mentioned.Message ID: @.***>
@workingjubilee I've addressed the issue with TerminatingDynamicBackgroundWorker::wait_for_shutdown signature as we've discussed.