Remove UpdateFreeSpaceMap(), use FreeSpaceMapVacuumRange() instead.
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 29 Mar 2018 16:22:37 +0000 (12:22 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 29 Mar 2018 16:22:44 +0000 (12:22 -0400)
FreeSpaceMapVacuumRange has the same effect, is more efficient if many
pages are involved, and makes fewer assumptions about how it's used.
Notably, Claudio Freire pointed out that UpdateFreeSpaceMap could fail
if the specified freespace value isn't the maximum possible.  This isn't
a problem for the single existing user, but the function represents an
attractive nuisance IMO, because it's named as though it were a
general-purpose update function and its limitations are undocumented.
In any case we don't need multiple ways to get the same result.

In passing, do some code review and cleanup in RelationAddExtraBlocks.
In particular, I see no excuse for it to omit the PageIsNew safety check
that's done in the mainline extension path in RelationGetBufferForTuple.

Discussion: https://postgr.es/m/CAGTBQpYR0uJCNTt3M5GOzBRHo+-GccNO1nCaQ8yEJmZKSW5q1A@mail.gmail.com

src/backend/access/heap/hio.c
src/backend/storage/freespace/freespace.c
src/include/storage/freespace.h

index 42e75ec0b67ed1801090a01c83cdcb49d9514dc9..b8b5871559bd48134549837e9bd637f01e2ae64d 100644 (file)
@@ -177,13 +177,10 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
 static void
 RelationAddExtraBlocks(Relation relation, BulkInsertState bistate)
 {
-   Page        page;
-   BlockNumber blockNum = InvalidBlockNumber,
+   BlockNumber blockNum,
                firstBlock = InvalidBlockNumber;
-   int         extraBlocks = 0;
-   int         lockWaiters = 0;
-   Size        freespace = 0;
-   Buffer      buffer;
+   int         extraBlocks;
+   int         lockWaiters;
 
    /* Use the length of the lock wait queue to judge how much to extend. */
    lockWaiters = RelationExtensionLockWaiterCount(relation);
@@ -198,18 +195,40 @@ RelationAddExtraBlocks(Relation relation, BulkInsertState bistate)
     */
    extraBlocks = Min(512, lockWaiters * 20);
 
-   while (extraBlocks-- >= 0)
+   do
    {
-       /* Ouch - an unnecessary lseek() each time through the loop! */
+       Buffer      buffer;
+       Page        page;
+       Size        freespace;
+
+       /*
+        * Extend by one page.  This should generally match the main-line
+        * extension code in RelationGetBufferForTuple, except that we hold
+        * the relation extension lock throughout.
+        */
        buffer = ReadBufferBI(relation, P_NEW, bistate);
 
-       /* Extend by one page. */
        LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
        page = BufferGetPage(buffer);
+
+       if (!PageIsNew(page))
+           elog(ERROR, "page %u of relation \"%s\" should be empty but is not",
+                BufferGetBlockNumber(buffer),
+                RelationGetRelationName(relation));
+
        PageInit(page, BufferGetPageSize(buffer), 0);
+
+       /*
+        * We mark all the new buffers dirty, but do nothing to write them
+        * out; they'll probably get used soon, and even if they are not, a
+        * crash will leave an okay all-zeroes page on disk.
+        */
        MarkBufferDirty(buffer);
+
+       /* we'll need this info below */
        blockNum = BufferGetBlockNumber(buffer);
        freespace = PageGetHeapFreeSpace(page);
+
        UnlockReleaseBuffer(buffer);
 
        /* Remember first block number thus added. */
@@ -223,18 +242,15 @@ RelationAddExtraBlocks(Relation relation, BulkInsertState bistate)
         */
        RecordPageWithFreeSpace(relation, blockNum, freespace);
    }
+   while (--extraBlocks > 0);
 
    /*
     * Updating the upper levels of the free space map is too expensive to do
     * for every block, but it's worth doing once at the end to make sure that
     * subsequent insertion activity sees all of those nifty free pages we
     * just inserted.
-    *
-    * Note that we're using the freespace value that was reported for the
-    * last block we added as if it were the freespace value for every block
-    * we added.  That's actually true, because they're all equally empty.
     */
-   UpdateFreeSpaceMap(relation, firstBlock, blockNum, freespace);
+   FreeSpaceMapVacuumRange(relation, firstBlock, blockNum + 1);
 }
 
 /*
index 7eb4f3ee930141d75e8ac5c8a0ae1f76a9be6671..fd18c851140de77b194a95b7158d24ea3513dd2e 100644 (file)
@@ -111,8 +111,6 @@ static BlockNumber fsm_search(Relation rel, uint8 min_cat);
 static uint8 fsm_vacuum_page(Relation rel, FSMAddress addr,
                BlockNumber start, BlockNumber end,
                bool *eof);
-static BlockNumber fsm_get_lastblckno(Relation rel, FSMAddress addr);
-static void fsm_update_recursive(Relation rel, FSMAddress addr, uint8 new_cat);
 
 
 /******** Public API ********/
@@ -192,46 +190,6 @@ RecordPageWithFreeSpace(Relation rel, BlockNumber heapBlk, Size spaceAvail)
    fsm_set_and_search(rel, addr, slot, new_cat, 0);
 }
 
