btvacuumpage useless "orig_blkno"
I just noticed that btvacuumpage has two BlockNumber parameters -- blkno
and orig_blkno. The only caller passes them as the same value; the
header comments state that blkno would be different when recursing, but
actually the function implements recursion internally by way of a cute
"goto" trick. So it seems to me that the orig_blkno parameter is
useless -- we could just remove it.
Unless I'm completely missing something?
--
Álvaro Herrera <alvherre@alvh.no-ip.org>
Attachments:
btvacuumscan.patchapplication/octet-stream; name=btvacuumscan.patchDownload
*** a/src/backend/access/nbtree/nbtree.c
--- b/src/backend/access/nbtree/nbtree.c
***************
*** 71,78 **** static void btbuildCallback(Relation index,
static void btvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
IndexBulkDeleteCallback callback, void *callback_state,
BTCycleId cycleid);
! static void btvacuumpage(BTVacState *vstate, BlockNumber blkno,
! BlockNumber orig_blkno);
/*
--- 71,77 ----
static void btvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
IndexBulkDeleteCallback callback, void *callback_state,
BTCycleId cycleid);
! static void btvacuumpage(BTVacState *vstate, BlockNumber blkno);
/*
***************
*** 804,810 **** btvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
/* Iterate over pages, then loop back to recheck length */
for (; blkno < num_pages; blkno++)
{
! btvacuumpage(&vstate, blkno, blkno);
}
}
--- 803,809 ----
/* Iterate over pages, then loop back to recheck length */
for (; blkno < num_pages; blkno++)
{
! btvacuumpage(&vstate, blkno);
}
}
***************
*** 848,859 **** btvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
* must go back and re-examine previously-scanned pages; this routine
* recurses when necessary to handle that case.
*
! * blkno is the page to process. orig_blkno is the highest block number
! * reached by the outer btvacuumscan loop (the same as blkno, unless we
! * are recursing to re-examine a previous page).
*/
static void
! btvacuumpage(BTVacState *vstate, BlockNumber blkno, BlockNumber orig_blkno)
{
IndexVacuumInfo *info = vstate->info;
IndexBulkDeleteResult *stats = vstate->stats;
--- 847,857 ----
* must go back and re-examine previously-scanned pages; this routine
* recurses when necessary to handle that case.
*
! * blkno is the page to process.
! *
*/
static void
! btvacuumpage(BTVacState *vstate, BlockNumber blkno)
{
IndexVacuumInfo *info = vstate->info;
IndexBulkDeleteResult *stats = vstate->stats;
***************
*** 865,870 **** btvacuumpage(BTVacState *vstate, BlockNumber blkno, BlockNumber orig_blkno)
--- 863,874 ----
Buffer buf;
Page page;
BTPageOpaque opaque;
+ /*
+ * orig_blkno is the highest block number reached by the outer btvacuumscan
+ * loop (the same as blkno, unless we are recursing to re-examine a
+ * previous page).
+ */
+ BlockNumber orig_blkno = blkno;
restart:
delete_now = false;
On Mon, Nov 21, 2011 at 10:03 PM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:
I just noticed that btvacuumpage has two BlockNumber parameters -- blkno
and orig_blkno. The only caller passes them as the same value; the
header comments state that blkno would be different when recursing, but
actually the function implements recursion internally by way of a cute
"goto" trick. So it seems to me that the orig_blkno parameter is
useless -- we could just remove it.Unless I'm completely missing something?
tail recursion - read comments at bottom of the function
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Excerpts from Simon Riggs's message of lun nov 21 19:11:21 -0300 2011:
On Mon, Nov 21, 2011 at 10:03 PM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:I just noticed that btvacuumpage has two BlockNumber parameters -- blkno
and orig_blkno. The only caller passes them as the same value; the
header comments state that blkno would be different when recursing, but
actually the function implements recursion internally by way of a cute
"goto" trick. So it seems to me that the orig_blkno parameter is
useless -- we could just remove it.Unless I'm completely missing something?
tail recursion - read comments at bottom of the function
Right, but we don't need to pass the value as a parameter, we can just
save it at the start of the function, as my proposed patch does, right?
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Simon Riggs's message of lun nov 21 19:11:21 -0300 2011:
tail recursion - read comments at bottom of the function
Right, but we don't need to pass the value as a parameter, we can just
save it at the start of the function, as my proposed patch does, right?
If you do this, it's not really tail recursion anymore, so the comments
need to be adjusted. The patch sounds reasonable, but you have more
work to do to fix the comments ...
regards, tom lane