Обсуждение: Checksum errors in pg_stat_database

On Fri, Jan 11, 2019 at 5:21 AM Magnus Hagander <magnus@hagander.net> wrote:
> Would it make sense to add a column to pg_stat_database showing the total number of checksum errors that have
occurredin a database?
 
>
> It's really a ">1 means it's bad", but it's a lot easier to monitor that in the statistics views, and given how much
alot of people set their systems out to log, it's far too easy to miss individual checksum matches in the logs.
 
>
> If we track it at the database level, I don't think the overhead of adding one more counter would be very high
either.

It's probably not the idea way to track it.  If you have a terabyte or
fifty of data, and you see that you have some checksum failures, good
luck finding the offending blocks.

But I'm tentatively in favor of your proposal anyway, because it's
pretty simple and cheap and might help people, and doing something
noticeably better is probably annoyingly complicated.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 1/11/19 7:40 PM, Robert Haas wrote:
> On Fri, Jan 11, 2019 at 5:21 AM Magnus Hagander <magnus@hagander.net> wrote:
>> Would it make sense to add a column to pg_stat_database showing
>> the total number of checksum errors that have occurred in a database?
>> 
>> It's really a ">1 means it's bad", but it's a lot easier to monitor
>> that in the statistics views, and given how much a lot of people
>> set their systems out to log, it's far too easy to miss individual
>> checksum matches in the logs.
>>
>> If we track it at the database level, I don't think the overhead 
>> of adding one more counter would be very high either.
> 
> It's probably not the idea way to track it.  If you have a terabyte or
> fifty of data, and you see that you have some checksum failures, good
> luck finding the offending blocks.
> 

Isn't that somewhat similar to deadlocks, which we also track in
pg_stat_database? The number of deadlocks is rather useless on it's own,
you need to dive into the server log to find the details. Same for
checksum errors.

> But I'm tentatively in favor of your proposal anyway, because it's
> pretty simple and cheap and might help people, and doing something
> noticeably better is probably annoyingly complicated.
> 

+1

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 1/11/19 10:25 PM, Magnus Hagander wrote:
> On Fri, Jan 11, 2019 at 9:20 PM Tomas Vondra 
>     On 1/11/19 7:40 PM, Robert Haas wrote:
>      > But I'm tentatively in favor of your proposal anyway, because it's
>      > pretty simple and cheap and might help people, and doing something
>      > noticeably better is probably annoyingly complicated.
>      >
> 
>     +1
> 
> Yeah, that's the idea behind it -- it's cheap, and an 
> early-warning-indicator.

+1

-- 
-David
david@pgmasters.net

On Fri, Feb 22, 2019 at 3:01 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> PFA is a patch to do this.

+void
+pgstat_report_checksum_failure(void)
+{
+   PgStat_MsgDeadlock msg;

I think that you meant PgStat_MsgChecksumFailure :)

