Horton

Thread

Commits

  1. Don't use bms_membership() in cases where we don't need to

  1. Don't use bms_membership in places where it's not needed

    David Rowley <dgrowleyml@gmail.com> — 2023-11-24T04:06:25Z

    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 */
}

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().

I'm trying to reduce the footprint of what's being worked on in [1]
and I highlighted this as something that'll help with that.

Any objections to me pushing the attached?

David

[1] https://postgr.es/m/CAApHDvqHCNKJi9CrQZG-reQDXTfRWnT5rhzNtDQhnrBzAAusfA@mail.gmail.com

  2. Re: Don't use bms_membership in places where it's not needed

    Richard Guo <guofenglinux@gmail.com> — 2023-11-24T06:53:50Z

    On Fri, Nov 24, 2023 at 12:06 PM David Rowley <dgrowleyml@gmail.com> 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

  3. Re: Don't use bms_membership in places where it's not needed

    David Rowley <dgrowleyml@gmail.com> — 2023-11-27T21:43:00Z

    On Fri, 24 Nov 2023 at 19:54, Richard Guo <guofenglinux@gmail.com> 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

  4. Re: Don't use bms_membership in places where it's not needed

    Andres Freund <andres@anarazel.de> — 2023-11-27T22:21:34Z

    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

  5. Re: Don't use bms_membership in places where it's not needed

    David Rowley <dgrowleyml@gmail.com> — 2023-11-27T23:16:21Z

    On Tue, 28 Nov 2023 at 11:21, Andres Freund <andres@anarazel.de> 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