Re: Inconsistency in determining the timestamp of the db statfile.

On Wed, Sep 9, 2020 at 3:56 PM Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)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(at)hagander(dot)net>
> wrote:
> >>
> >> On Wed, Sep 9, 2020 at 5:04 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> 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.

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