Thanks for taking a look! On Mon, Jan 20, 2025 at 12:53 PM Jacob Champion <jacob.champ...@enterprisedb.com> wrote: > > On Mon, Jan 20, 2025 at 7:01 AM Bertrand Drouvot > <bertranddrouvot...@gmail.com> wrote: > > Though time changes are "rare", given the fact that those metrics could > > provide > > "inaccurate" measurements during that particular moment (time change) then > > it > > might be worth considering instr_time instead for this particular metric. > > +1, I think a CLOCK_MONOTONIC source should be used for this if we've got it.
Done in v3 (see [1]). > For the EXEC_BACKEND case (which, to be honest, I don't know much > about), I originally wondered if the fork_duration should include any > of the shared memory manipulations or library reloads that are done to > match Unix behavior. But I think I prefer the way the patch does it. You mean if the EXEC_BACKEND not defined version should calculate the end of fork_duration basically at the end of postmaster_child_launch()? > Can the current timestamp be recorded right at the beginning of > SubPostmasterMain(), to avoid counting the time setting up GUCs and > reading the variables file, or do we have to wait? We actually don't have the startup data until after we read_backend_variables(), so I did it as soon as I could after that. You are right that this will include timing from some extra steps. But, some of these steps are overhead unique to the slow process of "forking" a backend when EXEC_BACKEND is defined anyway, so it probably makes sense for them to be included in the timing of "fork" for these backends. > nit: conn_timing is currently declared in the "interrupts and crit > section" part of miscadmin.h; should it be moved down to the > general-purpose globals? Whoops, it would help if I read comments and stuff. Thanks! Fixed in v3 in [1]. - Melanie [1] https://www.postgresql.org/message-id/CAAKRu_YrNsA7-v5L9d318XZu%2BtPqcxp%2BctCGy2EGYrSt69ON%3DA%40mail.gmail.com
