Hi, On Tue, Dec 17, 2024 at 03:26:02PM +0900, Michael Paquier wrote: > On Mon, Dec 16, 2024 at 03:42:04PM +0000, Bertrand Drouvot wrote: > >> Perhaps there's an argument for an entirely separate > >> callback that would run before pgstat is plugged off, like a new > >> before_shmem_exit() callback registered after the entry is created? > > > > As the ProcKill() is run in shmem_exit() (and so after before_shmem_exit()), > > I think that the way we currently drop the backend stats entry is fine > > (unless > > I miss something about your concern). > > Looking closely at this one, I think that you're right as long as the > shutdown callback is registered before the entry is created.
Yeah, which is the case. > Anyway, I have put my hands on v9. A couple of notes, while hacking > through it. See v9-0002 for the parts I have modified, that applies > on top of your v9-0001. Thanks! > You may have noticed fee2b3ea2ecdg, which led me to fix two comments > as these paths are not related to only database objects. Yeap ;-) > Added more documentation, tweaked quite a bit the comments, a bit less > the docs (no need for the two linkends) and applied an indentation. Looks good to me. > pg_stat_get_backend_io() can be simpler, and does not need the loop > with the extra LocalPgBackendStatus. It is possible to call once > pgstat_get_beentry_by_proc_number() and retrieve the PID and the > bktype for the two sanity checks we want to do on them. Looked at the changes you've done and agree that's simpler. > s/pgstat_fetch_proc_stat_io/pgstat_fetch_stat_backend/. Not sure about this one as being called in pg_stat_get_backend_io(). > s/pgstat_create_backend_stat/pgstat_create_backend/. LGTM. > I've been wondering for quite a bit about PgStat_BackendPendingIO and > PgStat_PendingIO, and concluded to define both in pgstat.h, with the > former being defined based on the latter to keep the dependency > between both at the same place. That's fine by me. Also: === 1 the changes in pgstat_flush_backend() make sense to me. === 2 - * Create the backend statistics entry for procnum. + * Create backend statistics entry for proc number. Yeah, "proc number" looks better as already used in multiple places. === 3 the changes in stats.sql look good to me. Having said that, v9-0002 looks good to me (except the pgstat_fetch_proc_stat_io renaming). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
