Re: WIP: Access method extendability - Mailing list pgsql-hackers

From Alexander Korotkov
Subject Re: WIP: Access method extendability
Date
Msg-id CAPpHfdvbzeJitmq4nCq_pwtuaa7_rwNoh-4SJ8BskkufaeEqWg@mail.gmail.com
Whole thread Raw
In response to Re: WIP: Access method extendability  (Aleksander Alekseev <a.alekseev@postgrespro.ru>)
Responses Re: WIP: Access method extendability  (Aleksander Alekseev <a.alekseev@postgrespro.ru>)
List pgsql-hackers
Hi!

On Fri, Apr 1, 2016 at 12:45 PM, Aleksander Alekseev <a.alekseev@postgrespro.ru> wrote:
Code looks much better now, thanks. Still I believe it could be improved.

I don't think that using srand() / rand() in signValue procedure the
way you did is such a good idea. You create a side affect (changing
current randseed) which could cause problems in some cases. And there
is no real need for that. For instance you could use following formula
instead:

hash(attno || hashVal || j)

I've discussed this with Teodor privately.  Extra hash calculation could cause performance regression.  He proposed to use own random generator instead.  Implemented in attached version of patch.
 
And a few more things.

> +     memset(opaque, 0, sizeof(BloomPageOpaqueData));
> +     opaque->maxoff = 0;
 
This looks a bit redundant.

Fixed.
 
> + for (my $i = 1; $i <= 10; $i++)

More idiomatic Perl would be `for my $i (1..10)`.

Fixed.
 
> +                     UnlockReleaseBuffer(buffer);
> +                     ReleaseBuffer(metaBuffer);
> +                     goto away;

In general I don't have anything against goto. But are you sure that
using it here is really justified?

Fixed with small code duplication which seems to be better than goto.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 
Attachment

pgsql-hackers by date:

Previous
From: Alvaro Herrera
Date:
Subject: Re: [PATCH] Phrase search ported to 9.6
Next
From: Teodor Sigaev
Date:
Subject: Re: [PATCH] Phrase search ported to 9.6