Subject: Re: popcount
To: David Fetter <david@fetter.org>
References: <20201230105535.GJ13234@fetter.org>
 <5ea2932e-50f5-4a74-9550-5e3291b6652b@manitou-mail.org>
 <20201230164108.GK13234@fetter.org>
 <eb416bd3-9ee2-8a81-8cd4-d61baaf93ec2@enterprisedb.com>
 <20210111161354.GA8721@fetter.org>
Cc: Daniel Verite <daniel@manitou-mail.org>,
 PostgreSQL Development <pgsql-hackers@postgresql.org>,
 Vik Fearing <vik.fearing@2ndquadrant.com>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Organization: EnterpriseDB
Message-ID: <e48355d3-efaf-e762-fffa-4b2191680292@enterprisedb.com>
Date: Mon, 18 Jan 2021 10:24:12 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:52.0)
 Gecko/20100101 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20210111161354.GA8721@fetter.org>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2021-01-11 17:13, David Fetter wrote:
> On Mon, Jan 11, 2021 at 03:50:54PM +0100, Peter Eisentraut wrote:
>> On 2020-12-30 17:41, David Fetter wrote:
>>>> The input may have more than 2 billion bits set to 1. The biggest possible
>>>> result should be 8 billion for bytea (1 GB with all bits set to 1).
>>>> So shouldn't this function return an int8?
>>> It does now, and thanks for looking at this.
>>
>> The documentation still reflects the previous int4 return type (in two
>> different spellings, too).
> 
> Thanks for looking this over!
> 
> Please find attached the next version with corrected documentation.

The documentation entries should be placed in alphabetical order in 
their respective tables.

For the bytea function, maybe choose a simpler example that a reader can 
compute in their head.  Also for the test cases.

In varbit.c:

The comment says

+ * Returns the number of bits set in a bit array.

but it should be "bit string".

+   int8        popcount;

should be int64.

Also, pg_popcount() returns uint64, not int64.  Perhaps overflow is not 
possible here, but perhaps a small comment about why or an assertion 
could be appropriate.

+   p = VARBITS(arg1);

Why not assign that in the declaration block as well?

This comment:

+   /*
+    * ceil(VARBITLEN(ARG1)/BITS_PER_BYTE)
+    * done to minimize branches and instructions.
+    */

I don't know what that is supposed to mean or why that kind of tuning 
would be necessary for a user-callable function.

+   popcount = pg_popcount((const char *)p, len);

The "const" is probably not necessary.

byteapopcount() looks better, but also needs int8 -> int64.