On Thu, Sep 10, 2020 at 1:03 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> On Thu, Sep 10, 2020 at 9:05 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Thu, Sep 10, 2020 at 11:52 AM Masahiko Sawada
>> <masahiko.sawada@2ndquadrant.com> wrote:
>> >
>> >
>> > 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;
>> >
>>
>> makes sense, attached find the updated patch.
>
>
> As a minor nitpick, technically, I think the comment change is wrong, because it says that the caller *must* rely on
thetimestamp, which it of course doesn't. I think a more proper one is "The caller must not rely on the timestamp in
casethe function returns false" or "The caller must only rely on the timestamp if the function returns true".
>
The comments already say what you said in the second suggestion:"The
caller must rely on timestamp stored in *ts iff the function returns
true.". Read iff "as if and only if"
> +1 on the code parts.
>
BTW, do we want to backpatch this? There is no user reported bug and
not sure if the user will encounter any problem. I think it is a minor
improvement and more of code consistency. So, making HEAD only change
should be okay.
--
With Regards,
Amit Kapila.