Re: Checksum errors in pg_stat_database

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