From: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> |
---|---|
To: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
Cc: | Postgres hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Microoptimization of Bitmapset usage in postgres_fdw |
Date: | 2018-05-30 13:36:35 |
Message-ID: | CAFjFpRdvh-pzT83Q+7hJvVgbMOjHdevach38swVWOpX=cOSLHw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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().
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.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
From | Date | Subject | |
---|---|---|---|
Next Message | Antonin Houska | 2018-05-30 13:45:32 | Re: Incorrect visibility test function assigned to snapshot |
Previous Message | Alvaro Herrera | 2018-05-30 13:28:54 | Re: Incorrect visibility test function assigned to snapshot |