On Tue, Apr 02, 2019 at 07:43:12AM +0200, Julien Rouhaud wrote:
> On Tue, Apr 2, 2019 at 6:56 AM Michael Paquier <michael@paquier.xyz> wrote:
>> One thing which is not
>> proposed on this patch, and I am fine with it as a first draft, is
>> that we don't have any information about the broken block number and
>> the file involved. My gut tells me that we'd want a separate view,
>> like pg_stat_checksums_details with one tuple per (dboid, rel, fork,
>> blck) to be complete. But that's just for future work.
>
> That could indeed be nice.
Actually, backpedaling on this one... pg_stat_checksums_details may
be a bad idea as we could finish with one row per broken block. If
a corruption is spreading quickly, pgstat would not be able to sustain
that amount of objects. Having pg_stat_checksums would allow us to
plugin more data easily based on the last failure state:
- last relid of failure
- last fork type of failure
- last block number of failure.
Not saying to do that now, but having that in pg_stat_database does
not seem very natural to me. And on top of that we would have an
extra row full of NULLs for shared objects in pg_stat_database if we
adopt the unique view approach... I find that rather ugly.
>> No need for two ifs here. What about just that?
>> if (NULL)
>> PG_RETURN_NULL();
>> else
>> PG_RETURN_TIMESTAMPTZ(last_checksum_failure);
>
> I do agree, but this is done like this everywhere in pgstatfuncs.c, so
> I think it's better to keep it as-is for consistency.
Okay, this is not an issue for me.
The patch looks fine to me as-is. Let's see what Magnus or others have to
say about it. I can take care of this open item if necessary but
that's not my commit so I'd rather not step on Magnus' toes.
--
Michael