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 (which could 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
Attachments:
fix_atomic_init_group_clear_v1.patchapplication/octet-stream; name=fix_atomic_init_group_clear_v1.patchDownload+9-2
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
Attachments:
0001-Fix-the-initialization-of-atomic-variables-introduce.patchapplication/octet-stream; name=0001-Fix-the-initialization-of-atomic-variables-introduce.patchDownload+9-3
0001-Fix-the-initialization-of-atomic-variable-introduced-10.patchapplication/octet-stream; name=0001-Fix-the-initialization-of-atomic-variable-introduced-10.patchDownload+7-2
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