Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system - Mailing list pgsql-hackers

Tomas Vondra wrote:

> diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
> index be3adf1..4ec485e 100644
> --- a/src/backend/postmaster/pgstat.c
> +++ b/src/backend/postmaster/pgstat.c
> @@ -64,10 +64,14 @@
>
>  /* ----------
>   * Paths for the statistics files (relative to installation's $PGDATA).
> + * Permanent and temprorary, global and per-database files.

Note typo in the line above.

> -#define PGSTAT_STAT_PERMANENT_FILENAME        "global/pgstat.stat"
> -#define PGSTAT_STAT_PERMANENT_TMPFILE        "global/pgstat.tmp"
> +#define PGSTAT_STAT_PERMANENT_DIRECTORY        "stat"
> +#define PGSTAT_STAT_PERMANENT_FILENAME        "stat/global.stat"
> +#define PGSTAT_STAT_PERMANENT_TMPFILE        "stat/global.tmp"
> +#define PGSTAT_STAT_PERMANENT_DB_FILENAME    "stat/%d.stat"
> +#define PGSTAT_STAT_PERMANENT_DB_TMPFILE    "stat/%d.tmp"

> +char       *pgstat_stat_directory = NULL;
>  char       *pgstat_stat_filename = NULL;
>  char       *pgstat_stat_tmpname = NULL;
> +char       *pgstat_stat_db_filename = NULL;
> +char       *pgstat_stat_db_tmpname = NULL;

I don't like the quoted parts very much; it seems awkward to have the
snprintf patterns in one place and have them be used in very distant
places.  Is there a way to improve that?  Also, if I understand clearly,
the pgstat_stat_db_filename value needs to be an snprintf pattern too,
right?  What if it doesn't contain the required % specifier?

Also, if you can filter this through pgindent, that would be best.  Make
sure to add DBWriteRequest to src/tools/pgindent/typedefs_list.

> +        /*
> +         * There's no request for this DB yet, so lets create it (allocate a
> +         * space for it, set the values).
> +         */
> +        if (last_statrequests == NULL)
> +            last_statrequests = palloc(sizeof(DBWriteRequest));
> +        else
> +            last_statrequests = repalloc(last_statrequests,
> +                                (num_statrequests + 1)*sizeof(DBWriteRequest));
> +
> +        last_statrequests[num_statrequests].databaseid = msg->databaseid;
> +        last_statrequests[num_statrequests].request_time = msg->clock_time;
> +        num_statrequests += 1;

Having to repalloc this array each time seems wrong.  Would a list
instead of an array help?  see ilist.c/h; I vote for a dlist because you
can easily delete elements from the middle of it, if required (I think
you'd need that.)

> +        char db_statfile[strlen(pgstat_stat_db_filename) + 11];
> +        snprintf(db_statfile, strlen(pgstat_stat_db_filename) + 11,
> +                 pgstat_stat_filename, dbentry->databaseid);

This pattern seems rather frequent.  Can we use a macro or similar here?
Encapsulating the "11" better would be good.  Magic numbers are evil.

> diff --git a/src/include/pgstat.h b/src/include/pgstat.h
> index 613c1c2..b3467d2 100644
> --- a/src/include/pgstat.h
> +++ b/src/include/pgstat.h
> @@ -205,6 +205,7 @@ typedef struct PgStat_MsgInquiry
>      PgStat_MsgHdr m_hdr;
>      TimestampTz clock_time;        /* observed local clock time */
>      TimestampTz cutoff_time;    /* minimum acceptable file timestamp */
> +    Oid            databaseid;        /* requested DB (InvalidOid => all DBs) */
>  } PgStat_MsgInquiry;

Do we need to support the case that somebody requests stuff from the
"shared" DB?  IIRC that's what InvalidOid means in pgstat ...

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services