Re: Calling pgstat_report_wait_end() before ereport(ERROR)

On Tue, Apr 16, 2019 at 2:45 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Apr 12, 2019 at 10:06:41PM +0900, Masahiko Sawada wrote:
> > But I think that's not right, I've checked the code. If the startup
> > process failed in that function it raises a FATAL and recovery fails,
> > and if checkpointer process does then it calls
> > pgstat_report_wait_end() in CheckpointerMain().
>
> Well, the point is that the code raises an ERROR, then a FATAL because
> it gets upgraded by recovery.  The take, at least it seems to me, is
> that if any new caller of the function misses to clean up the event
> then the routine gets cleared.  So it seems to me that the current
> coding is aimed to be more defensive than anything.  I agree that
> there is perhaps little point in doing so.  In my experience a backend
> switches very quickly back to ClientRead, cleaning up the previous
> event.  Looking around, we have also some code paths in slot.c and
> origin.c which close a transient file, clear the event flag...  And
> then PANIC, which makes even less sense.
>
> In short, I tend to think that the attached is an acceptable cleanup.
> Thoughts?

Agreed. There are also some code which raise an ERROR after close a
transient file but I think it's a good idea to not include them for
safety. It looks to me that the patch you proposed cleans places as
much as we can do.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center