Thread: updated GiST patch

This is an updated version of the GiST patch I posted a few months ago.
As before, it makes the following changes:

- ensure that all user-supplied GiST methods are invoked in a
short-lived memory context. Therefore, explicitly releasing palloc'ed
memory via pfree is unnecessary (and probably results in worse
performance). This lowers the barrier to entry for writing GiST-based
indexes.

- change GiST so that we keep a pin on a scan's current buffer, rather
than doing ReadBuffer() for each tuple produced by the scan.
ReadBuffer() is relatively expensive, so this is a win.

- the previous change makes it pretty easy to implement dead tuple
killing for GiST (which means that all the builtin indexes now do this).

The patch also cleans up a lot of pretty ugly code in GiST. AFAIK all
existing GiST-based indexes should continue to work unchanged -- the
regression tests for all the contrib/ indexes pass, at any rate.

Barring any objections I'll apply this to HEAD tomorrow or the day after.

-Neil

Neil Conway wrote:
> This is an updated version of the GiST patch I posted a few months ago.

Attached is a revised version. This update fixes some newly-added bugs
in mark and restore (I forgot to save and restore the current buffer),
and replaces two ReleaseBuffer() + ReadBuffer() pairs with
ReleaseAndReadBuffer(). (Is this still worth doing? It seems it no
longer saves a lock acquire/release, but perhaps it will again be a win
in some future version of the bufmgr...)

BTW, this idiom occurs a few times:

     if (BufferIsValid(buf))
     {
         ReleaseBuffer(buf);
         buf = InvalidBuffer;
     }

it would be nice to make this more concise. Perhaps:

     InvalidateBuffer(&buf);

although that doesn't make the modification of `buf' obvious. An
alternative would be to have ReleaseBuffer() always return
InvalidBuffer, so:

     if (BufferIsValid(buf))
         buf = ReleaseBuffer(buf);

Any thoughts on this? (I'm inclined to prefer InvalidateBuffer().)

-Neil

Neil Conway <neilc@samurai.com> writes:
> ... replaces two ReleaseBuffer() + ReadBuffer() pairs with
> ReleaseAndReadBuffer(). (Is this still worth doing? It seems it no
> longer saves a lock acquire/release, but perhaps it will again be a win
> in some future version of the bufmgr...)

I think there is no longer any noticeable win except in the case where
the old and new pages are actually the same.  Which is probably unlikely
to be true inside an index AM (it is a useful win for random access into
a heap for instance).  But as you say it might someday again be useful.
My advice is to combine if and only if you're not contorting the code
to do so.

> BTW, this idiom occurs a few times:

>      if (BufferIsValid(buf))
>      {
>          ReleaseBuffer(buf);
>          buf = InvalidBuffer;
>      }

I'd leave it as-is; ISTM to be more easily understandable than the
alternatives you suggest.

            regards, tom lane

Patch applied.

Tom Lane wrote:
> Neil Conway <neilc@samurai.com> writes:
>>BTW, this idiom occurs a few times:
>
>
>>     if (BufferIsValid(buf))
>>     {
>>         ReleaseBuffer(buf);
>>         buf = InvalidBuffer;
>>     }
>
>
> I'd leave it as-is; ISTM to be more easily understandable than the
> alternatives you suggest.

Yeah, fair enough.

-Neil