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

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.

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