Hi! On Fri, Apr 1, 2016 at 12:45 PM, Aleksander Alekseev < a.aleks...@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
0003-bloom-contrib.16.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
