Re: Microoptimization of Bitmapset usage in postgres_fdw

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
Cc: Postgres hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Microoptimization of Bitmapset usage in postgres_fdw
Date: 2018-05-30 14:37:27
Message-ID: BB9AAEE6-294F-4296-A8F2-70AEE9EF3E70@yesql.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 30 May 2018, at 09:36, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>
> On Tue, May 29, 2018 at 9:10 PM, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>> There are a couple of places in postgres_fdw where we check if the Bitmapset
>> has multiple members using bms_num_members(), without storing the returned
>> count. The attached patch instead use bms_membership() which is optimized for
>> just that usecase, and (IMO) makes for clearer code.
>
> +1 for that change. Some of those usages of bms_num_members() were
> added by me. Sorry for that. It was mostly because I wasn't aware of
> bms_membership() when I wrote that code. May be we should add a
> comment in the prologue of bms_num_members() like "Note: if the
> callers is interested only knowing whether the bitmapset has 0, 1 or
> more members, it should call bms_membership().". I understand that
> bms_membership() resides just below bms_num_members(), but 1.
> bms_membership doesn't sound like it would tell me that 2.
> bms_membership's prologue refers to bms_num_members, which should have
> been the other way; we want the developers to use bms_membership
> instead of bms_num_members(), when they land on bms_num_members. It's
> less likely that somebody landing on bms_membership would want to use
> bms_num_members().

That makes sense, I’ve added a second patch to this thread which expands the
comment on bms_num_members to make it clearer.

> I am not sure if this can b e squeezed into HEAD right now. It looks
> safe to do so. But in case not, please add this to the next commitfest
> so that it's not forgotten.

Will do, thanks for reviewing.

cheers ./daniel

Attachment Content-Type Size
bms_num_members_comment-v2.patch application/octet-stream 831 bytes
postgres_fdw_bms_multiple-v2.patch application/octet-stream 1.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2018-05-30 15:22:47 Re: PATCH pass PGOPTIONS to pg_regress
Previous Message Antonin Houska 2018-05-30 14:00:44 Re: Incorrect visibility test function assigned to snapshot