On Mon, Mar 15, 2021 at 10:56 PM Andres Freund <and...@anarazel.de> wrote: > I did roughly the first steps of the split as I had outlined. I moved: > > 1) wait event related functions into utils/activity/wait_event.c / > wait_event.h > > 2) "backend status" functionality (PgBackendStatus stuff) into > utils/activity/backend_status.c > > 3) "progress" related functionality into > utils/activity/backend_progress.c
In general, I like this. I'm not too sure about the names. I realize you don't want to have functions called status.c and progress.c, because that's awful generic, but now you have backend_progress.c, backend_status.c, and wait_event.c, which makes the last one look a little strange. Maybe command_progress.c instead of backend_progress.c? > I think 1 and 2 are good (albeit in need of further polish). I'm a bit > less sure about 3: > - There's dependency from backend_status.h to backend_progress.h, > because it PGSTAT_NUM_PROGRESS_PARAM etc. That doesn't seem like a catastrophe. > - it's a fairly small amount of code But it's not bad to have it separate. > - there's potential for confusion, because there's also > include/commands/progress.h That could be merged, perhaps. I think I only created that because I didn't want to jam too much stuff into pgstat.h. But if it has its own header then jamming some more stuff in there seems more OK. > - I'm inclined to leave pgstat_report_{activity, tmpfile, appname, > timestamp, ..} alone naming-wise, but to rename pgstat_bestart() to > something like pgbestat_start()? I'd probably rename them e.g. command_progress_start(), comand_progress_update_param(), etc. > - I've not gone through all the files that could now remove pgstat.h, > replacing it with wait_event.h - I'm thinking it might be worth > waiting till just after code freeze with that (there'll new additions, > and it's likely to cause conflicts)? Don't care. > - backend_status.h needs miscadmin.h, due to BackendType. Imo that's a > header we should try to avoid exposing in headers if possible. But > right now there's no good place to move BackendType to. So I'd let > this slide for now. Why the concern? miscadmin.h is extremely widely-included already. Maybe it should be broken up into pieces so that we're not including so MUCH stuff in a zillion places, but the header that contains the definition of CHECK_FOR_INTERRUPTS() is always going to be needed in a ton of spots. Honestly, I wonder why we don't just put that part in postgres.h. If you're writing any significant amount of code and you don't have at least one CHECK_FOR_INTERRUPTS() in there, you're probably doing it wrong. -- Robert Haas EDB: http://www.enterprisedb.com