Re: Strange Bitmapset manipulation in DiscreteKnapsack()

From: Richard Guo <guofenglinux(at)gmail(dot)com>
To: David Rowley <dgrowleyml(at)gmail(dot)com>
Cc: PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Strange Bitmapset manipulation in DiscreteKnapsack()
Date: 2024-01-16 09:18:24
Message-ID: CAMbWs49tF04AytSsrEN7zte7CPxurF27gURYLHdhhTr7i1Cw0g@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Jan 16, 2024 at 11:32 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

> While working on [1], I noticed some strange code in
> DiscreteKnapsack() which seems to be aiming to copy the Bitmapset.
>
> It's not that obvious at a casual glance, but:
>
> sets[j] = bms_del_members(sets[j], sets[j]);
>
> this is aiming to zero all the words in the set by passing the same
> set in both parameters.
>
> Now that 00b41463c changed Bitmapset to have NULL be the only valid
> representation of an empty set, this code no longer makes sense. We
> may as well just bms_free() the original set and bms_copy() in the new
> set as the bms_del_members() call will always pfree the set anyway.
>
> I've done that in the attached.

+1. This is actually what happens with the original code, i.e.
bms_del_members() will pfree sets[j] and bms_add_members() will bms_copy
sets[ow] to sets[j]. But the new code looks more natural.

I also checked other callers of bms_del_members() and did not find
another case that passes the same set in both parameters.

Thanks
Richard

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Jelte Fennema-Nio 2024-01-16 09:37:22 Re: [PATCH] Add additional extended protocol commands to psql: \parse and \bindx
Previous Message Li, Yong 2024-01-16 09:12:16 Re: Proposal to add page headers to SLRU pages