Re: Change xl_hash_vacuum_one_page.ntuples from int to uint16

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.

> 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. 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.

--
With Regards,
Amit Kapila.