Re: pg_stat_activity crashes

At Thu, 21 Apr 2016 14:05:28 -0400, Robert Haas <robertmhaas(at)gmail(dot)com> wrote in <CA+TgmobYX6+d695SE3-UZ1HL326at-9sr4JzQxBW3GAW4exjEA(at)mail(dot)gmail(dot)com>
> >>> Here is PoC patch which fixes the problem. I am wondering if we should
> >>> raise warning in the pg_stat_get_backend_wait_event_type() and
> >>> pg_stat_get_backend_wait_event() like the pg_signal_backend() does
> >>> when proc is NULL instead of just returning NULL which is what this
> >>> patch does though.
> >>
> >> It still makes the two relevant columns in pg_stat_activity
> >> inconsistent each other since it reads the procarray entry twice
> >> without a lock on procarray.
> >>
> >> The attached patch adds pgstat_get_wait_event_info to read
> >> wait_event_info in more appropriate way. Then change
> >> pg_stat_get_wait_event(_type) to take the wait_event_info.
> >>
> >> Does this work for you?
> >
> > This is a hideously way of fixing this problem. The whole point of
> > storing the wait event in a 4-byte integer is that we already assume
> > reads of 4 byte integers are atomic and thus no lock is needed.

A lock was already held in BackendPidGetProc(). Is it also
needless? If so, we should use BackendPidGetProcWithLock() instad
(the name seems a bit confusing, though).

What I did in the patch was just extending the lock duration
until reading the pointer proc. I didn't added any additional
lock.

> > The
> > only thing we need to do is to prevent the value from being read
> > twice, and we already have precedent for how to prevent that in
> > freelist.c.

However, I don't have objections for the patch applied.

> That was intended to say "a hideously expensive way".

Thanks for the kind suppliment..

--
Kyotaro Horiguchi
NTT Open Source Software Center