Re: Change xl_hash_vacuum_one_page.ntuples from int to uint16

Hi,

On 2/27/23 6:27 AM, Amit Kapila wrote:
> On Fri, Feb 17, 2023 at 7:44 PM Drouvot, Bertrand
> <bertranddrouvot(dot)pg(at)gmail(dot)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