Re: Checksum errors in pg_stat_database

On Tue, Apr 16, 2019 at 5:39 PM Robert Treat <rob(at)xzilla(dot)net> wrote:

> On Mon, Apr 15, 2019 at 3:32 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> >
> > Sorry for late reply,
> >
> > On Sun, Apr 14, 2019 at 7:12 PM Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
> > >
> > > On Sat, Apr 13, 2019 at 8:46 PM Robert Treat <rob(at)xzilla(dot)net> wrote:
> > >>
> > >> On Fri, Apr 12, 2019 at 8:18 AM Magnus Hagander <magnus(at)hagander(dot)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 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.
>

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 :)

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>