On Sat, Oct 27, 2018 at 9:42 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Sat, Oct 27, 2018 at 8:52 PM Andres Freund <and...@anarazel.de> wrote: > > On October 27, 2018 3:36:45 PM GMT+01:00, Amit Kapila > > <amit.kapil...@gmail.com> wrote: > > >On Sat, Oct 27, 2018 at 4:11 PM Andres Freund <and...@anarazel.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? >
You haven't confirmed on this part. Do you want to see this change? I think if we make this change, we should backport this as well and I am not sure if we should make such a change without a strong reason in back-branches. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
