Re: Checksum errors in pg_stat_database

On Mon, Apr 15, 2019 at 3:32 PM Julien Rouhaud  wrote:
>
> Sorry for late reply,
>
> On Sun, Apr 14, 2019 at 7:12 PM Magnus Hagander  wrote:
> >
> > On Sat, Apr 13, 2019 at 8:46 PM Robert Treat  wrote:
> >>
> >> On Fri, Apr 12, 2019 at 8:18 AM Magnus Hagander  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 the database 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 others to 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 complicated to 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