Re: Don't use bms_membership in places where it's not needed
On Tue, 28 Nov 2023 at 11:21, Andres Freund wrote: > Hm, does this ever matter from a performance POV? The current code does look > simpler to read to me. If the overhead is relevant, I'd instead just move the > code into a static inline? I didn't particularly find the code in examine_variable() easy to read. I think what's there now is quite a bit better than what was there. bms_get_singleton_member() was added in d25367ec4 for this purpose, so it seems kinda weird not to use it. David
Re: Don't use bms_membership in places where it's not needed
Hi,
On 2023-11-24 17:06:25 +1300, David Rowley wrote:
> While working on the patch in [1], I noticed that ever since
> 00b41463c, it's now suboptimal to do the following:
>
> switch (bms_membership(relids))
> {
> case BMS_EMPTY_SET:
>/* handle empty set */
>break;
> case BMS_SINGLETON:
> /* call bms_singleton_member() and handle singleton set */
> break;
> case BMS_MULTIPLE:
>/* handle multi-member set */
>break;
> }
>
> The following is cheaper as we don't need to call bms_membership() and
> bms_singleton_member() for singleton sets. It also saves function call
> overhead for empty sets.
>
> if (relids == NULL)
>/* handle empty set */
> else
> {
> int relid;
>
> if (bms_get_singleton(relids, &relid))
> /* handle singleton set */
>else
>/* handle multi-member set */
> }
Hm, does this ever matter from a performance POV? The current code does look
simpler to read to me. If the overhead is relevant, I'd instead just move the
code into a static inline?
Greetings,
Andres Freund
Re: Don't use bms_membership in places where it's not needed
On Fri, 24 Nov 2023 at 19:54, Richard Guo wrote: > +1 to the idea. > > I think you have a typo in distribute_restrictinfo_to_rels. We should > remove the call of bms_singleton_member and use relid instead. Thanks for reviewing. I've now pushed this. David
Re: Don't use bms_membership in places where it's not needed
On Fri, Nov 24, 2023 at 12:06 PM David Rowley wrote: > In the attached, I've adjusted the code to use the latter of the two > above methods in 3 places. In examine_variable() this reduces the > complexity of the logic quite a bit and saves calling bms_is_member() > in addition to bms_singleton_member(). +1 to the idea. I think you have a typo in distribute_restrictinfo_to_rels. We should remove the call of bms_singleton_member and use relid instead. --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -2644,7 +2644,7 @@ distribute_restrictinfo_to_rels(PlannerInfo *root, * There is only one relation participating in the clause, so it * is a restriction clause for that relation. */ - rel = find_base_rel(root, bms_singleton_member(relids)); + rel = find_base_rel(root, relid); Thanks Richard
