Message-ID: <5598409b-4ce8-dec2-acee-272e94c4b21b@enterprisedb.com>
Date: Fri, 24 Feb 2023 17:04:17 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: PATCH: Using BRIN indexes for sorted output
Content-Language: en-US
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>,
 Justin Pryzby <pryzby@telsasoft.com>, Andres Freund <andres@anarazel.de>,
 Greg Stark <stark@mit.edu>, Zhihong Yu <zyu@yugabyte.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <20230224151416.waqpi5syn36hun2l@alvherre.pgsql>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <20230224151416.waqpi5syn36hun2l@alvherre.pgsql>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/5598409b-4ce8-dec2-acee-272e94c4b21b%40enterprisedb.com>
Precedence: bulk


On 2/24/23 16:14, Alvaro Herrera wrote:
> On 2023-Feb-24, Tomas Vondra wrote:
> 
>> I guess the easiest fix would be to do the arithmetic in 64 bits. That'd
>> eliminate the overflow.
> 
> Yeah, that might be easy to set up.  We then don't have to worry about
> it until BlockNumber is enlarged to 64 bits ... but by that time surely
> we can just grow it again to a 128 bit loop variable.
> 
>> Alternatively, we could do something like
>>
>>   prevHeapBlk = 0;
>>   for (heapBlk = 0; (heapBlk < nblocks) && (prevHeapBlk <= heapBlk);
>>        heapBlk += pagesPerRange)
>>   {
>>     ...
>>     prevHeapBlk = heapBlk;
>>   }
> 
> I think a formulation of this kind has the benefit that it works after
> BlockNumber is enlarged to 64 bits, and doesn't have to be changed ever
> again (assuming it is correct).
> 

Did anyone even propose doing that? I suspect this is unlikely to be the
only place that'd might be broken by that.

> ... if pagesPerRange is not a whole divisor of MaxBlockNumber, I think
> this will neglect the last range in the table.
> 

Why would it? Let's say BlockNumber is uint8, i.e. 255 max. And there
are 10 pages per range. That's 25 "full" ranges, and the last range
being just 5 pages. So we get into

   prevHeapBlk = 240
   heapBlk = 250

and we read the last 5 pages. And then we update

   prevHeapBlk = 250
   heapBlk = (250 + 10) % 255 = 5

and we don't do that loop. Or did I get this wrong, somehow?


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company