Re: Resetting PGPROC atomics in ProcessInit()

On Sat, Oct 27, 2018 at 8:52 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On October 27, 2018 3:36:45 PM GMT+01:00, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >On Sat, Oct 27, 2018 at 4:11 PM Andres Freund <andres(at)anarazel(dot)de>
> >wrote:
> >>
> >> Hi,
> >>
> >> I just noticed, while working on a patch adding things to PGPROC,
> >that
> >> the group clearning patches for the proc array and clog reset atomics
> >in
> >> InitProcess().
> >>
> >> I'm not a big fan of that, because it means that it's not safe to
> >look
> >> at the atomics of backends that aren't currently in use. Is there
> >any
> >> reason to not instead initialize them in InitProcGlobal() and just
> >> assert in InitProcess() that they're 0?
> >>
> >
> >It seems the code written has followed a natural practice i.e PGPROC
> >members are initialized in InitProcess and ProcGlobal members (like
> >procArrayGroupFirst) are initialized in InitProcGlobal. For your use
> >case, can't you look at procArrayGroupFirst? If not, then I think we
> >can do what you are saying as I don't see a problem in initializing
> >them in InitProcGlobal.
>
> In my opinion that's an argument for resetting the contents with pg_atomic_write, but not reinitializing the atomic
>

Okay, makes sense.

> (which could reset the spinlock inside while somebody else holds it).
>

This part is not clear to me, how can this happen? I think we only
access these variable for active procs which means no-one can hold it
till it's reinitialized.

> It's not really a problem for me, but I think the code is pretty much wrong like this...
>

I think I understand why it is better to write the way you are
suggesting, but not clear how the current code can lead to a problem,
can you please explain in more detail?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com