Re: Inconsistency in determining the timestamp of the db statfile. - Mailing list pgsql-hackers

On Thu, 10 Sep 2020 at 14:24, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 9, 2020 at 9:37 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >
> > On 2020/09/09 22:57, Magnus Hagander wrote:
> > > On Wed, Sep 9, 2020 at 3:56 PM Tomas Vondra <tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>>
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 <magnus@hagander.net <mailto:magnus@hagander.net>> wrote:
> > >      >>
> > >      >> On Wed, Sep 9, 2020 at 5:04 AM Amit Kapila <amit.kapila16@gmail.com <mailto:amit.kapila16@gmail.com>>
wrote:
> > >      >>>
> > >      >>
> > >      >> Though in fact the one inconsistent place in the code now is that if it is corrupt in the db entry part
ofthe 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