Thread

1. myProcLocks initialization

   Robert Haas <robertmhaas@gmail.com> — 2011-10-30T22:19:45Z

   I'd like to propose the attached patch, which initializes each
   PGPROC's myProcLocks just once at postmaster startup, rather than
   every time the PGPROC is handed out to a backend.  These lists should
   always be emptied before a backend shuts down, so a newly initialized
   backend will find the lists empty anyway.  Not reinitializing them
   shaves a few cycles.  In my testing, it saves about 1% of the cost of
   setting up and tearing down a connection, which is not a ton, but a
   cycle saved is a cycle earned.
   
   Of course, we have a few outstanding reports, like this one from Dave
   Gould, indicating that maybe we have a bug in there somewhere:
   
   http://archives.postgresql.org/message-id/20110822073131.GC3363@sonic.net
   
   ...but in that case it seems to me that this doesn't make anything
   worse than it already is.  If the myProcLocks pointers are pointing to
   random garbage, we're just kidding ourselves whatever we do; the
   system is screwed, and we ought to PANIC, and anything we do here is
   laughably inadequate.  OTOH, if it just so happens that a backend
   found a sneaky way through the exit path that doesn't involve calling
   LockReleaseAll(), then overwriting the shm queue pointers removes our
   last hope that the next backend can clean up the mess.  I'm not
   putting a lot of faith in that actually working, just saying that the
   current code doesn't seem to be accomplishing anything in the
   robustness department.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   

2. Re: myProcLocks initialization

   Tom Lane <tgl@sss.pgh.pa.us> — 2011-10-31T03:13:51Z

   Robert Haas <robertmhaas@gmail.com> writes:
   > I'd like to propose the attached patch, which initializes each
   > PGPROC's myProcLocks just once at postmaster startup, rather than
   > every time the PGPROC is handed out to a backend.  These lists should
   > always be emptied before a backend shuts down, so a newly initialized
   > backend will find the lists empty anyway.  Not reinitializing them
   > shaves a few cycles.  In my testing, it saves about 1% of the cost of
   > setting up and tearing down a connection, which is not a ton, but a
   > cycle saved is a cycle earned.
   
   That's not really enough to excite me, and the prospect of problems in
   one session corrupting an unrelated later one is pretty scary from a
   debugging standpoint.  How about at least an Assert that the lock is in
   a clean state?
   
   			regards, tom lane
   
   

3. Re: myProcLocks initialization

   Robert Haas <robertmhaas@gmail.com> — 2011-10-31T03:26:08Z

   On Sun, Oct 30, 2011 at 11:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > Robert Haas <robertmhaas@gmail.com> writes:
   >> I'd like to propose the attached patch, which initializes each
   >> PGPROC's myProcLocks just once at postmaster startup, rather than
   >> every time the PGPROC is handed out to a backend.  These lists should
   >> always be emptied before a backend shuts down, so a newly initialized
   >> backend will find the lists empty anyway.  Not reinitializing them
   >> shaves a few cycles.  In my testing, it saves about 1% of the cost of
   >> setting up and tearing down a connection, which is not a ton, but a
   >> cycle saved is a cycle earned.
   >
   > That's not really enough to excite me, and the prospect of problems in
   > one session corrupting an unrelated later one is pretty scary from a
   > debugging standpoint.  How about at least an Assert that the lock is in
   > a clean state?
   
   I can go for that.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

4. Re: myProcLocks initialization

   Robert Haas <robertmhaas@gmail.com> — 2011-10-31T19:54:57Z

   On Sun, Oct 30, 2011 at 11:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:
   > On Sun, Oct 30, 2011 at 11:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   >> Robert Haas <robertmhaas@gmail.com> writes:
   >>> I'd like to propose the attached patch, which initializes each
   >>> PGPROC's myProcLocks just once at postmaster startup, rather than
   >>> every time the PGPROC is handed out to a backend.  These lists should
   >>> always be emptied before a backend shuts down, so a newly initialized
   >>> backend will find the lists empty anyway.  Not reinitializing them
   >>> shaves a few cycles.  In my testing, it saves about 1% of the cost of
   >>> setting up and tearing down a connection, which is not a ton, but a
   >>> cycle saved is a cycle earned.
   >>
   >> That's not really enough to excite me, and the prospect of problems in
   >> one session corrupting an unrelated later one is pretty scary from a
   >> debugging standpoint.  How about at least an Assert that the lock is in
   >> a clean state?
   >
   > I can go for that.
   
   Revised patch attached.  I think it would be useful to assert this
   both at process startup time and at process shutdown, since it would
   really be much nicer to have the process that didn't clean up fail the
   assertion, rather than the new one that innocently inherited its slot;
   so the attached patch takes that approach.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   

5. Re: myProcLocks initialization

   Tom Lane <tgl@sss.pgh.pa.us> — 2011-10-31T20:03:47Z

   Robert Haas <robertmhaas@gmail.com> writes:
   > Revised patch attached.  I think it would be useful to assert this
   > both at process startup time and at process shutdown, since it would
   > really be much nicer to have the process that didn't clean up fail the
   > assertion, rather than the new one that innocently inherited its slot;
   > so the attached patch takes that approach.
   
   +1
   
   			regards, tom lane
   
   

6. Re: myProcLocks initialization

   Simon Riggs <simon@2ndquadrant.com> — 2011-10-31T20:18:26Z

   On Mon, Oct 31, 2011 at 7:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
   
   > Revised patch attached.  I think it would be useful to assert this
   > both at process startup time and at process shutdown, since it would
   > really be much nicer to have the process that didn't clean up fail the
   > assertion, rather than the new one that innocently inherited its slot;
   > so the attached patch takes that approach.
   
   Something stronger than an assertion at shutdown? Run-time test?
   
   -- 
    Simon Riggs                   http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
   
   

7. Re: myProcLocks initialization

   Tom Lane <tgl@sss.pgh.pa.us> — 2011-10-31T20:22:45Z

   Simon Riggs <simon@2ndQuadrant.com> writes:
   > On Mon, Oct 31, 2011 at 7:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
   >> Revised patch attached. I think it would be useful to assert this
   >> both at process startup time and at process shutdown, since it would
   >> really be much nicer to have the process that didn't clean up fail the
   >> assertion, rather than the new one that innocently inherited its slot;
   >> so the attached patch takes that approach.
   
   > Something stronger than an assertion at shutdown? Run-time test?
   
   There's currently no evidence to suggest this will ever fire at all,
   especially not in non-development builds, so an assert seems enough
   to me.
   
   			regards, tom lane