Hi,
On 2/27/23 6:27 AM, Amit Kapila wrote:
On Fri, Feb 17, 2023 at 7:44 PM Drouvot, Bertrand
<bertranddrouvot...@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
