Re: WIP: Access method extendability

From: Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>
To: Aleksander Alekseev <a(dot)alekseev(at)postgrespro(dot)ru>
Cc: Teodor Sigaev <teodor(at)sigaev(dot)ru>, Petr Jelinek <petr(at)2ndquadrant(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, David Steele <david(at)pgmasters(dot)net>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: WIP: Access method extendability
Date: 2016-04-01 13:14:43
Message-ID: CAPpHfdvbzeJitmq4nCq_pwtuaa7_rwNoh-4SJ8BskkufaeEqWg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi!

On Fri, Apr 1, 2016 at 12:45 PM, Aleksander Alekseev <
a(dot)alekseev(at)postgrespro(dot)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 Content-Type Size
0003-bloom-contrib.16.patch application/octet-stream 63.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Teodor Sigaev 2016-04-01 13:22:55 Re: [PATCH] Phrase search ported to 9.6
Previous Message Alvaro Herrera 2016-04-01 13:09:38 Re: [PATCH] Phrase search ported to 9.6