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

On 1.2.2013 17:19, Alvaro Herrera wrote:
> 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

I don't see that as particularly awkward, but that's a matter of taste.
I still see that as a bunch of constants that are sprintf patterns at
the same time.

> 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?

Ummmm, yes - it needs to be a pattern too, but the user specifies the
directory (stats_temp_directory) and this is used to derive all the
other values - see assign_pgstat_temp_directory() in guc.c.

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

Will do.

>
>> + /*
>> + * 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.)

Thanks. I'm not very familiar with the list interface, so I've used
plain array. But yes, there are better ways than doing repalloc all the
time.

>
>> + 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.

Yes, this needs to be cleaned / improved.

>> 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 ...

Frankly, I don't know, but I guess we do because it was in the original
code, and there are such inquiries right after the database starts
(that's why I had to add pgstat_write_db_dummyfile).

Tomas