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