From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Resetting PGPROC atomics in ProcessInit() |
Date: | 2018-11-02 09:50:12 |
Message-ID: | CAA4eK1+YpmYNCnHoDCq854OrtXmG9-+pQutTnXDGFHW1HYT-CQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sat, Oct 27, 2018 at 9:42 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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?
>
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
From | Date | Subject | |
---|---|---|---|
Next Message | Andrey Lepikhov | 2018-11-02 10:06:32 | Re: Making all nbtree entries unique by having heap TIDs participate in comparisons |
Previous Message | Evgeniy Efimkin | 2018-11-02 09:34:31 | Special role for subscriptions |