Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system

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