Thread: Did we forget to unpin buf in function "revmap_physical_extend" ?

Jinyu Zhang wrote:
> In function "revmap_physical_extend",  should we add "ReleaseBuffer(buf);" between line 438 and 439 ?
>  422     else
>  423     {
>  424         if (needLock)
>  425             LockRelationForExtension(irel, ExclusiveLock);
>  426 
>  427         buf = ReadBuffer(irel, P_NEW);
>  428         if (BufferGetBlockNumber(buf) != mapBlk)
>  429         {
>  430             /*
>  431              * Very rare corner case: somebody extended the relation
>  432              * concurrently after we read its length.  If this happens, give
>  433              * up and have caller start over.  We will have to evacuate that
>  434              * page from under whoever is using it.
>  435              */
>  436             if (needLock)
>  437                 UnlockRelationForExtension(irel, ExclusiveLock);
>  438             LockBuffer(revmap->rm_metaBuf, BUFFER_LOCK_UNLOCK);
>  439             return;
>  440         }

Yeah, clearly there's a missing ReleaseBuffer call there.

There are two callers of this code (brinRevmapExtend calls
revmap_extend_and_get_blkno calls revmap_physical_extend) -- one is
brin_doupdate.  This function is concerned with updating an existing
index tuple, that is, one that already has a revmap entry.  It follows
that the revmap page must already exist.  In other words, this callsite
is dead code.

The other caller is in brin_doinsert, which itself is called from
brinbuild (both directly and via brinbuildCallback) and summarize_range.
In the first case, the index is just being created, and therefore no one
can be trying to extend the relation concurrently.  In the second case,
we always hold ShareUpdateExclusiveLock, and therefore no one can be
also running the same thing.

Another thing to keep in mind is that since the revmap allocates blocks
from the start of the main fork, the pages it wants to use are most of
the time already used by "regular index pages".  The only case where it
actually needs to physically extend the relation is when the index is
new.  I've been trying to think of a case in which the index is created
to an empty relation, and then we try to insert a tuple during a
vacuum-induced summarization, which decides to create a new revmap page,
and then somehow another process needs to insert another index tuple and
allocate a regular index page for it, which happens to get to the
relation extension before the first process.  But I don't think this can
happen either, because every path that inserts regular index tuples
extends the revmap first.

Therefore I think this is dead code and there is no live bug.

All that said, adding the call is simple enough and I don't want to
spend a lot of time constructing a test case to prove that this cannot
happen.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services