+/* ----------
+ * pgstat_recv_checksum_failure() -
+ *
+ * Process a DEADLOCK message.
+ * ----------

same here

Otherwise LGTM.

On Fri, Feb 22, 2019 at 3:25 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Fri, Feb 22, 2019 at 3:23 PM Magnus Hagander <magnus@hagander.net> wrote:
>>
>>
>>
>> On Fri, Feb 22, 2019 at 3:16 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>>>
>>> On Fri, Feb 22, 2019 at 3:01 PM Magnus Hagander <magnus@hagander.net> wrote:
>>> >
>>> > PFA is a patch to do this.
>>>
>>> +void
>>> +pgstat_report_checksum_failure(void)
>>> +{
>>> +   PgStat_MsgDeadlock msg;
>>>
>>> I think that you meant PgStat_MsgChecksumFailure :)
>>>
>>> +/* ----------
>>> + * pgstat_recv_checksum_failure() -
>>> + *
>>> + * Process a DEADLOCK message.
>>> + * ----------
>>>
>>> same here
>>>
>>> Otherwise LGTM.
>>
>>
>> Haha, damit, that's embarassing. You can probably guess where I copy/pasted from :)

heh :)

>>
>
> And of course, then I forgot to attach the new file.

It all looks fine.  One minor nitpicking issue I just noticed, there's
an extra space there:

+   dbentry->n_checksum_failures ++;

I'm marking it as ready for committer!

On Fri, Feb 22, 2019 at 3:01 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> It tracks things that happen in the general backends. Possibly we should also consider counting the errors actually
foundwhen running base backups? OTOH, that part of the code doesn't really track things like databases (as it operates
juston the raw data directory underneath), so that implementation would definitely not be as clean... 

Sorry I just realized that I totally forgot this part of the thread.

While it's true that we operate on raw directory, I see that sendDir()
already setup a isDbDir var, and if this is true lastDir should
contain the oid of the underlying database.  Wouldn't it be enough to
call sendFile() using this, something like (untested):

if (!sizeonly)
- sent = sendFile(pathbuf, pathbuf + basepathlen + 1, &statbuf, true);
+ sent = sendFile(pathbuf, pathbuf + basepathlen + 1, &statbuf, true,
isDbDir ? pg_atoi(lastDir+1, 4) : InvalidOid);

and accordingly report any checksum error from sendFile()?

On Mon, Mar 4, 2019 at 8:31 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Fri, Feb 22, 2019 at 3:01 PM Magnus Hagander <magnus@hagander.net> wrote:
> >
> > It tracks things that happen in the general backends. Possibly we should also consider counting the errors actually
foundwhen running base backups? OTOH, that part of the code doesn't really track things like databases (as it operates
juston the raw data directory underneath), so that implementation would definitely not be as clean... 
>
> Sorry I just realized that I totally forgot this part of the thread.
>
> While it's true that we operate on raw directory, I see that sendDir()
> already setup a isDbDir var, and if this is true lastDir should
> contain the oid of the underlying database.  Wouldn't it be enough to
> call sendFile() using this, something like (untested):
>
> if (!sizeonly)
> - sent = sendFile(pathbuf, pathbuf + basepathlen + 1, &statbuf, true);
> + sent = sendFile(pathbuf, pathbuf + basepathlen + 1, &statbuf, true,
> isDbDir ? pg_atoi(lastDir+1, 4) : InvalidOid);
>
> and accordingly report any checksum error from sendFile()?

So this seem to work just fine without adding much code.  PFA v3 of
Magnus' patch including error reporting for BASE_BACKUP.

On Sat, Mar 9, 2019 at 12:35 AM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Mon, Mar 4, 2019 at 11:31 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>>
>> On Fri, Feb 22, 2019 at 3:01 PM Magnus Hagander <magnus@hagander.net> wrote:
>> >
>> > It tracks things that happen in the general backends. Possibly we should also consider counting the errors
actuallyfound when running base backups? OTOH, that part of the code doesn't really track things like databases (as it
operatesjust on the raw data directory underneath), so that implementation would definitely not be as clean... 
>>
>> Sorry I just realized that I totally forgot this part of the thread.
>>
>> While it's true that we operate on raw directory, I see that sendDir()
>> already setup a isDbDir var, and if this is true lastDir should
>> contain the oid of the underlying database.  Wouldn't it be enough to
>> call sendFile() using this, something like (untested):
>>
>> if (!sizeonly)
>> - sent = sendFile(pathbuf, pathbuf + basepathlen + 1, &statbuf, true);
>> + sent = sendFile(pathbuf, pathbuf + basepathlen + 1, &statbuf, true,
>> isDbDir ? pg_atoi(lastDir+1, 4) : InvalidOid);
>>
>> and accordingly report any checksum error from sendFile()?
>
>
> That seems it was easy enough. PFA an updated patch that does this, and also rebased so it doesn't conflict on oid.
>
> (yes, while moving this from draft to publish after lunch, I realized that you put a patch togerher for about the
same.So let's merge it) 

Thanks!  Our implementations are quite similar, so I'm fine with most
of the changes :) I'm just not sure about having two distinct
functions for reporting failures, given that there's only one caller
for each.  On the other hand it avoids to include miscadmin.h in
bufpage.c.

That's just a detail, so I'm marking it (again) as ready for committer!

On Sat, Mar 9, 2019 at 9:34 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Sat, Mar 9, 2019 at 12:35 AM Magnus Hagander <magnus@hagander.net> wrote:
> >
> > On Mon, Mar 4, 2019 at 11:31 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >>
> >> On Fri, Feb 22, 2019 at 3:01 PM Magnus Hagander <magnus@hagander.net> wrote:
> >> >
> >> > It tracks things that happen in the general backends. Possibly we should also consider counting the errors
actuallyfound when running base backups? OTOH, that part of the code doesn't really track things like databases (as it
operatesjust on the raw data directory underneath), so that implementation would definitely not be as clean... 
> >>
> >> Sorry I just realized that I totally forgot this part of the thread.
> >>
> >> While it's true that we operate on raw directory, I see that sendDir()
> >> already setup a isDbDir var, and if this is true lastDir should
> >> contain the oid of the underlying database.  Wouldn't it be enough to
> >> call sendFile() using this, something like (untested):
> >>
> >> if (!sizeonly)
> >> - sent = sendFile(pathbuf, pathbuf + basepathlen + 1, &statbuf, true);
> >> + sent = sendFile(pathbuf, pathbuf + basepathlen + 1, &statbuf, true,
> >> isDbDir ? pg_atoi(lastDir+1, 4) : InvalidOid);
> >>
> >> and accordingly report any checksum error from sendFile()?
> >
> > That seems it was easy enough. PFA an updated patch that does this, and also rebased so it doesn't conflict on oid.
> >

Sorry, I have again new comments after a little bit more thinking.
I'm wondering if we can do something about shared objects while we're
at it.  They don't belong to any database, so it's a little bit
orthogonal to this proposal, but it seems quite important to track
error on those too!

What about adding a new field in PgStat_GlobalStats for that?  We can
use the same lastDir to easily detect such objects and slightly adapt
sendFile again, which seems quite straightforward.

On Sat, Mar 9, 2019 at 7:48 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Sat, Mar 9, 2019 at 12:33 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>>
>> Thanks!  Our implementations are quite similar, so I'm fine with most
>> of the changes :) I'm just not sure about having two distinct
>> functions for reporting failures, given that there's only one caller
>> for each.  On the other hand it avoids to include miscadmin.h in
>> bufpage.c.
>
>
> Yeah, and it brings "cosistence" to at least the calling point(s) within regular backends.
>
>
>>
>> That's just a detail, so I'm marking it (again) as ready for committer!
>
>
> Thanks, and pushed :)

Thanks :)

On Sat, Mar 9, 2019 at 7:50 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Sat, Mar 9, 2019 at 10:41 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>>
>> Sorry, I have again new comments after a little bit more thinking.
>> I'm wondering if we can do something about shared objects while we're
>> at it.  They don't belong to any database, so it's a little bit
>> orthogonal to this proposal, but it seems quite important to track
>> error on those too!
>>
>> What about adding a new field in PgStat_GlobalStats for that?  We can
>> use the same lastDir to easily detect such objects and slightly adapt
>> sendFile again, which seems quite straightforward.
>
>
> Ah, didn't spot that one until after I pushed :/ Sorry about that.

No problem, I should have thought about it sooner anyway.

> Hmm. That's an interesting thought. And then add a column to pg_stat_bgwriter, I assume?

Yes, and a new entry for PgStat_Shared_Reset_Target I guess.

 (Which is an ever increasingly bad name for the view, but that's
unrelated to this)

yeah :/

> Question is then what number that should show -- only the checksum counter in non-database-fields, or the total
numberacross the cluster?
 

I'd say only for non-database-fields errors, especially if we can
reset each counters separately.  If necessary, we can add a new view
to give a global overview of checksum errors for DBA convenience.

On Sat, Mar 9, 2019 at 7:58 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Sat, Mar 9, 2019 at 7:50 PM Magnus Hagander <magnus@hagander.net> wrote:
> >
> > On Sat, Mar 9, 2019 at 10:41 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >>
> >> Sorry, I have again new comments after a little bit more thinking.
> >> I'm wondering if we can do something about shared objects while we're
> >> at it.  They don't belong to any database, so it's a little bit
> >> orthogonal to this proposal, but it seems quite important to track
> >> error on those too!
> >>
> >> What about adding a new field in PgStat_GlobalStats for that?  We can
> >> use the same lastDir to easily detect such objects and slightly adapt
> >> sendFile again, which seems quite straightforward.
>
> > Question is then what number that should show -- only the checksum counter in non-database-fields, or the total
numberacross the cluster?
 
>
> I'd say only for non-database-fields errors, especially if we can
> reset each counters separately.  If necessary, we can add a new view
> to give a global overview of checksum errors for DBA convenience.

So, after reading current implementation, I don't think that
PgStat_GlobalStats is the right place.  It's already enough of a mess,
and clearly pg_stat_reset_shared('bgwriter') would not make any sense
if it did reset the shared relations checksum errors (though arguably
the fact that's it's resetting checkpointer stats right now is hardly
better), and handling a different target to reset part of
PgStat_GlobalStats counters would be an ugly kludge.

I'm considering adding a new PgStat_ChecksumStats for that purpose
instead, but I don't know if that's acceptable to do so in the last
commitfest.  It seems worthwhile to add it eventually, since we'll
probably end up having more things to report to users related to
checksum.  Online enabling of checksum could be the most immediate
potential target.

If that's acceptable, I was thinking this new stat could have those
fields with the first drop:

- number of non-db-related checksum checks done
- number of non-db-related checksum checks failed
- last stats reset

(and adding the number of checks for db-related blocks done with the
current checksum errors counter).  Maybe also adding a
pg_checksum_stats view that would summarise all the counters in one
place.

It'll obviously add some traffic to the stats collector, but I'd hope
not too much, since BufferAlloc shouldn't be that frequent, and
BASE_BACKUP reports stats only once per file.

On Sun, Mar 10, 2019 at 1:13 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Sat, Mar 9, 2019 at 7:58 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > On Sat, Mar 9, 2019 at 7:50 PM Magnus Hagander <magnus@hagander.net> wrote:
> > >
> > > On Sat, Mar 9, 2019 at 10:41 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > >>
> > >> Sorry, I have again new comments after a little bit more thinking.
> > >> I'm wondering if we can do something about shared objects while we're
> > >> at it.  They don't belong to any database, so it's a little bit
> > >> orthogonal to this proposal, but it seems quite important to track
> > >> error on those too!
> > >>
> > >> What about adding a new field in PgStat_GlobalStats for that?  We can
> > >> use the same lastDir to easily detect such objects and slightly adapt
> > >> sendFile again, which seems quite straightforward.
> >
> > > Question is then what number that should show -- only the checksum counter in non-database-fields, or the total
numberacross the cluster?
 
> >
> > I'd say only for non-database-fields errors, especially if we can
> > reset each counters separately.  If necessary, we can add a new view
> > to give a global overview of checksum errors for DBA convenience.
>
> I'm considering adding a new PgStat_ChecksumStats for that purpose
> instead, but I don't know if that's acceptable to do so in the last
> commitfest.  It seems worthwhile to add it eventually, since we'll
> probably end up having more things to report to users related to
> checksum.  Online enabling of checksum could be the most immediate
> potential target.

I wasn't aware that we were already storing informations about shared
objects in PgStat_StatDBEntry, with an InvalidOid as databaseid
(though we don't have any system view that are actually showing
information for such objects).

As a result I ended up simply adding counters for the number of total
checks and the timestamp of the last failure in PgStat_StatDBEntry,
making attached patch very lightweight.  I moved all the checksum
related counters out of pg_stat_database in a new pg_stat_checksum
view.  It avoids to make pg_stat_database too wide, and also allows to
display information about shared object in this new view (some of the
other counters don't really make sense for shared objects or could
break existing monitoring query).  While at it, I tried to add a
little bit of documentation wrt. checksum monitoring.

On Wed, Mar 13, 2019 at 4:53 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Sun, Mar 10, 2019 at 1:13 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > On Sat, Mar 9, 2019 at 7:58 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > >
> > > On Sat, Mar 9, 2019 at 7:50 PM Magnus Hagander <magnus@hagander.net> wrote:
> > > >
> > > > On Sat, Mar 9, 2019 at 10:41 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > > >>
> > > >> Sorry, I have again new comments after a little bit more thinking.
> > > >> I'm wondering if we can do something about shared objects while we're
> > > >> at it.  They don't belong to any database, so it's a little bit
> > > >> orthogonal to this proposal, but it seems quite important to track
> > > >> error on those too!
> > > >>
> > > >> What about adding a new field in PgStat_GlobalStats for that?  We can
> > > >> use the same lastDir to easily detect such objects and slightly adapt
> > > >> sendFile again, which seems quite straightforward.
> > >
> > > > Question is then what number that should show -- only the checksum counter in non-database-fields, or the total
numberacross the cluster?
 
> > >
> > > I'd say only for non-database-fields errors, especially if we can
> > > reset each counters separately.  If necessary, we can add a new view
> > > to give a global overview of checksum errors for DBA convenience.
> >
> > I'm considering adding a new PgStat_ChecksumStats for that purpose
> > instead, but I don't know if that's acceptable to do so in the last
> > commitfest.  It seems worthwhile to add it eventually, since we'll
> > probably end up having more things to report to users related to
> > checksum.  Online enabling of checksum could be the most immediate
> > potential target.
>
> I wasn't aware that we were already storing informations about shared
> objects in PgStat_StatDBEntry, with an InvalidOid as databaseid
> (though we don't have any system view that are actually showing
> information for such objects).
>
> As a result I ended up simply adding counters for the number of total
> checks and the timestamp of the last failure in PgStat_StatDBEntry,
> making attached patch very lightweight.  I moved all the checksum
> related counters out of pg_stat_database in a new pg_stat_checksum
> view.  It avoids to make pg_stat_database too wide, and also allows to
> display information about shared object in this new view (some of the
> other counters don't really make sense for shared objects or could
> break existing monitoring query).  While at it, I tried to add a
> little bit of documentation wrt. checksum monitoring.

and of course I forgot to attach the patch.

On Sat, Mar 30, 2019 at 2:33 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Wed, Mar 13, 2019 at 4:54 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>>
>> On Wed, Mar 13, 2019 at 4:53 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>> >
>> > As a result I ended up simply adding counters for the number of total
>> > checks and the timestamp of the last failure in PgStat_StatDBEntry,
>> > making attached patch very lightweight.  I moved all the checksum
>> > related counters out of pg_stat_database in a new pg_stat_checksum
>> > view.  It avoids to make pg_stat_database too wide, and also allows to
>> > display information about shared object in this new view (some of the
>> > other counters don't really make sense for shared objects or could
>> > break existing monitoring query).  While at it, I tried to add a
>> > little bit of documentation wrt. checksum monitoring.
>>
>> and of course I forgot to attach the patch.
>
>
> Does it really make any sense to track "number of checksum checks"? In any sort of interesting database that's just
goingto be an insanely high number, isn't it? (And also, to stay consistent with checksum failures, we should of course
alsocount the checks done in base backups, which is not in the patch. But I'm more thinking we should drop it) 

Thanks for looking at it!

It's surely going to be a huge number on databases with a large number
of buffer eviction and/or frequent pg_basebackup.  The idea was to be
able to know if the possible lack of failure was due to lack of check
at all or because the server appears to be healthy, without spamming
gettimeofday calls.  If having a last_check() is better, I'm fine with
it.  If it's useless, let's drop it.

The number of checks was supposed to also be tracked in base_backups, with

@@ -1527,6 +1527,8 @@ sendFile(const char *readfilename, const char
*tarfilename, struct stat *statbuf
                                             "failures in file \"%s\" will not "
                                             "be reported", readfilename)));
                     }
+                    else if (block_retry == false)
+                        checksum_checks++;

> Having thought some more about this, I wonder if the right thing to do is to actually add a row to pg_stat_database
forthe global stats, rather than invent a separate view. I can see the argument going both ways, but particularly with
thename pg_stat_checksums we are setting a pattern that will create one view for each counter. That's not very good, I
think.
>
> In the end I'm somewhat split on the idea of pg_stat_database with a NULL row or pg_stat_checkpoints. What do others
think?

I agree that having a separate view for each counter if a bad idea.
But what I was thinking is that we'll probably end up with a view to
track per-db online checksum activation progress/activity/status at
some point (similar to pg_stat_progress_vacuum), so why not starting
with this dedicated view right now and add new counters later, either
in pgstat and/or some shmem, as long as we keep the view name as SQL
interface.

Anyway, I don't have a strong preference for any implementation, so
I'll be happy to send an updated patch with what ends up being
preferred.

Sorry for delay, I had to catch a train.

On Sat, Mar 30, 2019 at 4:02 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> My vote is still to drop it completely, but if we're keeping it, it has to go in both paths.

Ok.  For now I'm attaching v2, which drops this field, rename the view
to pg_stat_checksums (terminal s), and use the policy for choosing
random oid in the 8000..9999 range for new functions.

I'd also have to get more feedback on this.  For now, I'll add this
thread to the pg12 open items, as a follow up of the initial code
drop.

> Technically, that should be in pg_stat_progress_checksums to be consistent :) So whichever way we turn, it's going to
beinconsistent with something.
 

Indeed :)

On Sat, Mar 30, 2019 at 06:15:11PM +0100, Julien Rouhaud wrote:
> I'd also have to get more feedback on this.  For now, I'll add this
> thread to the pg12 open items, as a follow up of the initial code
> drop.

Catching up here...  I think that having a completely separate view
with one row for each database and one row for shared objects makes
the most sense based on what has been proposed on this thread.  Being
able to track checksum failures for shared catalogs is really
something I'd like to be able to see easily, and I have seen
corruption involving such objects from time to time.  I think that we
should have a design which is extensible.  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.

For the progress part, we would most likely have a separate view for
that as well, as the view should show no rows if there is no operation
in progress.

The patch looks rather clean to me, I have some comments.

-   <application>pg_checksums</application>. The exit status is zero if there
-   are no checksum errors when checking them, and nonzero if at least one
-   checksum failure is detected. If enabling or disabling checksums, the
-   exit status is nonzero if the operation failed.
+   <application>pg_checksums</application>. As a consequence, the
+   <structname>pg_stat_checksums</structnameview won't reflect this activity.
+   The exit status is zero if there are no checksum errors when checking them,
+   and nonzero if at least one checksum failure is detected. If enabling or
+   disabling checksums, the exit status is nonzero if the operation failed.

The docs of pg_checksums already clearly state that the cluster needs
to be offline, so I am not sure that this addition is necessary.

@@ -1539,6 +1539,8 @@ pgstat_report_checksum_failures_in_db(Oid dboid,
int failurecount)

Please note that there is no need to have the list of arguments in the
comment block at the top of pgstat_report_checksum_failures_in_db().

+    if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL)
+        result = 0;
+    else
+        result = dbentry->last_checksum_failure;
+
+    if (result == 0)
+        PG_RETURN_NULL();
+    else
+        PG_RETURN_TIMESTAMPTZ(result);
+}

No need for two ifs here.  What about just that?
if (NULL)
    PG_RETURN_NULL();
else
    PG_RETURN_TIMESTAMPTZ(last_checksum_failure);
--
Michael

On Tue, Apr 2, 2019 at 6:56 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Mar 30, 2019 at 06:15:11PM +0100, Julien Rouhaud wrote:
> > I'd also have to get more feedback on this.  For now, I'll add this
> > thread to the pg12 open items, as a follow up of the initial code
> > drop.
>
> Catching up here...  I think that having a completely separate view
> with one row for each database and one row for shared objects makes
> the most sense based on what has been proposed on this thread.  Being
> able to track checksum failures for shared catalogs is really
> something I'd like to be able to see easily, and I have seen
> corruption involving such objects from time to time.  I think that we
> should have a design which is extensible.

Ok!

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

> For the progress part, we would most likely have a separate view for
> that as well, as the view should show no rows if there is no operation
> in progress.

Ok.

> The patch looks rather clean to me, I have some comments.
>
> -   <application>pg_checksums</application>. The exit status is zero if there
> -   are no checksum errors when checking them, and nonzero if at least one
> -   checksum failure is detected. If enabling or disabling checksums, the
> -   exit status is nonzero if the operation failed.
> +   <application>pg_checksums</application>. As a consequence, the
> +   <structname>pg_stat_checksums</structnameview won't reflect this activity.
> +   The exit status is zero if there are no checksum errors when checking them,
> +   and nonzero if at least one checksum failure is detected. If enabling or
> +   disabling checksums, the exit status is nonzero if the operation failed.
>
> The docs of pg_checksums already clearly state that the cluster needs
> to be offline, so I am not sure that this addition is necessary.

Agreed, removed.

> @@ -1539,6 +1539,8 @@ pgstat_report_checksum_failures_in_db(Oid dboid,
> int failurecount)
>
> Please note that there is no need to have the list of arguments in the
> comment block at the top of pgstat_report_checksum_failures_in_db().

Indeed, fixed.

> +       if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL)
> +               result = 0;
> +       else
> +               result = dbentry->last_checksum_failure;
> +
> +       if (result == 0)
> +               PG_RETURN_NULL();
> +       else
> +               PG_RETURN_TIMESTAMPTZ(result);
> +}
>
> 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.

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

On Tue, Apr 02, 2019 at 07:06:35PM +0200, Magnus Hagander wrote:
> I think having the count and hte last time make sense, but I'm very
> sceptical about the rest.

There may be other things which we are not considering on this
thread.  I don't know.

> I can somewhat agree that splitting it  on a per database level might even
> at that be overdoing it. What might actually be more interesting from a
> failure-location perspective would be tablespace, rather than any of the
> others. Or we could reduce it down to just putting it in pg_stat_bgwriter
> and only count global values perhaps, if in the end we don't think the
> split-per-database is reasonable?

A split per database or per tablespace is I think a very good thing.
This helps in tracking down which partitions have gone crazy, and a
single global counter does not allow that.
--
Michael

On Wed, Apr 3, 2019 at 3:43 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> > I can somewhat agree that splitting it  on a per database level might even
> > at that be overdoing it. What might actually be more interesting from a
> > failure-location perspective would be tablespace, rather than any of the
> > others. Or we could reduce it down to just putting it in pg_stat_bgwriter
> > and only count global values perhaps, if in the end we don't think the
> > split-per-database is reasonable?
>
> A split per database or per tablespace is I think a very good thing.
> This helps in tracking down which partitions have gone crazy, and a
> single global counter does not allow that.

Indeed, a per-tablespace would be much more convenient to track the
problem down at the physical level, but we don't have the required
infrastructure for that yet, and it seems quite late to add it now.
IMHO, a per-database has also some value, as it can help to track down
issues at the application level.

Maybe we could add a new column to the view (for instance "source")
which would always be 'database', and we could later add
per-tablespace counters, keeping the view compatibility.

On Wed, Apr 3, 2019 at 11:31 AM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Wed, Apr 3, 2019 at 10:44 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>>
>> On Wed, Apr 3, 2019 at 3:43 AM Michael Paquier <michael@paquier.xyz> wrote:
>> >
>> > > I can somewhat agree that splitting it  on a per database level might even
>> > > at that be overdoing it. What might actually be more interesting from a
>> > > failure-location perspective would be tablespace, rather than any of the
>> > > others. Or we could reduce it down to just putting it in pg_stat_bgwriter
>> > > and only count global values perhaps, if in the end we don't think the
>> > > split-per-database is reasonable?
>> >
>> > A split per database or per tablespace is I think a very good thing.
>> > This helps in tracking down which partitions have gone crazy, and a
>> > single global counter does not allow that.
>>
>> Indeed, a per-tablespace would be much more convenient to track the
>> problem down at the physical level, but we don't have the required
>> infrastructure for that yet, and it seems quite late to add it now.
>> IMHO, a per-database has also some value, as it can help to track down
>> issues at the application level.
>>
>> Maybe we could add a new column to the view (for instance "source")
>> which would always be 'database', and we could later add
>> per-tablespace counters, keeping the view compatibility.
>
>
> Ugh.
>
> If we wanted per tablespace counters, shouldn't we have a pg_stat_tablespace instead? So we'd have a checksum
failurescounter in pg_state_database separated by database, and one in pg_stat_tablespace separated by tablespace?
(Alongwith probably a bunch of other entries for tablespaces) 

But there's still the problem of reporting errors on shared relation,
so pg_stat_database doesn't really fit for that.  If we go with a
checksum centric view, it'd be strange to have some of the counters in
another view.

On Wed, Apr 03, 2019 at 11:56:14AM +0200, Julien Rouhaud wrote:
> But there's still the problem of reporting errors on shared relation,
> so pg_stat_database doesn't really fit for that.  If we go with a
> checksum centric view, it'd be strange to have some of the counters in
> another view.

Having pg_stat_database filled with a phantom row full of NULLs to
track checksum failures of shared objects would be confusing I think.
I personally quite like the separate view approach, with one row per
database, but one opinion does not stand as an agreement.

Anyway, even if we have no agreement on the shape of what we'd like to
do, I don't think that HEAD is in a proper shape now because we just
don't track a portion of the objects which could have checksum
failures.  So we should either revert the patch currently committed,
or add tracking for shared objects, but definitely not keep the code
in a state in-between.
--
Michael

On Thu, Apr 4, 2019 at 10:25 AM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Thu, Apr 4, 2019 at 6:22 AM Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Wed, Apr 03, 2019 at 11:56:14AM +0200, Julien Rouhaud wrote:
>> > But there's still the problem of reporting errors on shared relation,
>> > so pg_stat_database doesn't really fit for that.  If we go with a
>> > checksum centric view, it'd be strange to have some of the counters in
>> > another view.
>>
>> Having pg_stat_database filled with a phantom row full of NULLs to
>> track checksum failures of shared objects would be confusing I think.
>> I personally quite like the separate view approach, with one row per
>> database, but one opinion does not stand as an agreement.
>
> It wouldn't be just that, but it would make sense to include things like blks_read/blks_hit there as well, wouldn't
it?As well as read/write time. Things we don't track today, but it could be useful to do so.
 

Actually we do track counters for shared relations (see
pgstat_report_stat), we just don't expose them in any view.  But it's
still possible to get the counters manually:

# select pg_stat_get_db_blocks_hit(0);
 pg_stat_get_db_blocks_hit
---------------------------
                   2710329
(1 row)

My main concern is that pg_stat_get_db_numbackends(0) report something
like the total number of backend (though it seems that there's an
extra connection accounted for, I don't know which process it's), so
if we expose it in pg_stat_database, sum(numbackends) won't make sense
anymore.

>> Anyway, even if we have no agreement on the shape of what we'd like to
>> do, I don't think that HEAD is in a proper shape now because we just
>> don't track a portion of the objects which could have checksum
>> failures.  So we should either revert the patch currently committed,
>> or add tracking for shared objects, but definitely not keep the code
>> in a state in-between.
>
>
> Definitely. That's why we're discussing it now :) Maybe we should put it on the open items list, because we
definitelydon't want to ship it one way and then change our mind in the next version.
 

I already added an open item for that.

On Thu, Apr 4, 2019 at 1:25 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Thu, Apr 4, 2019 at 10:47 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>>
>> Actually we do track counters for shared relations (see
>> pgstat_report_stat), we just don't expose them in any view.  But it's
>> still possible to get the counters manually:
>>
>> # select pg_stat_get_db_blocks_hit(0);
>>  pg_stat_get_db_blocks_hit
>> ---------------------------
>>                    2710329
>> (1 row)
>
>
> Oh, right, we do actually collect it, we just don't show is. So that's another argument *for* having it in
pg_stat_database.Or at least not for having it in a checksum specific view, because then we should really make a
separateview for this as well.
 

Ok, so let's expose all the shared counters in pg_stat_database and
remove the pg_stat_checksum view.

>> My main concern is that pg_stat_get_db_numbackends(0) report something
>> like the total number of backend (though it seems that there's an
>> extra connection accounted for, I don't know which process it's), so
>> if we expose it in pg_stat_database, sum(numbackends) won't make sense
>> anymore.
>
> We could also just hardcoded it so that one always shows 0?

That's a bit hacky, but that's probably the best compromise.  Attached
v4 with all those changes.

Thanks for looking it it!

On Sun, Apr 7, 2019 at 4:36 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> I'm not sure I like the idea of using "<shared_objects>" as the database name. It's not very likely that somebody
wouldbe using that as a name for their database, but i's not impossible. But it also just looks strrange. Wouldn't NULL
bea more appropriate choice? 
>
> Likewise, shouldn't we return NULL as the number of backends for the shared counters, rather than 0?
I wanted to make things more POLA-compliant, but maybe it was a bad
idea.  I changed it for NULL here and for numbackends.

> Micro-nit:
> +     <entry>Time at which the last data page checksum failures was detected in
> s/failures/failure/

Oops.

v5 attached.

On Fri, Apr 12, 2019 at 2:18 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Sun, Apr 7, 2019 at 6:28 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>>
>> v5 attached.
>
> Thanks. Pushed!

Thanks!

I started looking at this the other night but I see Magnus beat me in
committing it...

On Fri, Apr 12, 2019 at 8:18 AM Magnus Hagander <magnus@hagander.net> wrote:
> On Sun, Apr 7, 2019 at 6:28 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>> Thanks for looking it it!
>> On Sun, Apr 7, 2019 at 4:36 PM Magnus Hagander <magnus@hagander.net> wrote:
>> >
>> > I'm not sure I like the idea of using "<shared_objects>" as the database name. It's not very likely that somebody
wouldbe using that as a name for their database, but i's not impossible. But it also just looks strrange. Wouldn't NULL
bea more appropriate choice? 
>> >
>> > Likewise, shouldn't we return NULL as the number of backends for the shared counters, rather than 0?
>> I wanted to make things more POLA-compliant, but maybe it was a bad
>> idea.  I changed it for NULL here and for numbackends.
>>

ISTM the argument here is go with zero since you have zero connections
vs go with null since you can't actually connect, so it doesn't make
sense. (There is a third argument about making it -1 since you can't
connect, but that breaks sum(numbackends) so it's easily dismissed.) I
think I would have gone for 0 personally, but what ended up surprising
me was that a bunch of other stuff like xact_commit show zero when
AFAICT the above reasoning would apply the same to those columns.
(unless there is a way to commit a transaction in the global objects
that I don't know about).

>> > Micro-nit:
>> > +     <entry>Time at which the last data page checksum failures was detected in
>> > s/failures/failure/
>>
>> Oops.
>>
>> v5 attached.
>

What originally got me looking at this was the idea of returning -1
(or maybe null) for checksum failures for cases when checksums are not
enabled. This seems a little more complicated to set up, but seems
like it might ward off people thinking they are safe due to no
checksum error reports when they actually aren't.


Robert Treat
https://xzilla.net

Sorry for late reply,

On Sun, Apr 14, 2019 at 7:12 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Sat, Apr 13, 2019 at 8:46 PM Robert Treat <rob@xzilla.net> wrote:
>>
>> On Fri, Apr 12, 2019 at 8:18 AM Magnus Hagander <magnus@hagander.net> wrote:
>> ISTM the argument here is go with zero since you have zero connections
>> vs go with null since you can't actually connect, so it doesn't make
>> sense. (There is a third argument about making it -1 since you can't
>> connect, but that breaks sum(numbackends) so it's easily dismissed.) I
>> think I would have gone for 0 personally, but what ended up surprising
>> me was that a bunch of other stuff like xact_commit show zero when
>> AFAICT the above reasoning would apply the same to those columns.
>> (unless there is a way to commit a transaction in the global objects
>> that I don't know about).
>
>
> That's a good point. I mean, you can commit a transaction that involves changes of global objects, but it counts in
thedatabase that you were conneced to.
 
>
> We should probably at least make it consistent and make it NULL in all or 0 in all.
>
> I'm -1 for using -1 (!), for the very reason that you mention. But either changing the numbackends to 0, or the
othersto NULL would work for consistency. I'm leaning towards the 0 as well.
 

+1 for 0 :)  Especially since it's less code in the view.

>> What originally got me looking at this was the idea of returning -1
>> (or maybe null) for checksum failures for cases when checksums are not
>> enabled. This seems a little more complicated to set up, but seems
>> like it might ward off people thinking they are safe due to no
>> checksum error reports when they actually aren't.
>
>
> NULL seems like the reasonable thing to return there. I'm not sure what you're referring to with a little more
complicatedto set up, thought? Do you mean somehow for the end user?
 
>
> Code-wise it seems it should be simple -- just do an "if checksums disabled then return null"  in the two functions.

That's indeed a good point!  Lack of checksum error is distinct from
checksums not activated and we should make it obvious.

I don't know if that counts as an open item, but I attach a patch for
all points discussed here.

On Mon, Apr 15, 2019 at 3:32 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> Sorry for late reply,
>
> On Sun, Apr 14, 2019 at 7:12 PM Magnus Hagander <magnus@hagander.net> wrote:
> >
> > On Sat, Apr 13, 2019 at 8:46 PM Robert Treat <rob@xzilla.net> wrote:
> >>
> >> On Fri, Apr 12, 2019 at 8:18 AM Magnus Hagander <magnus@hagander.net> wrote:
> >> ISTM the argument here is go with zero since you have zero connections
> >> vs go with null since you can't actually connect, so it doesn't make
> >> sense. (There is a third argument about making it -1 since you can't
> >> connect, but that breaks sum(numbackends) so it's easily dismissed.) I
> >> think I would have gone for 0 personally, but what ended up surprising
> >> me was that a bunch of other stuff like xact_commit show zero when
> >> AFAICT the above reasoning would apply the same to those columns.
> >> (unless there is a way to commit a transaction in the global objects
> >> that I don't know about).
> >
> >
> > That's a good point. I mean, you can commit a transaction that involves changes of global objects, but it counts in
thedatabase that you were conneced to.
 
> >
> > We should probably at least make it consistent and make it NULL in all or 0 in all.
> >
> > I'm -1 for using -1 (!), for the very reason that you mention. But either changing the numbackends to 0, or the
othersto NULL would work for consistency. I'm leaning towards the 0 as well.
 
>
> +1 for 0 :)  Especially since it's less code in the view.
>

+1 for 0

> >> What originally got me looking at this was the idea of returning -1
> >> (or maybe null) for checksum failures for cases when checksums are not
> >> enabled. This seems a little more complicated to set up, but seems
> >> like it might ward off people thinking they are safe due to no
> >> checksum error reports when they actually aren't.
> >
> >
> > NULL seems like the reasonable thing to return there. I'm not sure what you're referring to with a little more
complicatedto set up, thought? Do you mean somehow for the end user?
 
> >
> > Code-wise it seems it should be simple -- just do an "if checksums disabled then return null"  in the two
functions.
>
> That's indeed a good point!  Lack of checksum error is distinct from
> checksums not activated and we should make it obvious.
>
> I don't know if that counts as an open item, but I attach a patch for
> all points discussed here.

ISTM we should mention shared objects in both places in the docs, and
want "NULL if data checksums" rather than "NULL is data checksums".
Attaching slightly modified patch with those changes, but otherwise
LGTM.

Robert Treat
https://xzilla.net

On Wed, Apr 17, 2019 at 1:55 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Tue, Apr 16, 2019 at 5:39 PM Robert Treat <rob@xzilla.net> wrote:
>>
>> On Mon, Apr 15, 2019 at 3:32 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>> >
>> > I don't know if that counts as an open item, but I attach a patch for
>> > all points discussed here.
>>
>> ISTM we should mention shared objects in both places in the docs, and
>> want "NULL if data checksums" rather than "NULL is data checksums".
>> Attaching slightly modified patch with those changes, but otherwise
>> LGTM.

Thanks, that's indeed embarassing typos.  And agreed for mentioning
shared objects in both places.

>
>  Interestingly enough, that patch comes out as corrupt. I have no idea why though :) v1 is fine.
>
> So I tried merging back your changes into it, and then pushing. Please doublecheck I didn't miss something :)

Thanks!  I double checked and it all looks fine.

On Wed, Apr 17, 2019 at 9:07 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Wed, Apr 17, 2019 at 1:55 PM Magnus Hagander <magnus@hagander.net> wrote:
> >
> > On Tue, Apr 16, 2019 at 5:39 PM Robert Treat <rob@xzilla.net> wrote:
> >>
> >> On Mon, Apr 15, 2019 at 3:32 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >> >
> >> > I don't know if that counts as an open item, but I attach a patch for
> >> > all points discussed here.
> >>
> >> ISTM we should mention shared objects in both places in the docs, and
> >> want "NULL if data checksums" rather than "NULL is data checksums".
> >> Attaching slightly modified patch with those changes, but otherwise
> >> LGTM.
>
> Thanks, that's indeed embarassing typos.  And agreed for mentioning
> shared objects in both places.
>
> >
> >  Interestingly enough, that patch comes out as corrupt. I have no idea why though :) v1 is fine.
> >
> > So I tried merging back your changes into it, and then pushing. Please doublecheck I didn't miss something :)
>
> Thanks!  I double checked and it all looks fine.

+1

Robert Treat
https://xzilla.net

On 4/2/19 7:06 PM, Magnus Hagander wrote:
> On Tue, Apr 2, 2019 at 8:47 AM Michael Paquier <michael@paquier.xyz <mailto:michael@paquier.xyz>> wrote:
> 
>     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 <mailto: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.
> 
> 
> I think that tracking each and every block is of course a non-starter, as you've noticed.

I think that's less of a concern now that the stats collector process has gone and that the stats are now collected in
sharedmemory, what do you think?
 

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Sun, Dec 11, 2022 at 09:18:42PM +0100, Magnus Hagander wrote:
> It would be less of a concern yes, but I think it still would be a concern.
> If you have a large amount of corruption you could quickly get to millions
> of rows to keep track of which would definitely be a problem in shared
> memory as well, wouldn't it?

Yes.  I have discussed this item with Bertrand off-list and I share
the same concern.  This would lead to an lot of extra workload on a
large seqscan for a corrupted relation when the stats are written
(shutdown delay) while bloating shared memory with potentially
millions of items even if variable lists are handled through a dshash
and DSM.

> But perhaps we could keep a list of "the last 100 checksum failures" or
> something like that?

Applying a threshold is one solution.  Now, a second thing I have seen
in the past is that some disk partitions were busted but not others,
and the current database-level counters are not enough to make a
difference when it comes to grab patterns in this area.  A list of the
last N failures may be able to show some pattern, but that would be
like analyzing things with a lot of noise without a clear conclusion.
Anyway, the workload caused by the threshold number had better be
measured before being decided (large set of relation files with a full
range of blocks corrupted, much better if these are in the OS cache
when scanned), which does not change the need of a benchmark.

What about just adding a counter tracking the number of checksum
failures for relfilenodes in a new structure related to them (note
that I did not write PgStat_StatTabEntry)?

If we do that, it is then possible to cross-check the failures with
tablespaces, which would point to disk areas that are more sensitive
to corruption.
--
Michael

Hi,

On 2022-12-12 08:40:04 +0900, Michael Paquier wrote:
> What about just adding a counter tracking the number of checksum
> failures for relfilenodes in a new structure related to them (note
> that I did not write PgStat_StatTabEntry)?

Why were you thinking of tracking it separately from PgStat_StatTabEntry?

I think there's a good argument for starting to track some stats based on the
relfilenode, rather the oid, because it'd allow us to track e.g. the number of
writes for a relation too (we don't have the oid when writing out
buffers). But that's a relatively large change...

Greetings,

Andres Freund

On Sun, Dec 11, 2022 at 04:51:49PM -0800, Andres Freund wrote:
> Why were you thinking of tracking it separately from PgStat_StatTabEntry?

We only know the relfilenode when loading the page on a checksum
failure, not its parent relation, and there are things like physical
base backups where we would not know them anyway because we may not be
connected to a database.  Or perhaps it would be possible to link
table entries with their relfilenodes using some tweaks in the stat
APIs?  I am sure that you know the business in this area better than I
do currently :)

> I think there's a good argument for starting to track some stats based on the
> relfilenode, rather the oid, because it'd allow us to track e.g. the number of
> writes for a relation too (we don't have the oid when writing out
> buffers). But that's a relatively large change...

Yeah.  I was thinking among the lines of sync requests and sync
failures, as well.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Dec 11, 2022 at 04:51:49PM -0800, Andres Freund wrote:
>> I think there's a good argument for starting to track some stats based on the
>> relfilenode, rather the oid, because it'd allow us to track e.g. the number of
>> writes for a relation too (we don't have the oid when writing out
>> buffers). But that's a relatively large change...

> Yeah.  I was thinking among the lines of sync requests and sync
> failures, as well.

I think a stats table indexed solely by relfilenode wouldn't be a great
idea, because you'd lose all the stats about a table as soon as you
do vacuum full/cluster/rewriting-alter-table/etc.  Can we create two
index structures over the same stats table entries, so you can look
up by either relfilenode or OID?  I'm not quite sure how to manage
rewrites, where you transiently have two relfilenodes for "the
same" table ... maybe we could allow multiple pointers to the same
stats entry??

            regards, tom lane

On Sun, Dec 11, 2022 at 08:48:15PM -0500, Tom Lane wrote:
> I think a stats table indexed solely by relfilenode wouldn't be a great
> idea, because you'd lose all the stats about a table as soon as you
> do vacuum full/cluster/rewriting-alter-table/etc.  Can we create two
> index structures over the same stats table entries, so you can look
> up by either relfilenode or OID?  I'm not quite sure how to manage
> rewrites, where you transiently have two relfilenodes for "the
> same" table ... maybe we could allow multiple pointers to the same
> stats entry??

FWIW, I am not sure that I would care much if we were to dropped the
stats associated to a relfilenode on a rewrite.  In terms of checksum
failures, tuples are deformed so if there is one checksum failure a
rewrite would just not happen.  The potential complexity is not really
appealing compared to the implementation simplicity and its gains, and
rewrites are lock-heavy so I'd like to think that people avoid them
(cough)..
--
Michael

On 12/12/22 12:40 AM, Michael Paquier wrote:
> On Sun, Dec 11, 2022 at 09:18:42PM +0100, Magnus Hagander wrote:
>> It would be less of a concern yes, but I think it still would be a concern.
>> If you have a large amount of corruption you could quickly get to millions
>> of rows to keep track of which would definitely be a problem in shared
>> memory as well, wouldn't it?
> 
> Yes.  I have discussed this item with Bertrand off-list and I share
> the same concern.  This would lead to an lot of extra workload on a
> large seqscan for a corrupted relation when the stats are written
> (shutdown delay) while bloating shared memory with potentially
> millions of items even if variable lists are handled through a dshash
> and DSM.
> 
>> But perhaps we could keep a list of "the last 100 checksum failures" or
>> something like that?
> 
> Applying a threshold is one solution.  Now, a second thing I have seen
> in the past is that some disk partitions were busted but not others,
> and the current database-level counters are not enough to make a
> difference when it comes to grab patterns in this area.  A list of the
> last N failures may be able to show some pattern, but that would be
> like analyzing things with a lot of noise without a clear conclusion.
> Anyway, the workload caused by the threshold number had better be
> measured before being decided (large set of relation files with a full
> range of blocks corrupted, much better if these are in the OS cache
> when scanned), which does not change the need of a benchmark.
> 
> What about just adding a counter tracking the number of checksum
> failures for relfilenodes 

Agree about your concern for tracking the corruption for every single block.
I like this idea for relfilenodes tracking instead. Indeed it looks like this is enough useful historical information
towork with.
 

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 12/12/22 5:09 AM, Michael Paquier wrote:
> On Sun, Dec 11, 2022 at 08:48:15PM -0500, Tom Lane wrote:
>> I think a stats table indexed solely by relfilenode wouldn't be a great
>> idea, because you'd lose all the stats about a table as soon as you
>> do vacuum full/cluster/rewriting-alter-table/etc.  Can we create two
>> index structures over the same stats table entries, so you can look
>> up by either relfilenode or OID?  I'm not quite sure how to manage
>> rewrites, where you transiently have two relfilenodes for "the
>> same" table ... maybe we could allow multiple pointers to the same
>> stats entry??
> 
> FWIW, I am not sure that I would care much if we were to dropped the
> stats associated to a relfilenode on a rewrite.  In terms of checksum
> failures, tuples are deformed so if there is one checksum failure a
> rewrite would just not happen.  The potential complexity is not really
> appealing compared to the implementation simplicity and its gains, and
> rewrites are lock-heavy so I'd like to think that people avoid them
> (cough)..

Agree that this is less "problematic" for the checksum use case.
On the other hand, losing IO stats (as the ones we could add later on, suggested by Andres up-thread) looks more of a
concernto me.
 

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 12/12/22 8:15 AM, Drouvot, Bertrand wrote:
> 
> 
> On 12/12/22 5:09 AM, Michael Paquier wrote:
>> On Sun, Dec 11, 2022 at 08:48:15PM -0500, Tom Lane wrote:
>>> I think a stats table indexed solely by relfilenode wouldn't be a great
>>> idea, because you'd lose all the stats about a table as soon as you
>>> do vacuum full/cluster/rewriting-alter-table/etc.  Can we create two
>>> index structures over the same stats table entries, so you can look
>>> up by either relfilenode or OID?  I'm not quite sure how to manage
>>> rewrites, where you transiently have two relfilenodes for "the
>>> same" table ... maybe we could allow multiple pointers to the same
>>> stats entry??
>>
>> FWIW, I am not sure that I would care much if we were to dropped the
>> stats associated to a relfilenode on a rewrite.  In terms of checksum
>> failures, tuples are deformed so if there is one checksum failure a
>> rewrite would just not happen.  The potential complexity is not really
>> appealing compared to the implementation simplicity and its gains, and
>> rewrites are lock-heavy so I'd like to think that people avoid them
>> (cough)..
> 
> Agree that this is less "problematic" for the checksum use case.
> On the other hand, losing IO stats (as the ones we could add later on, suggested by Andres up-thread) looks more of a
concernto me.
 
> 

One option could be to have a dedicated PgStat_StatRelFileNodeEntry and populate the related PgStat_StatTabEntry when
callingthe new to be created pgstat_relfilenode_flush_cb()? (That's what we are doing currently to
 
flush some of the table stats to the database stats for example).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On 2022-12-11 20:48:15 -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > On Sun, Dec 11, 2022 at 04:51:49PM -0800, Andres Freund wrote:
> >> I think there's a good argument for starting to track some stats based on the
> >> relfilenode, rather the oid, because it'd allow us to track e.g. the number of
> >> writes for a relation too (we don't have the oid when writing out
> >> buffers). But that's a relatively large change...
> 
> > Yeah.  I was thinking among the lines of sync requests and sync
> > failures, as well.
> 
> I think a stats table indexed solely by relfilenode wouldn't be a great
> idea, because you'd lose all the stats about a table as soon as you
> do vacuum full/cluster/rewriting-alter-table/etc.

I don't think that'd be a huge issue - we already have code to keep some
stats as part of rewrites that change the oid of a relation. We could do
the same for rewrites that just change the relfilenode.

However, I'm not sure it's a good idea to keep the stats during
rewrites. Most rewrites end up not copying dead tuples, for example, so
keeping the old counts of updated tuples doesn't really make sense.


> Can we create two index structures over the same stats table entries,
> so you can look up by either relfilenode or OID?

We could likely do that, yes. I think we'd have one "stats body" and
multiple hash table entries pointing to one. The complicated bit would
likely be that we'd need some additional refcounting to know when
there's no references to the "stats body" left.

Greetings,

Andres Freund