Hi,
On Mon, Jul 08, 2024 at 03:49:34PM +0900, Michael Paquier wrote:
> On Mon, Jul 08, 2024 at 06:39:56AM +0000, Bertrand Drouvot wrote:
> > + for (int kind = PGSTAT_KIND_FIRST_VALID; kind <= PGSTAT_KIND_LAST; kind++)
> > + {
> > + char *ptr;
> > + const PgStat_KindInfo *info = pgstat_get_kind_info(kind);
> >
> > I wonder if we could avoid going through stats that are not fixed ones. What about
> > doing something like?
> > Same would apply for the read part added in 9004abf6206e.
>
> This becomes more relevant when the custom stats are added, as this
> performs a scan across the full range of IDs supported. So this
> choice is here for consistency, and to ease the pluggability.
Gotcha.
>
> > 2 ===
> >
> > + pgstat_build_snapshot_fixed(kind);
> > + ptr = ((char *) &pgStatLocal.snapshot) + info->snapshot_ctl_off;
> > + write_chunk(fpout, ptr, info->shared_data_len);
> >
> > I think that using "shared_data_len" is confusing here (was not the case in the
> > context of 9004abf6206e). I mean it is perfectly correct but the wording "shared"
> > looks weird to me when being used here. What it really is, is the size of the
> > stats. What about renaming shared_data_len with stats_data_len?
>
> It is the stats data associated to a shared entry. I think that's OK,
> but perhaps I'm just used to it as I've been staring at this area for
> days.
Yeah, what I meant to say is that one could think for example that's the
PgStatShared_Archiver size while in fact it's the PgStat_ArchiverStats size.
I think it's more confusing when writing the stats. Here we are manipulating
"snapshot" and "snapshot" offsets. It was not that confusing when reading as we
are manipulating "shmem" and "shared" offsets.
As I said, the code is fully correct, that's just the wording here that sounds
weird to me in the "snapshot" context.
Except the above (which is just a Nit), 0001 LGTM.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
