I took a look at the earlier comments. > + /* Read and verify the hash key */ > + if (!pgstat_read_chunk(fd, (void *) key, sizeof(PgStat_HashKey))) > + return; > [...] > + /* Write the hash key to identify this entry */ > + pgstat_write_chunk(fd, (void *) key, sizeof(PgStat_HashKey));
> I am puzzled by this part of 0002. Why are you overwriting the key
> once after loading it from the main pgstats file?
Yes, this is not necessary. I removed it.
> I would have done it in a slightly different way, I guess, with the data
> stored on disk in the main pgstats file including an offset to know
> where to search in the secondary file.
that's a much better approach and the pattern we would want to use
going forward. Since this does not require we read the entires
back in the same order as written, so it's much more flexible.
I not did this in the test module.
> Perhaps the callback in the module for end_extra_stats should use a
> switch based on PgStat_StatsFileOp. Minor point.
Agree. Done.
>> * In the current path, pgstat performs its own write, then call
>> * callbacks. What about if a callback fails? Will that leave pgstat
>> * in a stale state?
> For the write state, end_extra_stats() would take care of that. It
> depends on what kind of errors you would need to deal with, but as
> proposed the patch would offer the same level of protection for the
> writes of the stats, where we'd check for an error on the fd saved by
> an extension for an extra file.
> I think that you have a fair point about the stats read path though,
> shouldn't we make the callback from_serialized_extra_stats() return a
> state to be able to trigger a full-scale cleanup, at least?
In this case, even if the callback does not return a state, the cleanup
will eventually occur at the end of the read, see
```
done:
/* First, cleanup the main stats file, PGSTAT_STAT_PERMANENT_FILENAME */
FreeFile(fpin);
elog(DEBUG2, "removing permanent stats file \"%s\"", statfile);
unlink(statfile);
/* Let each stats kind run its cleanup callback, if it provides one */
for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
{
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
if (kind_info && kind_info->end_extra_stats)
kind_info->end_extra_stats(STATS_READ);
}
```
However, this could also mean some entries could be read back correctly, and
others not, so maybe it's not such a good idea. So, I did what is suggested
and allow the callback to return a bool which will raise an error and trigger
the cleanup code.
> + if (pgstat_is_kind_custom(key.kind) &&
> kind_info->from_serialized_extra_stats)
> + kind_info->from_serialized_extra_stats(&key, header, fpin);
> [...]
> + if (pgstat_is_kind_custom(ps->key.kind) &&
> kind_info->to_serialized_extra_stats)
> + kind_info->to_serialized_extra_stats(&ps->key, shstats, fpout);
> These restrictions based on custom kinds do not seem mandatory.
> Why not allowing built-in kinds the same set of operations?
No good reason not to. In fact, maybe a follow-up will be to move the
replslot to this infrastructure and remove reliance on PGSTAT_FILE_ENTRY_NAME.
attached is the v4 patch set which includes:
0001 - which is just moving the tests out of injection points into a new
test module. This is similar to v3 [0].
0002 - Is the code changes to implement the callbacks and the necessary
tests in the new test module.
[0]
https://www.postgresql.org/message-id/CAA5RZ0sG2RUKg%3DOLY%2B6-e4q%3DX9rsLfK3pKn03d%3DRZQppEDR%3DBg%40mail.gmail.com
--
Sami Imseih
Amazon Web Services (AWS)
v4-0002-Allow-cumulative-statistics-to-serialize-auxiliar.patch
Description: Binary data
v4-0001-Move-custom-stats-tests-from-injection_points-to-.patch
Description: Binary data
