Re: make BufferGetBlockNumber() a macro - Mailing list pgsql-patches

On Mon, 01 Apr 2002 17:59:06 -0500
"Tom Lane" <[email protected]> wrote:
> Neil Conway <[email protected]> writes:
> > The attached patch re-implements BufferGetBlockNumber() as a macro,
> > for performance reasons.
>
> The trouble with this is that it forces storage/buf_internals.h to
> become part of bufmgr.h, thereby completely destroying any information
> hiding that we had from the division of the two headers --- any client
> of bufmgr.h might now access bufmgr internal stuff without noticing
> that it was doing wrong.

Yeah, I considered that. IMHO, this isn't that big of a drawback:
C's information hiding is weak enough that you need to rely on the
client programmer to exercise good judgement anyway; for example, there
is absolutely nothing to stop someone from just including
storage/buf_internals.h to begin with. Furthermore, in order to actually
_see_ the declarations in buf_internals.h, a client programmer still
needs to look at it, not bufmgr.h (i.e. the #include only effects
cpp).

If you prefer, I can move the definition of BufferDesc from
storage/buf_internals.h to storage/bufmgr.h; this still destroys
a little of the information hiding of the old code, but if a client
programmer is stupid enough to manipulate a structure that is
marked as internal and isn't packaged with any support functions
or other code for dealing with it, they deserve their code getting
broken.

> I am not convinced that BufferGetBlockNumber is a bottleneck anyway;
> at least not if you don't have Assert checking enabled.

I didn't have assert checking enabled. As for it being a bottleneck,
I'm sure the reason hash index creation is so slow lies elsewhere;
but that doesn't stop this from being a performance improvement.

> There are a few places that do things like
>     ItemPointerSet(&(tuple->t_self), BufferGetBlockNumber(buffer), offnum);
> which I believe results in two calls to BufferGetBlockNumber because of
> the way BlockIdSet is coded.  This would be good to clean up, but it's
> BlockIdSet's problem not BufferGetBlockNumber's.

If BufferGetBlockNumber is inlined (i.e. implemented as either a macro
or a C99 inline function), it becomes just a conditional and an array
access, so there's little need to optimize the usage of it any further.
As it stands, a full function call is quite a lot of overhead,
particularly for something that is called so often (at least for the
situation I was looking at).

> > It also adds an assertion that should probably
> > be present.
>
> FWIW, BufferIsPinned also checks BufferIsValid; you do not need both.

Ah, ok. So BufferIsPinned() is the correct assertion. An updated patch
is attached. Also, I removed an obsolete comment + line of code from
storage/buf.h, and deleted an incoherent comment from storage/bufmgr.h

Cheers,

Neil

--
Neil Conway <[email protected]>
PGP Key ID: DB3C29FC