Horton

Thread

Commits

  1. Fix the initialization of atomic variables introduced by the

  2. Fix the initialization of atomic variable introduced by the

  1. Resetting PGPROC atomics in ProcessInit()

    Andres Freund <andres@anarazel.de> — 2018-10-27T10:41:38Z

    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

  2. Re: Resetting PGPROC atomics in ProcessInit()

    Amit Kapila <amit.kapila16@gmail.com> — 2018-10-27T14:36:45Z

    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

  3. Re: Resetting PGPROC atomics in ProcessInit()

    Andres Freund <andres@anarazel.de> — 2018-10-27T15:22:03Z

    
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.

  4. Re: Resetting PGPROC atomics in ProcessInit()

    Amit Kapila <amit.kapila16@gmail.com> — 2018-10-27T16:12:12Z

    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

  5. Re: Resetting PGPROC atomics in ProcessInit()

    Amit Kapila <amit.kapila16@gmail.com> — 2018-11-02T09:50:12Z

    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

  6. Re: Resetting PGPROC atomics in ProcessInit()

    Robert Haas <robertmhaas@gmail.com> — 2018-11-04T01:00:11Z

    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

  7. Re: Resetting PGPROC atomics in ProcessInit()

    Amit Kapila <amit.kapila16@gmail.com> — 2018-11-08T11:08:25Z

    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

  8. Re: Resetting PGPROC atomics in ProcessInit()

    Amit Kapila <amit.kapila16@gmail.com> — 2018-11-09T04:58:36Z

    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

  9. Re: Resetting PGPROC atomics in ProcessInit()

    Amit Kapila <amit.kapila16@gmail.com> — 2018-11-13T05:48:11Z

    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