Thread: Resetting PGPROC atomics in ProcessInit()
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? If they're not, we'd be in deep trouble anyway, no? Greetings, Andres Freund
On Sat, Oct 27, 2018 at 4:11 PM Andres Freund <andres@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. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On October 27, 2018 3:36:45 PM GMT+01:00, Amit Kapila <amit.kapila16@gmail.com> wrote: >On Sat, Oct 27, 2018 at 4:11 PM Andres Freund <andres@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 (whichcould reset the spinlock inside while somebody else holds it). It's not really a problem for me, but I think the code is pretty much wrong like this... Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Sat, Oct 27, 2018 at 8:52 PM Andres Freund <andres@anarazel.de> wrote: > On October 27, 2018 3:36:45 PM GMT+01:00, Amit Kapila <amit.kapila16@gmail.com> wrote: > >On Sat, Oct 27, 2018 at 4:11 PM Andres Freund <andres@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? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Sat, Oct 27, 2018 at 9:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Sat, Oct 27, 2018 at 8:52 PM Andres Freund <andres@anarazel.de> wrote: > > On October 27, 2018 3:36:45 PM GMT+01:00, Amit Kapila <amit.kapila16@gmail.com> wrote: > > >On Sat, Oct 27, 2018 at 4:11 PM Andres Freund <andres@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
On Sat, Oct 27, 2018 at 6:41 AM Andres Freund <andres@anarazel.de> wrote: > 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? If they're not, we'd be in deep > trouble anyway, no? I think you are correct. I think it would be better in general for InitProcess() to Assert() rather than reinitializing. Apart from this issue, it's not free. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sun, Nov 4, 2018 at 6:30 AM Robert Haas <robertmhaas@gmail.com> wrote: > > On Sat, Oct 27, 2018 at 6:41 AM Andres Freund <andres@anarazel.de> wrote: > > 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? If they're not, we'd be in deep > > trouble anyway, no? > > I think you are correct. I think it would be better in general for > InitProcess() to Assert() rather than reinitializing. > Okay, changed the code as per Andres's and your suggestion. Do you think the attached change makes sense? I think we should backpatch this. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Attachment
On Thu, Nov 8, 2018 at 4:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Sun, Nov 4, 2018 at 6:30 AM Robert Haas <robertmhaas@gmail.com> wrote: > > On Sat, Oct 27, 2018 at 6:41 AM Andres Freund <andres@anarazel.de> wrote: > > > 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? If they're not, we'd be in deep > > > trouble anyway, no? > > > > I think you are correct. I think it would be better in general for > > InitProcess() to Assert() rather than reinitializing. > > > > Okay, changed the code as per Andres's and your suggestion. Do you > think the attached change makes sense? I think we should backpatch > this. > For 10 and 9.6, we need a slightly different patch as the change of group clog update went in 11. Attached are the patches for the same, note that there is a slight change in the commit message for the patch written for 10 and 9.6. I will wait for a few days (till Tuesday@8:00 AM IST) to see if somebody has any comments or want to review before I push. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Attachment
On Fri, Nov 9, 2018 at 10:28 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Thu, Nov 8, 2018 at 4:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Sun, Nov 4, 2018 at 6:30 AM Robert Haas <robertmhaas@gmail.com> wrote: > > > On Sat, Oct 27, 2018 at 6:41 AM Andres Freund <andres@anarazel.de> wrote: > > Okay, changed the code as per Andres's and your suggestion. Do you > > think the attached change makes sense? I think we should backpatch > > this. > > > > For 10 and 9.6, we need a slightly different patch as the change of > group clog update went in 11. Attached are the patches for the same, > note that there is a slight change in the commit message for the patch > written for 10 and 9.6. I will wait for a few days (till Tuesday@8:00 > AM IST) to see if somebody has any comments or want to review before I > push. > Pushed. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com