Re: [PATCH] avoid buffer underflow in errfinish()

On 27.03.2013 14:50, Robert Haas wrote:
> On Sat, Mar 23, 2013 at 6:45 PM, Xi Wang<xi.wang@gmail.com>  wrote:
>> A side question: at src/backend/storage/lmgr/proc.c:1150, is there a
>> null pointer deference for `autovac'?

I think you mean on line 1142:

>         PGPROC       *autovac = GetBlockingAutoVacuumPgproc();
> *HERE*    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))


> Not really.  If the deadlock_state is DS_BLOCKED_BY_AUTOVACUUM, there
> has to be a blocking autovacuum proc.  As in the other case that you
> found, though, some code rearrangement would likely make the intent of
> the code more clear and avoid future mistakes.
>
> Perhaps something like:
>
>          if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM&&
> allow_autovacuum_cancel
>              &&  (autovac = GetBlockingAutoVacuumPgproc()) != NULL)
>          {
>              PGXACT     *autovac_pgxact =
> &ProcGlobal->allPgXact[autovac->pgprocno];
>              ...

Writing it like that suggests that autovac might sometimes be NULL, even 
if deadlock_state == DS_BLOCKED_BY_AUTOVACUUM. From your explanation 
above, I gather that's not possible (and I think you're right), so the 
NULL check is unnecessary. If we think it might be NULL after all, the 
above makes sense.

- Heikki