-/*
- * Update the upper levels of the free space map all the way up to the root
- * to make sure we don't lose track of new blocks we just inserted.  This is
- * intended to be used after adding many new blocks to the relation; we judge
- * it not worth updating the upper levels of the tree every time data for
- * a single page changes, but for a bulk-extend it's worth it.
- */
-void
-UpdateFreeSpaceMap(Relation rel, BlockNumber startBlkNum,
-                  BlockNumber endBlkNum, Size freespace)
-{
-   int         new_cat = fsm_space_avail_to_cat(freespace);
-   FSMAddress  addr;
-   uint16      slot;
-   BlockNumber blockNum;
-   BlockNumber lastBlkOnPage;
-
-   blockNum = startBlkNum;
-
-   while (blockNum <= endBlkNum)
-   {
-       /*
-        * Find FSM address for this block; update tree all the way to the
-        * root.
-        */
-       addr = fsm_get_location(blockNum, &slot);
-       fsm_update_recursive(rel, addr, new_cat);
-
-       /*
-        * Get the last block number on this FSM page.  If that's greater than
-        * or equal to our endBlkNum, we're done.  Otherwise, advance to the
-        * first block on the next page.
-        */
-       lastBlkOnPage = fsm_get_lastblckno(rel, addr);
-       if (lastBlkOnPage >= endBlkNum)
-           break;
-       blockNum = lastBlkOnPage + 1;
-   }
-}
-
 /*
  * XLogRecordPageWithFreeSpace - like RecordPageWithFreeSpace, for use in
  *     WAL replay
@@ -929,42 +887,3 @@ fsm_vacuum_page(Relation rel, FSMAddress addr,
 
    return max_avail;
 }
-
-/*
- * This function will return the last block number stored on given
- * FSM page address.
- */
-static BlockNumber
-fsm_get_lastblckno(Relation rel, FSMAddress addr)
-{
-   int         slot;
-
-   /*
-    * Get the last slot number on the given address and convert that to block
-    * number
-    */
-   slot = SlotsPerFSMPage - 1;
-   return fsm_get_heap_blk(addr, slot);
-}
-
-/*
- * Recursively update the FSM tree from given address to
- * all the way up to root.
- */
-static void
-fsm_update_recursive(Relation rel, FSMAddress addr, uint8 new_cat)
-{
-   uint16      parentslot;
-   FSMAddress  parent;
-
-   if (addr.level == FSM_ROOT_LEVEL)
-       return;
-
-   /*
-    * Get the parent page and our slot in the parent page, and update the
-    * information in that.
-    */
-   parent = fsm_get_parent(addr, &parentslot);
-   fsm_set_and_search(rel, parent, parentslot, new_cat, 0);
-   fsm_update_recursive(rel, parent, new_cat);
-}
index a2032518ac2acd96a681c27793255c5e4d94a9ec..726eb30fb807abb75f61081a8aafc0d3653efffa 100644 (file)
@@ -34,9 +34,5 @@ extern void FreeSpaceMapTruncateRel(Relation rel, BlockNumber nblocks);
 extern void FreeSpaceMapVacuum(Relation rel);
 extern void FreeSpaceMapVacuumRange(Relation rel, BlockNumber start,
                        BlockNumber end);
-extern void UpdateFreeSpaceMap(Relation rel,
-                  BlockNumber startBlkNum,
-                  BlockNumber endBlkNum,
-                  Size freespace);
 
 #endif                         /* FREESPACE_H_ */