On Thu, 10 Sep 2020 at 14:24, Amit Kapila <[email protected]> wrote:
>
> On Wed, Sep 9, 2020 at 9:37 PM Fujii Masao <[email protected]>
> wrote:
> >
> > On 2020/09/09 22:57, Magnus Hagander wrote:
> > > On Wed, Sep 9, 2020 at 3:56 PM Tomas Vondra <[email protected]
> > > <mailto:[email protected]>> wrote:
> > >
> > > On Wed, Sep 09, 2020 at 03:53:40PM +0530, Amit Kapila wrote:
> > > >On Wed, Sep 9, 2020 at 3:15 PM Magnus Hagander <[email protected]
> > > <mailto:[email protected]>> wrote:
> > > >>
> > > >> On Wed, Sep 9, 2020 at 5:04 AM Amit Kapila
> > > <[email protected] <mailto:[email protected]>> wrote:
> > > >>>
> > > >>
> > > >> Though in fact the one inconsistent place in the code now is that
> > > if it is corrupt in the db entry part of the file it returns true and the
> > > global timestamp, which I would argue is perhaps incorrect and it should
> > > return false.
> > > >>
> > > >
> > > >Yeah, this is exactly the case I was pointing out where we return
> > > true
> > > >before the patch, basically the code below:
> > > >case 'D':
> > > >if (fread(&dbentry, 1, offsetof(PgStat_StatDBEntry, tables),
> > > > fpin) != offsetof(PgStat_StatDBEntry, tables))
> > > >{
> > > >ereport(pgStatRunningInCollector ? LOG : WARNING,
> > > >(errmsg("corrupted statistics file \"%s\"",
> > > >statfile)));
> > > >goto done;
> > > >}
> > > >
> > > >done:
> > > >FreeFile(fpin);
> > > >return true;
> > > >
> > > >Now, if we decide to return 'false' here, then surely there is no
> > > >argument and we should return false in other cases as well.
> > > Basically,
> > > >I think we should be consistent in handling the corrupt file case.
> > > >
> > >
> > > FWIW I do agree with this - we should return false here, to make it
> > > return false like in the other data corruption cases. AFAICS that's
> > > the
> > > only inconsistency here.
> > >
> > >
> > > +1, I think that's the place to fix, rather than reversing all the other
> > > places.
> >
> > +1 as I suggested upthread!
> >
>
> Please find the patch attached based on the above discussion. I have
> slightly adjusted the comments.
FWIW reading the discussion, I also agree to fix it to return false in
case of file corruption.
Regarding the v2 patch, I think we should return false in the
following case too:
default:
ereport(pgStatRunningInCollector ? LOG : WARNING,
(errmsg("corrupted statistics file \"%s\"",
statfile)));
goto done;
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
