Postgres Pro
Downloads
Home   >   mailing lists

Thread: Dead code or buggy code?

Dead code or buggy code?

From
Greg Stark
Date:
The following code is in the ProcSleep at proc.c:1138.
GetBlockingAutoVacuumPgproc() should presumably always return a vacuum
pgproc entry since the deadlock state says it's blocked by autovacuum.
But I'm not really familiar enough with this codepath to know whether
there's not a race condition here where it can sometimes return null.
The following code checks autovac != NULL but the PGXACT initializer
would have seg faulted if it returned NULL if that's possible.
       if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM &&
allow_autovacuum_cancel)       {           PGPROC       *autovac = GetBlockingAutoVacuumPgproc();           PGXACT
*autovac_pgxact =
 
&ProcGlobal->allPgXact[autovac->pgprocno];
           LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
           /*            * Only do it if the worker is not working to protect against Xid            * wraparound.
     */           if ((autovac != NULL) &&               (autovac_pgxact->vacuumFlags & PROC_IS_AUTOVACUUM) &&
    !(autovac_pgxact->vacuumFlags & PROC_VACUUM_FOR_WRAPAROUND))           {
 


-- 
greg

Re: Dead code or buggy code?

From
Robert Haas
Date:
On Wed, Sep 18, 2013 at 6:20 PM, Greg Stark <stark@mit.edu> wrote:
> The following code is in the ProcSleep at proc.c:1138.
> GetBlockingAutoVacuumPgproc() should presumably always return a vacuum
> pgproc entry since the deadlock state says it's blocked by autovacuum.
> But I'm not really familiar enough with this codepath to know whether
> there's not a race condition here where it can sometimes return null.
> The following code checks autovac != NULL but the PGXACT initializer
> would have seg faulted if it returned NULL if that's possible.
>
>         if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM &&
> allow_autovacuum_cancel)
>         {
>             PGPROC       *autovac = GetBlockingAutoVacuumPgproc();
>             PGXACT       *autovac_pgxact =
> &ProcGlobal->allPgXact[autovac->pgprocno];
>
>             LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
>
>             /*
>              * Only do it if the worker is not working to protect against Xid
>              * wraparound.
>              */
>             if ((autovac != NULL) &&
>                 (autovac_pgxact->vacuumFlags & PROC_IS_AUTOVACUUM) &&
>                 !(autovac_pgxact->vacuumFlags & PROC_VACUUM_FOR_WRAPAROUND))
>             {

Hmm, yeah.  I remember noticing this some time ago but never got
around to fixing it.  +1 for rearranging things there somehow.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Dead code or buggy code?

From
Greg Stark
Date:
<p dir="ltr">So I'm just going to make the code defensive and assume NULL is possible when if maybe it isn't. <p
dir="ltr">Incase it's not clear, this is one of the thing's Xi Wang's took picked up. There not to many but it turns
outthey are indeed not all in the adt code so I might wait until after the commit fest to commit it to avoid causing
bitchurn.<p dir="ltr">-- <br /> greg<div class="gmail_quote">On 19 Sep 2013 12:52, "Robert Haas" <<a
href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>>wrote:<br type="attribution" /><blockquote
class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Wed, Sep 18, 2013 at 6:20
PM,Greg Stark <<a href="mailto:stark@mit.edu">stark@mit.edu</a>> wrote:<br /> > The following code is in the
ProcSleepat proc.c:1138.<br /> > GetBlockingAutoVacuumPgproc() should presumably always return a vacuum<br /> >
pgprocentry since the deadlock state says it's blocked by autovacuum.<br /> > But I'm not really familiar enough
withthis codepath to know whether<br /> > there's not a race condition here where it can sometimes return null.<br
/>> The following code checks autovac != NULL but the PGXACT initializer<br /> > would have seg faulted if it
returnedNULL if that's possible.<br /> ><br /> >         if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM
&&<br/> > allow_autovacuum_cancel)<br /> >         {<br /> >             PGPROC       *autovac =
GetBlockingAutoVacuumPgproc();<br/> >             PGXACT       *autovac_pgxact =<br /> >
&ProcGlobal->allPgXact[autovac->pgprocno];<br/> ><br /> >             LWLockAcquire(ProcArrayLock,
LW_EXCLUSIVE);<br/> ><br /> >             /*<br /> >              * Only do it if the worker is not working to
protectagainst Xid<br /> >              * wraparound.<br /> >              */<br /> >             if ((autovac
!=NULL) &&<br /> >                 (autovac_pgxact->vacuumFlags & PROC_IS_AUTOVACUUM) &&<br
/>>                 !(autovac_pgxact->vacuumFlags & PROC_VACUUM_FOR_WRAPAROUND))<br /> >             {<br
/><br/> Hmm, yeah.  I remember noticing this some time ago but never got<br /> around to fixing it.  +1 for rearranging
thingsthere somehow.<br /><br /> --<br /> Robert Haas<br /> EnterpriseDB: <a href="http://www.enterprisedb.com"
target="_blank">http://www.enterprisedb.com</a><br/> The Enterprise PostgreSQL Company<br /></blockquote></div>

Re: Dead code or buggy code?

From
Bruce Momjian
Date:
On Fri, Sep 20, 2013 at 12:13:18AM +0100, Greg Stark wrote:
> So I'm just going to make the code defensive and assume NULL is possible when
> if maybe it isn't.
> 
> In case it's not clear, this is one of the thing's Xi Wang's took picked up.
> There not to many but it turns out they are indeed not all in the adt code so I
> might wait until after the commit fest to commit it to avoid causing bit churn.

Uh, where are we on this?

---------------------------------------------------------------------------


> 
> --
> greg
> 
> On 19 Sep 2013 12:52, "Robert Haas" <robertmhaas@gmail.com> wrote:
> 
>     On Wed, Sep 18, 2013 at 6:20 PM, Greg Stark <stark@mit.edu> wrote:
>     > The following code is in the ProcSleep at proc.c:1138.
>     > GetBlockingAutoVacuumPgproc() should presumably always return a vacuum
>     > pgproc entry since the deadlock state says it's blocked by autovacuum.
>     > But I'm not really familiar enough with this codepath to know whether
>     > there's not a race condition here where it can sometimes return null.
>     > The following code checks autovac != NULL but the PGXACT initializer
>     > would have seg faulted if it returned NULL if that's possible.
>     >
>     >         if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM &&
>     > allow_autovacuum_cancel)
>     >         {
>     >             PGPROC       *autovac = GetBlockingAutoVacuumPgproc();
>     >             PGXACT       *autovac_pgxact =
>     > &ProcGlobal->allPgXact[autovac->pgprocno];
>     >
>     >             LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
>     >
>     >             /*
>     >              * Only do it if the worker is not working to protect against
>     Xid
>     >              * wraparound.
>     >              */
>     >             if ((autovac != NULL) &&
>     >                 (autovac_pgxact->vacuumFlags & PROC_IS_AUTOVACUUM) &&
>     >                 !(autovac_pgxact->vacuumFlags &
>     PROC_VACUUM_FOR_WRAPAROUND))
>     >             {
> 
>     Hmm, yeah.  I remember noticing this some time ago but never got
>     around to fixing it.  +1 for rearranging things there somehow.
> 
>     --
>     Robert Haas
>     EnterpriseDB: http://www.enterprisedb.com
>     The Enterprise PostgreSQL Company
> 

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +