On Thu, Sep 10, 2020 at 1:03 PM Magnus Hagander <mag...@hagander.net> wrote:
>
> On Thu, Sep 10, 2020 at 9:05 AM Amit Kapila <amit.kapil...@gmail.com> wrote:
>>
>> On Thu, Sep 10, 2020 at 11:52 AM Masahiko Sawada
>> <masahiko.saw...@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 the timestamp, which it of course
> doesn't. I think a more proper one is "The caller must not rely on the
> timestamp in case the 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.
