Re: Change xl_hash_vacuum_one_page.ntuples from int to uint16 - Mailing list pgsql-hackers

Hi,

On 2/27/23 6:27 AM, Amit Kapila wrote:
> On Fri, Feb 17, 2023 at 7:44 PM Drouvot, Bertrand
> <bertranddrouvot.pg@gmail.com> wrote:
>>
>> On 2/16/23 1:26 PM, Drouvot, Bertrand wrote:
>>> Hi,
>>>
>>> On 2/16/23 12:00 PM, Amit Kapila wrote:
>>>
>>>> BTW, feel free to create the second patch
>>>> (to align the types for variables/arguments) as that would be really
>>>> helpful after we commit this one.
>>
> 
> Pushed the first patch.

Thanks!

> 
>> Please find attached a patch proposal to do so.
>>
>> It looks like a Pandora's box as it produces
>> those cascading changes:
>>
>>       _hash_vacuum_one_page
>>               index_compute_xid_horizon_for_tuples
>>                       gistprunepage
>>                               PageIndexMultiDelete
>>                               gistXLogDelete
>>               PageIndexMultiDelete
>>                       spgRedoVacuumRedirect
>>                       vacuumRedirectAndPlaceholder
>>                       spgPageIndexMultiDelete
>>                               moveLeafs
>>                               doPickSplit
>>                       _bt_delitems_vacuum
>>                               btvacuumpage
>>                       _bt_delitems_delete
>>                               _bt_delitems_delete_check
>>                       hash_xlog_move_page_contents
>>                       gistvacuumpage
>>                               gistXLogUpdate
>>                                       gistplacetopage
>>                       hashbucketcleanup
>>
>>
>> Which makes me:
>>
>> - wonder it is not too intrusive (we could reduce the scope and keep the
>> PageIndexMultiDelete()'s nitems argument as an int for example).
>>
>> - worry if there is no side effects (like the one I'm mentioning as a comment
>> in PageIndexMultiDelete()) even if it passes the CI tests.
>>
>> - wonder if we should not change MaxIndexTuplesPerPage from int to uint16 too (given
>> the fact that the maximum block size is 32 KB.
>>
>> I'm sharing this version but I still need to think about it and
>> I'm curious about your thoughts too.
>>
> 
> @@ -591,11 +591,11 @@ hash_xlog_move_page_contents(XLogReaderState *record)
> 
>    if (len > 0)
>    {
> - OffsetNumber *unused;
> - OffsetNumber *unend;
> + uint16 *unused;
> + uint16 *unend;
> 
> - unused = (OffsetNumber *) ptr;
> - unend = (OffsetNumber *) ((char *) ptr + len);
> + unused = (uint16 *) ptr;
> + unend = (uint16 *) ((char *) ptr + len);
> 
> It doesn't seem useful to me to make such changes.

Yeah, the OffsetNumber is currently defined as uint16, but I wonder if it's
not better that those matches the functions arguments types they are linked to (should OffsetNumber
or the functions arguments types change).

> About other changes
> in the second patch, it is not clear whether there is much value
> addition by those even though I don't see anything wrong with them.
> So, let's see if Nathan or others see the value in the proposed patch
> or any subset of these changes.
> 

+1.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com